-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: Add ReadAt function for fileNode to allow gobinary extractor to successfully extract. #371
base: main
Are you sure you want to change the base?
Conversation
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 for adding the ReadAt
method Rex! Looks good, though tests might be a good idea.
@@ -67,6 +67,20 @@ func (f *fileNode) Read(b []byte) (n int, err error) { | |||
return f.file.Read(b) | |||
} | |||
|
|||
// Read reads the real file referred to by the fileNode. |
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.
Comment should start with "ReadAt".
You think we could refactor the Read
method? Wondering if Read(b)
is equivalent to ReadAt(b, 0)
.
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.
It's not quite the same, ReadAt will return io.EOF error if the buffer is bigger than what is there to read, but Read won't do that unless you do a second Read at the end of the file.
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.
Gotcha. The code lgtm then!
@@ -67,6 +67,20 @@ func (f *fileNode) Read(b []byte) (n int, err error) { | |||
return f.file.Read(b) | |||
} | |||
|
|||
// Read reads the real file referred to by the fileNode. | |||
func (f *fileNode) ReadAt(b []byte, off int64) (n int, err error) { |
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.
Oh, it might also be a good idea to add a few tests? Probably doesn't many given the Read
method is sufficiently tested.
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.
Good idea, added the tests! We might want to switch the wantErr in other tests to use the cmp library, since that also fails when wantErr is true but no error is returned.
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.
Yeah.... I'll create a bug to remind myself of changing wantErr
from other tests. Thanks for the note and the added tests!
… successfully extract.
477772e
to
e8bc5e8
Compare
@@ -67,6 +67,20 @@ func (f *fileNode) Read(b []byte) (n int, err error) { | |||
return f.file.Read(b) | |||
} | |||
|
|||
// Read reads the real file referred to by the fileNode. | |||
func (f *fileNode) ReadAt(b []byte, off int64) (n int, err error) { |
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.
Yeah.... I'll create a bug to remind myself of changing wantErr
from other tests. Thanks for the note and the added tests!
The gobinary extractor expects a ReaderAt, but fileNode does not implement it. The underlying file does though, so this just exposes it.