Unifying boilerplate code for handling child processes - part 3#8203
Unifying boilerplate code for handling child processes - part 3#8203alexey-tikhonov merged 16 commits intoSSSD:masterfrom
Conversation
e8040d7 to
4eac248
Compare
|
/gemini review |
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
|
Note: Covscan is green. |
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
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 <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
and make use of it in gpo/ifp/ssh code. Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
'nsupdate_child_state::timeout_handler' wasn't really used. Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Unused since c7e2d7a Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Reviewed-by: Pavel Březina <pbrezina@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
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. |
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).