Re: (ITS#5540) Normalization assertion in attr.c

[hyc@symas.com]

Sean Burford wrote:
> Hi,
>
> On Fri, May 30, 2008 at 2:40 AM,<hyc@symas.com>  wrote:
>> I wrote the attached patch for the original issue. Unfortunately it asserted
>> right away:
> ...
>> It was adding the createTimestamp attribute, without providing normalized
>> values. slap_add_opattrs was written before the generalizedTimeNormalize
>> function was written... I suspect there will be a fair number of cases that
>> need to be cleaned up. I don't have time at the moment to chase them all down.
>> Anyone else want to jump in here? If not, we may have to push this back a bit.
>
> I've modified my copy of the source to normalize data where needed.
> slapd starts up and my test can run.  With the old assertion in place
> it fails exactly as it did before, because the old attr_merge()
> assertion runs before your new attr_valadd() normalization check.

That aspect of the patch is not optional. The old check is in the wrong place 
and must be removed. It misses the other places where attrs are being added, 
while attr_valadd catches all of them.

> Removing the old attr_merge() assertion results in your new
> attr_valadd() check being triggered.  Removing the old assertion looks
> safe, since attr_merge() calls attr_valadd() almost immediately after
> the assertion anyway.
>
> With these changes, running my test script from the original tarball results in:
> bdb_modify_internal: add testAttribute
> attr_valadd: database inconsistent with schema definition of
> testAttribute, reload the DB
> bdb_modify_internal: 80 modify/add: testAttribute: merge error (80)
> hdb_modify: modify failed (80)
>
> I'm working through 'make test' now to find more instances that need
> fixing.  After that I'll send some patches.
>
>> Note that this patch will probably require a fair number of databases to be
>> reloaded.
>
> I'm not even sure that attributes can be automatically renormalized
> when a problem is detected, since bugs (eg. with the generation of
> timestamps, CSNs or referalls) can also trigger the normalization
> checks.

I think it's important to continue to assert as in my patch, at least at this 
stage, so we can catch all of those bugs. I also don't think we can silently 
renormalize just the cases that the "reload the DB" message detects, because 
that's obviously only going to happen on entries that are explicitly modified, 
and the rest of the DB will still have problems.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/