Skip to content
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

Fix: IO::FileDescriptor#seek from current position #4558

Conversation

ysbaddaden
Copy link
Contributor

Trying to seek from the current position failed because the system position of a buffered IO is different from the actual position in Crystal land. For example reading 4 bytes from a File actually reads up to BUFFER_SIZE bytes. Seeking back 4 bytes should return to position 0, but since the system read more bytes, it ended up at an unexpected place.

seek_value = LibC.lseek(@fd, offset, whence)

if whence.current?
seek_value = LibC.lseek(@fd, tell + offset, Seek::Set)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not be acceptable simply to remove @in_buffer_rem.size from the offset here and not use Seek::Set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess. It would avoid a second lseek call.

Trying to seek from the current position failed because the system
position of a buffered IO is different from the actual position in
the stream.
@ysbaddaden ysbaddaden force-pushed the fix/io-filedescriptor-seek-from-current branch from 6affbd6 to a862c51 Compare June 13, 2017 09:52
@asterite asterite merged commit 789421c into crystal-lang:master Jun 13, 2017
@asterite
Copy link
Member

@ysbaddaden Good catch, thanks!

@ysbaddaden ysbaddaden deleted the fix/io-filedescriptor-seek-from-current branch June 13, 2017 12:34
@mverzilli mverzilli added this to the Next milestone Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants