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

ccan: fix read_all() to always set errno on failure #3330

Closed
wants to merge 1 commit into from

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Dec 11, 2019

read(2) will return 0 to designate end-of-file and in this case it will
not set errno.

This is not considered as an error condition by read(2) itself, but all
callers of read_all() expect that exactly the requested number of bytes
will be read into the buffer, not less, so set errno to EBADMSG
"Bad message" in this case.

Related to #3185

read(2) will return 0 to designate end-of-file and in this case it will
not set errno.

This is not considered as an error condition by read(2) itself, but all
callers of read_all() expect that exactly the requested number of bytes
will be read into the buffer, not less, so set errno to EBADMSG
"Bad message" in this case.

Related to ElementsProject#3185
@vasild
Copy link
Contributor Author

vasild commented Dec 11, 2019

I chose EBADMSG arbitrarily:

     89 EBADMSG Bad message. A corrupted message was detected.

maybe EPROTO is also a good choice:

     92 EPROTO Protocol error. A device or socket encountered an unrecoverable
             protocol error.

@cdecker
Copy link
Member

cdecker commented Dec 11, 2019

Thanks for the PR @vasild. This is part of ccan which is tracked externally at https://github.com/rustyrussell/ccan. Could you open a PR there? If accepted we'd update the in-tree version from the upstream version in a second step.

@cdecker cdecker added the state::upstream This PR relates to an upstream dependency and needs to be submitted to the upstream repository. label Dec 11, 2019
@vasild
Copy link
Contributor Author

vasild commented Dec 11, 2019

@cdecker yes, sorry for the confusion.

continues at: rustyrussell/ccan#86

@vasild vasild closed this Dec 11, 2019
@cdecker
Copy link
Member

cdecker commented Dec 11, 2019

No problem ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state::upstream This PR relates to an upstream dependency and needs to be submitted to the upstream repository.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants