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

erts: Fix child_setup partial read #5861

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

garazdawi
Copy link
Contributor

When calling read it may return a partial result. So we make
sure that all the data is read before returning.

The problem has been observed when reading the ACK message,
but we use the new routine for all reads in the child so that
we know that it will work everywhere.

@garazdawi garazdawi added team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI bug Issue is reported as a bug labels Apr 5, 2022
@garazdawi garazdawi added this to the OTP-25.0 milestone Apr 5, 2022
@garazdawi garazdawi requested a review from sverker April 5, 2022 08:50
@garazdawi garazdawi self-assigned this Apr 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2022

CT Test Results

       3 files     125 suites   39m 20s ⏱️
1 439 tests 1 397 ✔️ 42 💤 0
1 737 runs  1 678 ✔️ 59 💤 0

Results for commit efc7f4c.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@garazdawi garazdawi force-pushed the lukas/erts/fix-child-setup-read branch from 0ccb83e to 96d7edd Compare April 5, 2022 09:02
Copy link
Contributor

@sverker sverker left a comment

Choose a reason for hiding this comment

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

In main there is a read(sigchld_pipe[0], ibuff, sizeof(ibuff));.
Should that also use read_all?

What about a write_all? write can also return partial results according to docs.

@garazdawi
Copy link
Contributor Author

In main there is a read(sigchld_pipe[0], ibuff, sizeof(ibuff));.
Should that also use read_all?

Yes, might as well change that one also.

What about a write_all? write can also return partial results according to docs.

I've never seen a write do a partial write in practice... though I suppose it does not hurt to be extra safe.

@garazdawi
Copy link
Contributor Author

Fixed, please review again @sverker.

Copy link
Contributor

@sverker sverker left a comment

Choose a reason for hiding this comment

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

Ok, except I don't get the point of that EPIPE condition in neither write_all or read_all.

@garazdawi
Copy link
Contributor Author

except I don't get the point of that EPIPE condition in neither write_all or read_all.

when 0 is read/written that is usually an indication that the pipe has been closed. Testcases for the old implementation of child_setup checked that this was the error code returned, so to be compatible I've had to return EPIPE for all of those as well. Strictly speaking I think it may only be one or two of the reads where is behaviour can be observed by the user, but I decided to add it for all just to be consistent.

When calling read it may return a partial result. So we make
sure that all the data is read before returning.

The problem has been observed when reading the ACK message,
but we use the new routine for all reads in the child so that
we know that it will work everywhere.
@garazdawi garazdawi force-pushed the lukas/erts/fix-child-setup-read branch from efc7f4c to 0d17cd6 Compare April 7, 2022 07:30
@garazdawi garazdawi merged commit 4d32d83 into erlang:master Apr 7, 2022
@garazdawi garazdawi deleted the lukas/erts/fix-child-setup-read branch April 7, 2022 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants