389-ds-base

#50132 Ticket 49658 - In replicated topology a single-valued attribute can diverge
Closed by spichugi. Opened by tbordaz.
tbordaz/389-ds-base ticket_49658  into  master

Download 50132.patch

Bug Description:
When deleting a specific value of a single valued attribute,
the deleted value can be erronously resurrected.

Fix Description:
This second fix is a rewrite of entry state resolution.
The original function (resolve_attribute_state_single_valued) implemented
a main algorythm but it was heavily merged with resolution of specific cases.
It was too difficult to make the function understandable and preserving
the handling of the specific cases.
The risk of that rewrite fix is that I can not guarantee it fully covers
the set of specific cases

https://pagure.io/389-ds-base/issue/49658

Reviewed by: ?

Platforms tested: F27

Flag Day: no

Doc impact: no

int32_t?

These shhouldn't be const because you immediately change them ;) if they need to be const, for the functions you call, then you probably should
cast as const at the function call

I think the rewrite looks very nice, a few C related changes though, but great :)

rebased onto 88ad2e29324f379c015fe75250a0e2eff1df53d2

@firstyear Thank you sooo much for having look at this long patch.
I fixed your remarks. in the previous rebase

Is CSN_String actually 100 bytes long? Or is this just a guess at how long it could be? Is there a risk this could overrun?

int32_t :)

Thanks mate for the update. Two more little things, but otherwise it looks good :)

rebased onto 47b83246f12d7bcc9bf136d7f14e29bfe99c231c

Thanks, I change the patch (csn_string useless, int->int32_t, remove a debug message)

I realize the patch is somehow complex and it is almost impossible to be sure it covers the various cases handled by the former code. Cleaning this part of code was intentional and it would be great to have it upstream. If it introduces regression, at least we will have a clearer algo to fix ;). Any volunteer ?

If we have a doubt, we could put in a PR_ASSERT to try and catch any possible concerns during testing and devel if we think it could be an issue?

I don't expect you to change this, but this could be a size_t for futureproofing and correctness.

Two very minor comments, but otherwise I think this code looks good :) So ack from me if you want, or you can apply the comments and I'll check again. :)

I think my concern was invalid.

Deleting the last value of an attribute (CSN_A) moves the attribute to the set of deleted attribute but not that the attribute was deleted at CSN_A.
If on an other master, a value is added with a CSN_B < CSN_A this value will move back the attribute to the present list. That is the expected behavior.
But if the attribute deletion was set on CSN_A, then the CSN_B update will be discard.

Note this behavior is the same on single/multi valued attribute

I think 'i' can not be a size_t , it is used for attr_first_deleted_value/attr_next_deleted_value that returns -1 when there is no more value.

rebased onto e09725e772d77723645acdca8d8d60ab15d021be

Pull-Request has been merged by tbordaz

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3191

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.