search

EMCECS / nfs-client-java

Native Java NFS client
Apache License 2.0
70 stars 33 forks source link

Fixed SETATTR sending the wrong ctime #22

Closed amarcionek closed 5 years ago

amarcionek commented 5 years ago

According to rfc1813, the settattr request includes a sattrguard3 for the guard time which includes a boolean and then a nfstime. The current NfsTime class's marshalling assumes the structure is used for the atime or mtime set_atime and set_mtime respectively which includes _timeSettingType in the request.

I figured the easiest way to accomplish this was to expose the seconds and nanoseconds values in time and use a custom marshal for NfsSetAttrRequest.java. The alternative would be to add some context NfsTime.java to know which request it's being used it, but it seemed like a lot of context when only the setattr request needed. So here you go.

DavidASeibert commented 5 years ago

Thanks, @amarcionek . I am looking at this now. Do you have a test that exposes the issue fixed by this change? If so, can you add it to the unit tests? I'd like to make sure that this fix is protected, so it doesn't get broken by a future change.

amarcionek commented 5 years ago

@DavidASeibert, the PR includes a test in Test_Nfs3.java. Is that the correct place?

DavidASeibert commented 5 years ago

@amarcionek Yes, that was the correct place. I should have checked for a test first.

The basic fix works as far as getting the marshaling correct, but I think for usability it needs either a slight class hierarchy tweak or a change to the NfsTime class, I haven't decided which. Here are the options as I see them.

Class hierarchy tweak - add a specialization of NfsTime, probably NfsGuardTime, that does the marshaling you implement (marshals the boolean then the numbers). That and NfsTime should both inherit from a new abstract NfsTimeBase, which handles just the numbers, so that code isn't duplicated.

NfsTime tweak (uglier, but an alternative) - add a boolean isGuardTime to NfsTime, and modify the marshaling accordingly, again avoiding duplicate marshaling code.

I would lean towards the first as being clearer to end users. Thoughts? I may not get back to this right away, as I will be traveling on business until Thursday.

amarcionek commented 5 years ago

My initial implementation looked like your second option, but I scratched it for the same reasons you mentioned...it was hacky. I agree with your first option because the current NfsTime object sort of assumes its used to set the time (thus the type is embedded in it.)

How about class names that are closer to the rfc? NfsTime is the base class (based off of nfstime3), NfsGuardTime (based off of sattrguard3) and NfsSetTime (based off of set_atime and set_mtime ) are derived from it, with the boolean and the type respectively?

I'm very busy early in this week, but might be able to revise this near the end, will update the PR if so.

DavidASeibert commented 5 years ago

@amarcionek I have some time to work on this now, so I created a separate branch for this change (feature-guard-time) so we can both contribute. I will merge your changes in there and then tweak them a bit. If you make further changes, I can merge those as well.

DavidASeibert commented 5 years ago

Merged content into new PR for editing. https://github.com/EMCECS/nfs-client-java/pull/25