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

Support SEEK_DATA and SEEK_HOLE for lseek #486

Merged
merged 11 commits into from
Jan 3, 2023

Conversation

sharifhsn
Copy link
Contributor

@sharifhsn sharifhsn commented Dec 13, 2022

Fixes #480

Adds two new variants to the SeekFrom API, Data and Hole.

Some questions:

  • Are the docs fine? They're basically copied from the man page.
  • Should the staged_api attribute and other aspects be used? I'm not sure what their purpose is but I included it just in case.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

* Are the docs fine? They're basically copied from [the man page](https://man7.org/linux/man-pages/man2/lseek.2.html).

I'd like to avoid just copying the text like this; could you instead either write your own text, or make it a quote with a citation?

And actually, looking at the text, I'm having trouble parsing "Sets the offset to next location in the file greater than or equal to the specified number of bytes that contains data." Would it be correct to rephrase it along the lines of "seeks forward from the current position by the specified number of bytes, and then if that's in a hole, seeks forward until it reaches data" or so?

* Should the `staged_api` attribute and other aspects be used? I'm not sure what their purpose is but I included it just in case.

Those are there because that file is derived from a file in std, and it's easier to keep up to date if we keep the diffs minimal. For these new values, we don't need to add them.

src/io/seek_from.rs Outdated Show resolved Hide resolved
@sharifhsn
Copy link
Contributor Author

Would it be correct to rephrase it along the lines of "seeks forward from the current position by the specified number of bytes, and then if that's in a hole, seeks forward until it reaches data" or so?

Yes, I think this would be much clearer. My only concern is if it's preferable that the wording of the documentation remain consistent across SeekFrom variants?

@sunfishcode
Copy link
Member

How about, "Sets the offset to the current position plus the specified number of bytes, plus the distance to the next byte which is not in a hole."?

@sunfishcode
Copy link
Member

The change here looks good to me.

That said, it looks like this change needs a semver bump, because users using std::io::SeekFrom will need to switch to rustix's. I'm hoping to avoid doing a semver bump in rustix for a while. If you'd like this feature to show up in a release sooner, what would you think of renaming seek to seek_ext or something, and re-introducing the old seek that takes a std::io::SeekFrom, so that it's semver-compatible?

@sharifhsn
Copy link
Contributor Author

sharifhsn commented Dec 21, 2022

I'm not pressed for this feature currently, I just was interested in its absence in the API. My use for it is in an experimental PR which will not be merged soon, if ever. This should probably be placed in some kind of 0.37 milestone or something.

@sunfishcode
Copy link
Member

Sounds good. I've now added a 0.37 label and labelled this PR with it.

@sunfishcode
Copy link
Member

sunfishcode commented Jan 3, 2023

#487 landed, which will need a semver bump, so let's land this too.

@sunfishcode sunfishcode merged commit 5e79bf7 into bytecodealliance:main Jan 3, 2023
@sunfishcode sunfishcode mentioned this pull request Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sparse file support for lseek?
2 participants