[PATCH] replmd changes to handle single-valued link conflicts

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] replmd changes to handle single-valued link conflicts

Samba - samba-technical mailing list
Hi,

The attached patch-set fixes the following bugs:
- 13055: Samba replication cannot resolve single-valued link conflicts
- 13059: Link attributes on Samba have different initial RMD_VERSION
compared to Windows

The bulk of the changes are to handle conflicts correctly for
single-valued link attributes. It required a bit of refactoring in the
replmd code to support this.

Patches can also be viewed here:
http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/tim-replmd-latest

Please let me know if you have any review comments.

Thanks,
Tim


replmd-patchset.txt (113K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] replmd changes to handle single-valued link conflicts

Samba - samba-technical mailing list
Hi Tim,

- For the function replmd_check_singleval_la_conflict, can you break
down the comment a bit? It's a large block of text that I'd probably
just skip over.

- In the commit message for replmd: Fix RMD_VERSION initial value to
match Windows, can you cite the sub-heading and section numbers for the
MS-DRSR spec? It's just one more clue that would be useful.

- In replmd: Make replmd_set_la_val() closer to replmd_build_la_val(),
the assignment of old_dsdb_dn to NULL should have been with the original
code. Perhaps this patch should occur at the beginning.

- Bug numbers need to be on the later patches


So if an incoming link is newer, it will always clobber any existing
links regardless of either being active or inactive?

The overall patches seem pretty reasonable, I think some changes could
have been squashed in a bit more, but I don't think it matters.

I noticed you mentioned in
https://bugzilla.samba.org/show_bug.cgi?id=13039 that it looks like the
object GUID is being used rather than the invocation ID, did you check
if that was the case here? Did we just never bother to implement the
actual object conflict algorithm at all?


Cheers,

Garming


On 28/09/17 17:01, Tim Beale via samba-technical wrote:

> Hi,
>
> The attached patch-set fixes the following bugs:
> - 13055: Samba replication cannot resolve single-valued link conflicts
> - 13059: Link attributes on Samba have different initial RMD_VERSION
> compared to Windows
>
> The bulk of the changes are to handle conflicts correctly for
> single-valued link attributes. It required a bit of refactoring in the
> replmd code to support this.
>
> Patches can also be viewed here:
> http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/tim-replmd-latest
>
> Please let me know if you have any review comments.
>
> Thanks,
> Tim
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] replmd changes to handle single-valued link conflicts

Samba - samba-technical mailing list
Hi,

Thanks for the review Garming. Replies inline below.

Cheers,
Tim

On 29/09/17 12:38, Garming Sam via samba-technical wrote:

> Hi Tim,
>
> - For the function replmd_check_singleval_la_conflict, can you break
> down the comment a bit? It's a large block of text that I'd probably
> just skip over.
>
> - In the commit message for replmd: Fix RMD_VERSION initial value to
> match Windows, can you cite the sub-heading and section numbers for the
> MS-DRSR spec? It's just one more clue that would be useful.
>
> - In replmd: Make replmd_set_la_val() closer to replmd_build_la_val(),
> the assignment of old_dsdb_dn to NULL should have been with the original
> code. Perhaps this patch should occur at the beginning.
>
> - Bug numbers need to be on the later patches
Attached is an updated patch-set with those changes.

Note the last 3 patches are just tidy-ups (i.e. not required for bug
pack-porting), so I left the BUG number off these.

> So if an incoming link is newer, it will always clobber any existing
> links regardless of either being active or inactive?

The existing link will only be clobbered if the received link is active,
i.e.

if (active) {
        ret = replmd_check_singleval_la_conflict(module, replmd_private,

If the received link is newer *and* inactive, then it just gets applied
as per normal. So we end up with the existing active link and the newer
inactive link. This seemed to be the Windows behaviour.

> The overall patches seem pretty reasonable, I think some changes could
> have been squashed in a bit more, but I don't think it matters.

Happy to squash patches further, I've left as is for now, until a second
reviewer looks at them.

> I noticed you mentioned in
> https://bugzilla.samba.org/show_bug.cgi?id=13039 that it looks like the
> object GUID is being used rather than the invocation ID, did you check
> if that was the case here? Did we just never bother to implement the
> actual object conflict algorithm at all?

I'm not sure. The current Windows spec for ResolveNameConflict() looks
significantly different to the logic that Samba currently uses. I'm not
sure if the Windows algorithm was revised after the Samba code was
implemented.

The spec suggests that the object GUID is used, rather than the
invocation ID. I haven't confirmed this. I just confirmed that the
RMD_VERSION is not used for resolving object conflicts.

>
> Cheers,
>
> Garming
>
>
> On 28/09/17 17:01, Tim Beale via samba-technical wrote:
>> Hi,
>>
>> The attached patch-set fixes the following bugs:
>> - 13055: Samba replication cannot resolve single-valued link conflicts
>> - 13059: Link attributes on Samba have different initial RMD_VERSION
>> compared to Windows
>>
>> The bulk of the changes are to handle conflicts correctly for
>> single-valued link attributes. It required a bit of refactoring in the
>> replmd code to support this.
>>
>> Patches can also be viewed here:
>> http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/tim-replmd-latest
>>
>>
>> Please let me know if you have any review comments.
>>
>> Thanks,
>> Tim
>>
>
>

replmd-patchset.txt (115K) Download Attachment