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?

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)?

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 42e2df3858a4e14706d57b5c907d1d3768f4d970
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 6229ad9a3d78cf6d83f09f2893c39dce5bbc71ca

3494fec..df7bafa 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit df7bafae3c069ecf90d951d0c0f1ede26ab521fd

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 c1ba7eb8c41c44bd7689bfc2526a2aa7fff47284

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]:

Ack, assuming the test script still passes.

Yes, it does. Thanks, Mark!

Reviewed by Mark (Thank you!!)

Pushed to master:
18ae65b..6557b82 master -> master
commit 6557b820dca7980067afc2a33184197b2d154a51

Pushed to 389-ds-base-1.3.4:
2783876..c1721f1 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit c1721f1d6e2344eefaec817ed47119c15c43fcfc

Pushed to 389-ds-base-1.3.3:
e7d3b2f..546aa6b 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 546aa6b1f4db1eefc426d3dcb6788f5251fd1e82

Pushed to 389-ds-base-1.2.11:
6a1f2f0..d2b69d5 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit d2b69d51c2d1d7d53be881b0e816b5088295a9c1

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.

Thank you for understanding. We apologize for all inconvenience.