-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pull request for combined inputs to guard time functionality #25
Conversation
Creating combined fix branch
Added checks for correct time type (setting or guard). Added test for guard time stopping modification. Modified copyright dates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made 2 nitpicky comments, but looks good to me. I'll test it on Monday too. Thanks for doing the work!
/** | ||
* This usually indicates a guard time - attributes should be changed only | ||
* if the object's ctime matches the associated time, as specified by RFC | ||
* 1813 (https://tools.ietf.org/html/rfc1813). The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra 'The' in the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up.
@@ -110,8 +138,10 @@ public String toString() { | |||
* @see com.emc.ecs.nfsclient.nfs.NfsRequest#marshalling(com.emc.ecs.nfsclient.rpc.Xdr) | |||
*/ | |||
public void marshalling(Xdr xdr) { | |||
xdr.putInt(_timeSettingType); | |||
if (SET_TO_CLIENT_TIME == _timeSettingType) { | |||
if ( IS_BARE_TIME != _timeSettingType ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to use if (!isBareTime())
instead? Nitpicking, I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Cleaned that up.
guardTime, | ||
new CredentialUnix(), 3)); | ||
|
||
assertEquals(NfsStatus.NFS3ERR_NOT_SYNC.getValue(), setAttrResponse.getState()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NICE test case there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem to just undo 8ad3495? Maybe I'm misunderstanding.
Will this be published to maven central as a new release? |
Yes, probably as soon as I get copyrights updated and have time for a release. |
Any comments on this version, @amarcionek ? This should contain the same changes, with some of the class hierarchy changes but also preserving backwards compatibility for all code that doesn't use guard time. I didn't worry about the guard time code, since that wasn't working before the fix. I did tighten up input checking, so that clear exceptions would be raised immediately if code used the wrong time type.