sbus: defer notification callbacks#8202
Conversation
![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 aims to fix a race condition with nested requests by deferring sbus notification callbacks. The changes achieve this by adding the tevent context to the sbus_request_list structure and using tevent_req_defer_callback when a request is finished. The overall approach is correct and directly addresses the issue.
I have one suggestion to improve robustness by adding a NULL check to prevent a potential crash, which I've detailed in a specific comment.
a486716 to
4c041f4
Compare
e3ddcb3 to
ce20e05
Compare
|
The PR CI failures may be related, although I see some issues in the master branch first. I will restart it one more time and then investigate. |
34efd07 to
ad9b35a
Compare
|
The patch works now correctly, previously there was a race condition when cr_domains may not have been yet populated and it resulted in a random crash. It is ready for review, remaining failures are not related. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with nested request chaining in sbus by deferring notification callbacks. The core change in sbus_request_hash.c uses tevent_req_defer_callback to ensure that requests are fully processed and cleaned up before their callbacks are executed, which correctly resolves the potential for incorrect chaining. The necessary event context is now stored in sbus_request_list to support this. A new integration test has been added to test_identity.py which effectively validates the fix by checking for a hang that occurred in the problematic scenario. Additionally, there's a small optimization in cache_req.c to only use cache_first behavior when multiple domains are present. The changes are logical, well-implemented, and the new test provides good coverage for the fix. I have no further comments.
|
Note: Covscan is green. |
Maybe unimportant, but looking at updates I don't understand where race was (or how it was fixed)... |
alexey-tikhonov
added
backport-to-sssd-2-9-4
ad9b35a to
821b0a2
Compare
The original patch relayed on cr_domains, which is flatten list of domain names for search resolution order. If we use IPA provider, we do not read the option from configuration file but from the server. In this case, the cr_domains was not yet populated at this time, it was NULL. The fix was to rely on rctx->domain. |
Otherwise, it is possible to incorrectly chain nested requests since the chain id is still present in the hash table. Resolves: SSSD#8194 Reviewed-by: Alexey Tikhonov <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Otherwise we will perform unnecessary data provider lookups. Resolves: SSSD#8194 Reviewed-by: Alexey Tikhonov <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Alexey Tikhonov <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
821b0a2 to
00b1cc1
Compare
4 participants
Otherwise, it is possible to incorrectly chain nested requests since the
chain id is still present in the hash table.
Resolves: #8194