Unifying boilerplate code for handling child processes - part 3#8203
Conversation
e8040d7 to
4eac248
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a good step towards unifying boilerplate code for handling child processes, specifically by centralizing timeout handling. The changes are consistent and remove a significant amount of redundant code. However, I've identified a critical issue related to potential NULL pointer dereferences in the new timeout handling logic. A memory allocation failure in the new helper function sss_child_create_timeout_cb_pvt is not handled by the callers, which could lead to a crash. I've provided two review comments with suggestions to make the new code more robust against such failures.
4eac248 to
22a31c7
Compare
alexey-tikhonov
added
coverity
|
Note: Covscan is green. |
thalman
left a comment
thalman
left a comment
There was a problem hiding this comment.
Codewise it looks great, thank you Alexey. Please, fix the commit message.
22a31c7 to
b65c1da
Compare
|
(rebased) |
b65c1da to
de664bb
Compare
pbrezina
left a comment
pbrezina
left a comment
There was a problem hiding this comment.
Ack, this is a nice improvement.
It might be better to use tevent_req_endtime, but since that would mean reworking the whole patch set, lets keep it as is.
I didn't know about this function... |
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
and make use of it in gpo/ifp/ssh code. Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
'nsupdate_child_state::timeout_handler' wasn't really used. Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Unused since c7e2d7a Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Pavel Březina <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
de664bb to
63f5a3c
Compare
That can be handled in _recv, we already convert it to ETIMEDOUT. https://github.com/SSSD/sssd/blob/master/src/util/util.h#L158 |
Currently 'EFAULT', 'ERR_RENEWAL_CHILD', 'ERR_DYNDNS_TIMEOUT', 'ERR_P11_CHILD_TIMEOUT' and 'ERR_PASSKEY_CHILD_TIMEOUT' are also used. |
4 participants
Follow up of #7991 / #8174
I also wanted to make 'proxy' use
sss_child_start()but that appeared to be not that straightforward:_iooutput arg => no way to get the pid of child processIt could be possible to amend
sss_child_start()to return 'pid' in a different way but... imo, a better solution would be to fix 'proxy' to end child process via sbus communication, not relying on a signal. This is out of scope for this PR (even thought keepingpc_init_sig_handler()that practically is a copy ofchild_sig_handler()is frustrating).