Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Add Seek() method to nfs.File #5

Merged
merged 3 commits into from
Oct 27, 2017
Merged

Conversation

kibab
Copy link

@kibab kibab commented Oct 25, 2017

Implement Seek() as required by io.Seeker() interface specification.
As there is no predefined interface that implements both Read(), Write(),
Seek() and Close(), return nfs.File from Open() and OpenFile() calls
instead of io.
interfaces.
So now nfs.File implements even more interfaces, including io.ReadSeeker(), io.ReadWriteCloser()
and so on.

Implement Seek() as required by io.Seeker() interface specification.
As there is no predefined interface that implements both Read(), Write(),
Seek() and Close(), return *nfs.File from Open() and OpenFile() calls
instead of io.* interfaces.
So now nfs.File implements even more interfaces, including io.ReadSeeker(), io.ReadWriteCloser()
and so on.
@vmwclabot
Copy link

@kibab, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

nfs/file.go Outdated
switch whence {
case io.SeekStart:
if offset < 0 {
return int64(f.curr), fmt.Errorf("offset cannot be negative")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use errors.New instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will!

nfs/file.go Outdated
f.curr = uint64(int64(f.curr) + offset)
return int64(f.curr), nil
case io.SeekEnd:
return int64(f.curr), fmt.Errorf("SeekEnd is not supported yet")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment. errors.New()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kibab
Copy link
Author

kibab commented Oct 25, 2017

Well, before submitting the updated version of the patch I have to go through approval process for VMWare CLA, as I'm submitting this as Google employee and Google doesn't allow signing CLAs that are not approved yet. I'll get back to you shortly.

@kibab
Copy link
Author

kibab commented Oct 26, 2017

I got an approval from Google to sign VMWare CLA. As soon as VMWare receives all required information from Google we can proceed with this pull request!

@kibab
Copy link
Author

kibab commented Oct 26, 2017

CLA thing is sorted out!

nfs/file.go Outdated
return int64(f.curr), errors.New("SeekEnd is not supported yet")
default:
// This indicates serious programming error
panic("Invalid whence")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking that panic might be heavy handed here? My main concern is people using this as a client may be surprised if it causes an unhandled panic unexpectedly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot imagine situation other than developer's mistake that could lead to "whence" values being something else than those three values (since this is more something you get from outside). But no problem, I'll update the code to just return an error instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant "not something" of course...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@matthewavery
Copy link

Thanks this looks great!!!

@matthewavery matthewavery merged commit 5547f82 into vmware-archive:master Oct 27, 2017
@vmwclabot
Copy link

@kibab, your company's legal contact did not review your signed contributor license agreement within the 14 day limit. The merge can not proceed. Click here to resign the agreement.

willscott pushed a commit to willscott/go-nfs-client that referenced this pull request Jan 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants