memberOf plugin: avoid ldb_dn_compare() in mbof_append_addop()#8546
Conversation
alexey-tikhonov
added
backport-to-sssd-2-9
Performance
c2aa375 to
24ec863
Compare
![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 refactors debug logging in several LDAP provider files by replacing direct DEBUG calls with DEBUG_CONDITIONAL for improved performance. It also updates the memberof LDB module to use linearized DNs for comparison, changing the comparison logic from ldb_dn_compare to sss_linearized_dn_match. Feedback indicates a potential issue in memberof.c where returning LDB_SUCCESS when ldb_dn_get_linearized fails might mask critical errors, and also notes an unaddressed FIXME comment that requires clarification regarding parent comparisons with the new DN matching logic.
I am having trouble creating individual review comments. Click here to see my feedback.
src/ldb_modules/memberof.c (349-351)
Returning LDB_SUCCESS when ldb_dn_get_linearized fails might mask a critical issue. If the entry_dn is invalid or cannot be linearized, it could indicate a problem that should be handled as an error rather than silently succeeding. This could lead to add operations being incorrectly ignored, causing data inconsistency or unexpected behavior.
if (entry_dn_linearized == NULL) {
/* Log an error here to indicate that linearization failed */
return LDB_ERR_INVALID_DN; /* Or another appropriate error code */
}src/ldb_modules/memberof.c (363)
The FIXME comment "check if this is right, might have to compare parents" remains unaddressed. Given the change from ldb_dn_compare to sss_linearized_dn_match, it's crucial to clarify if this FIXME is still relevant. If sss_linearized_dn_match sufficiently handles the comparison, the FIXME should be removed. If not, this change might introduce a subtle correctness issue regarding parent comparisons that needs to be resolved.
24ec863 to
b71221b
Compare
alexey-tikhonov
added
coverity
|
Note: Covscan is green. |
Justification is the same as in 704c31d In certain scenarios this function is a hot path and using heavy `ldb_dn_compare()` adds unnecessary overhead. Reviewed-by: Alejandro López <[email protected]> Reviewed-by: Justin Stephenson <[email protected]>
b71221b to
2a6192b
Compare
4 participants
Justification is the same as in 704c31d
In certain scenarios this function is a hot path and using heavy
ldb_dn_compare()adds unnecessary overhead.Testing with a following setup: 50 mid-level groups that all share the same 2500 leaf groups as members, plus one top-level group containing all mid-level groups. Each leaf group contains a single user.
ignore_group_members = false.time id userwith a cold cache:mbof_append_addop()accounts for ~35%mbof_append_addop()accounts for ~18%i.e. this patch makes
mbof_append_addop()approx x2 faster (this gain doesn't depend on number of groups; but weight ofmbof_append_addop()in total consumption depends on it)