#47981 COS cache doesn't properly mark vattr cache as invalid when there are multiple suffixes
Closed: Fixed None Opened 4 years ago by mreynolds.

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.

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!

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

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

2 years ago

Login to comment on this ticket.

Metadata