this patch removes rawentry usage
0025-posix-winsync.rawentry.patch

I added a patch where prevents the use of rawentry so posix-winsync plugin tolerate the changes of 716980 and 47314.
This contains also a fix for value compare of changed attributes.

Hi cgrzemba,

Thank you for providing your patch. I reviewed it and had a question. In the patch, you replaced rawentry with ad_entry in posix_winsync_pre_ds_mod_user_cb. How about another posix plug-ins using rawentry that is posix_winsync_pre_ds_add_user_cb? Should it also avoid using rawentry or this is ok?

Hello cgrzemba,

I'm trying to test your patch. Could you please provide us the steps to reproduce your problem?

Also, do you happen to have an answer to this question?

Replying to [comment:4 nhosoi]:

Hi cgrzemba,

Thank you for providing your patch. I reviewed it and had a question. In the patch, you replaced rawentry with ad_entry in posix_winsync_pre_ds_mod_user_cb. How about another posix plug-ins using rawentry that is posix_winsync_pre_ds_add_user_cb? Should it also avoid using rawentry or this is ok?

Thanks,
--noriko

Replying to [comment:6 nhosoi]:

Hello cgrzemba,

I'm trying to test your patch. Could you please provide us the steps to reproduce your problem?

Also, do you happen to have an answer to this question?

Replying to [comment:4 nhosoi]:

Hi cgrzemba,

Thank you for providing your patch. I reviewed it and had a question. In the patch, you replaced rawentry with ad_entry in posix_winsync_pre_ds_mod_user_cb. How about another posix plug-ins using rawentry that is posix_winsync_pre_ds_add_user_cb? Should it also avoid using rawentry or this is ok?

Yes, there may be rawentry also not be used and has to replaced with ad_entry.

Thanks,
--noriko

git patch file (master) -- Thanks to Carsten Grzemba for the patch. Made minimum changes such as replacing the direct access to the bv_len in Slapi_Value with slapi_value_get_length.
0001-Ticket-47763-winsync-plugin-modify-is-broken.patch

Why change the value comparison to be length aware? This is a big change in behavior. It means e.g. that "fo" == "foo". Is this necessary?

Replying to [comment:11 rmeggins]:

Why change the value comparison to be length aware? This is a big change in behavior. It means e.g. that "fo" == "foo". Is this necessary?

Carsten has the true answer. I thought the change made to attr_compare_equal is for the performance (instead of comparing all the multi-values, it just compared the first value). If the 2 lengths differ, it returns "NO MATCH" and attribute value replacement is executed next. It looks to me "NO MATCH" could be returned even if the 2 attr values match, E.g., a == {"abcd", "xyz"}, b == {"xyz", "abcd"}. The following modify/replacement is not needed, but no harm. Probably, he took the conservative approach?

Replying to [comment:11 rmeggins]:

Why change the value comparison to be length aware? This is a big change in behavior. It means e.g. that "fo" == "foo". Is this necessary?

Since we don't hear from the author, shall I back off the change on this compare function and retest it?
384 391 attr_compare_equal(Slapi_Attr a, Slapi_Attr b)

Replying to [comment:13 nhosoi]:

Replying to [comment:11 rmeggins]:

Why change the value comparison to be length aware? This is a big change in behavior. It means e.g. that "fo" == "foo". Is this necessary?

Since we don't hear from the author, shall I back off the change on this compare function and retest it?
384 391 attr_compare_equal(Slapi_Attr a, Slapi_Attr b)

ok

git patch file (master; take 2) -- Resurrected original attr_compare_equal
0001-Ticket-47763-winsync-plugin-modify-is-broken.2.patch

I ran more tests and verified this patch works fine for modify+add and replace attributes.

Attachment 0001-Ticket-47763-winsync-plugin-modify-is-broken.2.patch​ added
git patch file (master; take 2) -- Resurrected original attr_compare_equal

modify+delete attributes (e.g., loginShell) on AD is not sync'ed because the Windows Sync plugin does not know about the non pre-registered attributes. But this is not a regression. Rather, it's considered a limitation in WinSync callback framework? (Probably, we could open a new ticket for the issue.)

That's said the patch should be good for review.

Reviewed by Rich and Noriko.

Pushed to master:
788a48f..b6b7199 master -> master
commit b6b7199

Pushed to 389-ds-base-1.3.2:
dfbc46a..7666910 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 7666910

Pushed to 389-ds-base-1.3.1:
f18411d..0dd3192 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit 0dd319278267d6fd23d1af1c3cae81856190a914

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1118060

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

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.