Skip to content

main/streams/plain_wrapper: skip lseek(SEEK_CUR) for newly opened files #8540

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

main/streams/plain_wrapper: skip lseek(SEEK_CUR) for newly opened files #8540

wants to merge 1 commit into from

Conversation

MaxKellermann
Copy link
Contributor

A file that has just been opened is known to be at offset zero, and
the lseek(SEEK_CUR) system call to determine the current offset can be
skipped.

@cmb69
Copy link
Member

cmb69 commented May 12, 2022

A file that has just been opened is known to be at offset zero,

What about open mode "a"?

@MaxKellermann
Copy link
Contributor Author

What about open mode "a"?

Good point - I'll amend the patch to exclude append mode.

@ramsey ramsey added this to the PHP 8.2 milestone May 21, 2022
@devnexen devnexen requested a review from cmb69 June 18, 2022 05:22
cmb69
cmb69 reviewed Jun 29, 2022
View reviewed changes
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you! This looks reasonable to me, although I'm not too happy with the (minor) BC break.

@MaxKellermann
Copy link
Contributor Author

Which BC break do you mean?

@cmb69
Copy link
Member

cmb69 commented Jun 29, 2022

Which BC break do you mean?

The additional parameter to php_stream_fopen_from_fd_rel() which may affect extensions.

@MaxKellermann
main/streams/plain_wrapper: skip lseek(SEEK_CUR) for newly opened files
5aad60b 
A file that has just been opened is known to be at offset zero, and
the lseek(SEEK_CUR) system call to determine the current offset can be
skipped.
@MaxKellermann
Copy link
Contributor Author

The additional parameter to php_stream_fopen_from_fd_rel() which may affect extensions.

If that's important, I can rename the function and add the old one as compatibility wrapper, but that adds to the legacy baggage the code base has to carry around. You decide which is better, and I can amend the PR.

cmb69
cmb69 approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you!

I think the potential BC break is acceptable; it just needs an entry in UPGRADING.INTERNALS.

devnexen
devnexen approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

LGTM. Another nice change ;-)

@devnexen devnexen closed this in e2bd3b1 Jun 29, 2022
@MaxKellermann MaxKellermann deleted the omit_lseek branch June 30, 2022 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmb69 cmb69 cmb69 approved these changes

@devnexen devnexen devnexen approved these changes

Assignees
No one assigned
Labels
Category: Streams Waiting on Review
Projects
None yet
Milestone
PHP 8.2
Development

Successfully merging this pull request may close these issues.
4 participants
@MaxKellermann @cmb69 @devnexen @ramsey