If there are multiple suffixes, and one does not have any COS definitions, it will not invalidate the vattr cache after rebuilding the COS cache, and already cached entries will not reflect the new COS template/definition changes.
The problem arises when there are multiple suffixes. So when there is a COS change that rebuilds the COS cache, we check all the suffixes to see to see if there are COS definitions. If there are definitions then the virtual attribute cache is supposed to be invalidated. The problem is that if the last suffix checked does not contain any COS entries, then the virtual attribute cache is incorrectly NOT invalidated. So already cached entries will still contian the old COS attributes/values.
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1179370
Hi Mark,
Since your CI test has a good coverage, it may not exist such a case, but could it be possible if cos_cache_add_dn_defs returns 0 with vc == 0? In another word, the return value of cos_cache_add_dn_defs is always synced with the value of vc? My question is since the whole code 672 - 683 is in the do - while loop, if there is a case the return value == 0 && vc == 0, (if non-zero is already set) vattr_cacheable might be overwritten... We may want to do something like "if (vc) {vattr_cacheable = vc;}" at the line 678?
Also, we may want to set 0 to *vattr_cacheable at the beginning of cos_cache_build_definition_list (since we may have no chance to set any value to the value)?
{{{ 671 672 { 672 673 / here's a suffix, lets search it... / 673 674 if(suffixVals[valIndex]->bv_val) 675 { 676 if(!cos_cache_add_dn_defs(suffixVals[valIndex]->bv_val ,pDefs, &vc)) 677 { 678 *vattr_cacheable = vc; 675 679 cos_def_available = 1; 680 } 681 } 677 682 valIndex++; 678 683 } }}}
Replying to [comment:4 nhosoi]:
Hi Mark, Since your CI test has a good coverage, it may not exist such a case, but could it be possible if cos_cache_add_dn_defs returns 0 with vc == 0? In another word, the return value of cos_cache_add_dn_defs is always synced with the value of vc?
Since your CI test has a good coverage, it may not exist such a case, but could it be possible if cos_cache_add_dn_defs returns 0 with vc == 0? In another word, the return value of cos_cache_add_dn_defs is always synced with the value of vc?
It is not synced, and we do need to do it this way to cover all the scenarios. We only want to set vattr_cacheable(regardless if it's -1 or 0), if the function returns success.
My question is since the whole code 672 - 683 is in the do - while loop, if there is a case the return >value == 0 && vc == 0, (if non-zero is already set) vattr_cacheable might be overwritten... We may >want to do something like "if (vc) {vattr_cacheable = vc;}" at the line 678? Also, we may want to set 0 to *vattr_cacheable at the beginning of cos_cache_build_definition_list (since we may have no chance to set any value to the value)?
My question is since the whole code 672 - 683 is in the do - while loop, if there is a case the return >value == 0 && vc == 0, (if non-zero is already set) vattr_cacheable might be overwritten... We may >want to do something like "if (vc) {vattr_cacheable = vc;}" at the line 678?
It is set to "0" by default in cos_cache_create(), so this is covered.
Replying to [comment:5 mreynolds]:
Replying to [comment:4 nhosoi]: Hi Mark, Since your CI test has a good coverage, it may not exist such a case, but could it be possible if cos_cache_add_dn_defs returns 0 with vc == 0? In another word, the return value of cos_cache_add_dn_defs is always synced with the value of vc? It is not synced, and we do need to do it this way to cover all the scenarios. We only want to set vattr_cacheable(regardless if it's -1 or 0), if the function returns success.
Does cos_cache_add_dn_defs(suffixVals[valIndex]->bv_val ,pDefs, &vc) return success just once only when pDefs dn belongs to suffixVals[valIndex]->bv_val? If yes, your patch looks good. (Then, could you "break" the do - while loop then?) But it looks to me pDefs can be a list and cos_cache_add_dn_defs returns success multiple times with various vc values?
My question is since the whole code 672 - 683 is in the do - while loop, if there is a case the return >value == 0 && vc == 0, (if non-zero is already set) vattr_cacheable might be overwritten... We may >want to do something like "if (vc) {vattr_cacheable = vc;}" at the line 678? Also, we may want to set 0 to *vattr_cacheable at the beginning of cos_cache_build_definition_list (since we may have no chance to set any value to the value)? It is set to "0" by default in cos_cache_create(), so this is covered.
Good. Thanks!
Replying to [comment:6 nhosoi]:
Replying to [comment:5 mreynolds]: Replying to [comment:4 nhosoi]: Hi Mark, Since your CI test has a good coverage, it may not exist such a case, but could it be possible if cos_cache_add_dn_defs returns 0 with vc == 0? In another word, the return value of cos_cache_add_dn_defs is always synced with the value of vc? It is not synced, and we do need to do it this way to cover all the scenarios. We only want to set vattr_cacheable(regardless if it's -1 or 0), if the function returns success. Does cos_cache_add_dn_defs(suffixVals[valIndex]->bv_val ,pDefs, &vc) return success just once only when pDefs dn belongs to suffixVals[valIndex]->bv_val? If yes, your patch looks good. (Then, could you "break" the do - while loop then?) But it looks to me pDefs can be a list and cos_cache_add_dn_defs returns success multiple times with various vc values?
Actually I was typing up an amendment to my previous update :-) The return value of success is in sync with vatter_cacheable. So yes, breaking out of the suffix loop after the first success, will be sufficient to the fix the bug. We can also remove the int pointer to vattr_cacheable as well. I'll rework the patch.
My question is since the whole code 672 - 683 is in the do - while loop, if there is a case the return >value == 0 && vc == 0, (if non-zero is already set) vattr_cacheable might be overwritten... We may >want to do something like "if (vc) {vattr_cacheable = vc;}" at the line 678? Also, we may want to set 0 to *vattr_cacheable at the beginning of cos_cache_build_definition_list (since we may have no chance to set any value to the value)? It is set to "0" by default in cos_cache_create(), so this is covered. Good. Thanks!
revision 0001-Ticket-47981-COS-cache-doesn-t-properly-mark-vattr-c.patch
New patch attached...
Thanks for coming with the cleaner algorithm, Mark!
ebf24ff..42e2df3 master -> master commit 42e2df3 Author: Mark Reynolds mreynolds@redhat.com Date: Wed Jan 7 08:59:06 2015 -0500
18eb6ce..6229ad9 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 6229ad9
3494fec..df7bafa 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit df7bafa
864b677..c7c0e75 389-ds-base-1.3.1 -> 389-ds-base-1.3.1 commit c7c0e753d03349efc7f36654529a3be907d3187c
c49b030..c1ba7eb 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit c1ba7eb
git patch file (master) -- fixing a regression 0001-Ticket-47981-COS-cache-doesn-t-properly-mark-vattr-c.2.patch
Ack, assuming the test script still passes.
Replying to [comment:15 mreynolds]:
Yes, it does. Thanks, Mark!
Reviewed by Mark (Thank you!!)
Pushed to master: 18ae65b..6557b82 master -> master commit 6557b82
Pushed to 389-ds-base-1.3.4: 2783876..c1721f1 389-ds-base-1.3.4 -> 389-ds-base-1.3.4 commit c1721f1
Pushed to 389-ds-base-1.3.3: e7d3b2f..546aa6b 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 546aa6b
Pushed to 389-ds-base-1.2.11: 6a1f2f0..d2b69d5 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit d2b69d5
Metadata Update from @nhosoi: - Issue assigned to mreynolds - Issue set to the milestone: 1.2.11.33
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1312
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Log in to comment on this ticket.