bus-unit-util: fix bus_wait_for_jobs() debug output #8760
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We shouldn't print 'errno' if its not initialized properly.
|
lgtm. |
keszybz
reviewed
Apr 23, 2018
| @@ -1972,6 +1972,8 @@ int bus_wait_for_jobs(BusWaitForJobs *d, bool quiet, const char* const* extra_ar | |||
| if (q < 0 && r == 0) | |||
| r = q; | |||
|
|
|||
| errno = 0; /* Reset errno explicitly, so that log_debug_errno() will properly print 'Success' | |||
| * for q == 0, instead of whatever is set in errno */ | |||
There was a problem hiding this comment.
Shouldn't we just fix log_internalv_realm to unconditionally override errno, even if the value passed as the argument is 0. Seems like a much better solution.
There was a problem hiding this comment.
When we originally added the errno patching we went for a "best of both worlds" approach, i.e. that we override errno if an error is specified, but if no error is specified (i.e. 0 is passed as error code) then we use the previously set errno, similar in style how plain printf() would do it. In retrospect I think we almost never purposefully made use of the second, i.e. the plain printf() logic, but we multiple times — like in this case the bugfix is about — ran into this case accidentally and introduced a bug. Hence yes, it probably makes sense to switch this over, and consistently ignore the errno already set and always override it with the error passed in. The only problem I see with that is: I wonder if there might be a case or two lurking somewhere where we actually made use of the "best of both worlds" approach, and if so, if we can detect where... (But then again, even if there is, and we fail to find those cases, maybe that's not all bad, as it's just a few new bugs against probably fixing many more old and future bugs, if you follow what I mean).
So yeah, might be something to do indeed, we just need to be careful.
keszybz
added a commit
to keszybz/systemd
that referenced
this pull request
Apr 24, 2018
Quoting systemd#8760 (comment): > When we originally added the errno patching we went for a "best of both > worlds" approach, i.e. that we override errno if an error is specified, but > if no error is specified (i.e. 0 is passed as error code) then we use the > previously set errno, similar in style how plain `printf()` would do it. In > retrospect I think we almost never purposefully made use of the second, > i.e. the plain `printf()` logic, but we multiple times ran into this case > accidentally and introduced a bug. Hence yes, it probably makes sense to > switch this over, and consistently ignore the `errno` already set and always > override it with the error passed in. The only problem I see with that is: I > wonder if there might be a case or two lurking somewhere where we actually > made use of the "best of both worlds" approach, and if so, if we can detect > where... (But then again, even if there is, and we fail to find those cases, > maybe that's not all bad, as it's just a few new bugs against probably fixing > many more old and future bugs, if you follow what I mean). I scanned our codebase, and found some bugs in the value passed to log_*_errno, but no intentional cases of error=0 being passed.
keszybz
added a commit
to systemd/systemd-stable
that referenced
this pull request
May 11, 2018
Quoting systemd/systemd#8760 (comment): > When we originally added the errno patching we went for a "best of both > worlds" approach, i.e. that we override errno if an error is specified, but > if no error is specified (i.e. 0 is passed as error code) then we use the > previously set errno, similar in style how plain `printf()` would do it. In > retrospect I think we almost never purposefully made use of the second, > i.e. the plain `printf()` logic, but we multiple times ran into this case > accidentally and introduced a bug. Hence yes, it probably makes sense to > switch this over, and consistently ignore the `errno` already set and always > override it with the error passed in. The only problem I see with that is: I > wonder if there might be a case or two lurking somewhere where we actually > made use of the "best of both worlds" approach, and if so, if we can detect > where... (But then again, even if there is, and we fail to find those cases, > maybe that's not all bad, as it's just a few new bugs against probably fixing > many more old and future bugs, if you follow what I mean). I scanned our codebase, and found some bugs in the value passed to log_*_errno, but no intentional cases of error=0 being passed. (cherry picked from commit b29f648)
xnox
pushed a commit
to xnox/systemd
that referenced
this pull request
May 31, 2018
We shouldn't print 'errno' if its not initialized properly. (cherry picked from commit d6b8763)
Yamakuzure
pushed a commit
to elogind/elogind
that referenced
this pull request
Jun 6, 2018
Quoting systemd/systemd#8760 (comment): > When we originally added the errno patching we went for a "best of both > worlds" approach, i.e. that we override errno if an error is specified, but > if no error is specified (i.e. 0 is passed as error code) then we use the > previously set errno, similar in style how plain `printf()` would do it. In > retrospect I think we almost never purposefully made use of the second, > i.e. the plain `printf()` logic, but we multiple times ran into this case > accidentally and introduced a bug. Hence yes, it probably makes sense to > switch this over, and consistently ignore the `errno` already set and always > override it with the error passed in. The only problem I see with that is: I > wonder if there might be a case or two lurking somewhere where we actually > made use of the "best of both worlds" approach, and if so, if we can detect > where... (But then again, even if there is, and we fail to find those cases, > maybe that's not all bad, as it's just a few new bugs against probably fixing > many more old and future bugs, if you follow what I mean). I scanned our codebase, and found some bugs in the value passed to log_*_errno, but no intentional cases of error=0 being passed. (cherry picked from commit b29f6480eca0550ba65d30fbece8dd4d4bfe666d)
Yamakuzure
pushed a commit
to elogind/elogind
that referenced
this pull request
Jun 29, 2018
Quoting systemd/systemd#8760 (comment): > When we originally added the errno patching we went for a "best of both > worlds" approach, i.e. that we override errno if an error is specified, but > if no error is specified (i.e. 0 is passed as error code) then we use the > previously set errno, similar in style how plain `printf()` would do it. In > retrospect I think we almost never purposefully made use of the second, > i.e. the plain `printf()` logic, but we multiple times ran into this case > accidentally and introduced a bug. Hence yes, it probably makes sense to > switch this over, and consistently ignore the `errno` already set and always > override it with the error passed in. The only problem I see with that is: I > wonder if there might be a case or two lurking somewhere where we actually > made use of the "best of both worlds" approach, and if so, if we can detect > where... (But then again, even if there is, and we fail to find those cases, > maybe that's not all bad, as it's just a few new bugs against probably fixing > many more old and future bugs, if you follow what I mean). I scanned our codebase, and found some bugs in the value passed to log_*_errno, but no intentional cases of error=0 being passed. (cherry picked from commit b29f6480eca0550ba65d30fbece8dd4d4bfe666d)
Yamakuzure
pushed a commit
to elogind/elogind
that referenced
this pull request
Aug 23, 2018
Quoting systemd/systemd#8760 (comment): > When we originally added the errno patching we went for a "best of both > worlds" approach, i.e. that we override errno if an error is specified, but > if no error is specified (i.e. 0 is passed as error code) then we use the > previously set errno, similar in style how plain `printf()` would do it. In > retrospect I think we almost never purposefully made use of the second, > i.e. the plain `printf()` logic, but we multiple times ran into this case > accidentally and introduced a bug. Hence yes, it probably makes sense to > switch this over, and consistently ignore the `errno` already set and always > override it with the error passed in. The only problem I see with that is: I > wonder if there might be a case or two lurking somewhere where we actually > made use of the "best of both worlds" approach, and if so, if we can detect > where... (But then again, even if there is, and we fail to find those cases, > maybe that's not all bad, as it's just a few new bugs against probably fixing > many more old and future bugs, if you follow what I mean). I scanned our codebase, and found some bugs in the value passed to log_*_errno, but no intentional cases of error=0 being passed.
Yamakuzure
pushed a commit
to elogind/elogind
that referenced
this pull request
Aug 24, 2018
Quoting systemd/systemd#8760 (comment): > When we originally added the errno patching we went for a "best of both > worlds" approach, i.e. that we override errno if an error is specified, but > if no error is specified (i.e. 0 is passed as error code) then we use the > previously set errno, similar in style how plain `printf()` would do it. In > retrospect I think we almost never purposefully made use of the second, > i.e. the plain `printf()` logic, but we multiple times ran into this case > accidentally and introduced a bug. Hence yes, it probably makes sense to > switch this over, and consistently ignore the `errno` already set and always > override it with the error passed in. The only problem I see with that is: I > wonder if there might be a case or two lurking somewhere where we actually > made use of the "best of both worlds" approach, and if so, if we can detect > where... (But then again, even if there is, and we fail to find those cases, > maybe that's not all bad, as it's just a few new bugs against probably fixing > many more old and future bugs, if you follow what I mean). I scanned our codebase, and found some bugs in the value passed to log_*_errno, but no intentional cases of error=0 being passed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We shouldn't print 'errno' if its not initialized properly.