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

set default ticket origin to Community

0001-Ticket-426-support-posix-schema-for-user-and-group-s.patch
0001-Ticket-426-support-posix-schema-for-user-and-group-s.patch

please verify my comment for Issue #8 on https://github.com/cgrzemba/Posix-Winsync-Plugin-for-389-directory-server.

The problem was the forgotten braces on the if - statement

{{{
if(posix_winsync_config_get_mapMemberUid())

}}}

the next 3 lines must be enclosed in { } .

{{{
{
memberUidLock();
modGroupMembership(ds_entry,smods,do_modify);
memberUidUnlock();
}
}}}

Replying to [comment:4 cgrzemba]:

please verify my comment for Issue #8 on https://github.com/cgrzemba/Posix-Winsync-Plugin-for-389-directory-server.

The problem was the forgotten braces on the if - statement

{{{
if(posix_winsync_config_get_mapMemberUid())

}}}

the next 3 lines must be enclosed in { } .

{{{
{
memberUidLock();
modGroupMembership(ds_entry,smods,do_modify);
memberUidUnlock();
}
}}}

The original code was like this:
{{{
if(posix_winsync_config_get_mapMemberUid())
memberUidLock();
modGroupMembership(ds_entry,smods,do_modify);
memberUidUnlock();
}}}

Which means that if mapmemberuid was true, the only action it would take is to lock the memberuid lock. ''It would always call modGroupMembership - even if mapmemberuid was false''. The only thing wrong with this is that it will always call memberuidunlock(), even if memberuidlock() was not called - this is usually safe but not recommended. So the only thing I did was this:
{{{
if(posix_winsync_config_get_mapMemberUid())
memberUidLock();
modGroupMembership(ds_entry,smods,do_modify);
if(posix_winsync_config_get_mapMemberUid())
memberUidUnlock();
}}}

So the only change I made is to only call memberUidUnlock if memberUidLock was called. My change preserves the old behavior of always calling modGroupMembership.

fixed formatting in previous patch
0001-Ticket-426-support-posix-schema-for-user-and-group-s.2.patch

...
It would always call modGroupMembership - even if mapmemberuid was false.
...

That's wrong: posix_winsync_config_get_mapMemberUid() should control if modGroupMembership(ds_entry,smods,do_modify) is called or not.

Which is not happen because of the missing braces.

fix add/modGroupMembership and locking
0001-Ticket-426-support-posix-schema-for-user-and-group-s.3.patch

Replying to [comment:7 cgrzemba]:

...
It would always call modGroupMembership - even if mapmemberuid was false.
...

That's wrong: posix_winsync_config_get_mapMemberUid() should control if modGroupMembership(ds_entry,smods,do_modify) is called or not.

Which is not happen because of the missing braces.

Ok. I've attached another patch which does this:
{{{
if (posix_winsync_config_get_mapMemberUid()) {
memberUidLock();
addGroupMembership(ds_entry, ad_entry);
memberUidUnlock();
}
}}}
Same with modGroupMembership. Is this correct?

1) ldap/servers/plugins/posix-winsync/posix-winsync.c
+static void
+sync_acct_disable(
[...]
+ if (direction == ACCT_DISABLE_TO_DS) {
+ char attrtype = NULL;
+ char attrval;
+ char val[255];
[...]
+ if (isvirt) {
+ strcpy(val,"cn=nsManagedDisabledRole,");
+ strncat(val,slapi_sdn_get_dn(posix_winsync_config_get_suffix()),sizeof(val)-1);
+ attrval = val;
This would be a minor corner case, but if val is overflown, the error won't be detected and the the account won't be disabled?

I'm just curious. Quite a long list of APIs is passed to slapi_apib_register and registered. Some of them are just noop callbacks (e.g., posix_winsync_dirsync_search_params_cb, posix_winsync_pre_ad_search_cb, posix_winsync_pre_ds_search_entry_cb, posix_winsync_get_new_ds_user_dn_cb, posix_winsync_get_new_ds_group_dn_cb, posix_winsync_can_add_entry_to_ad_cb) May I ask why they are needed?
+static void posix_winsync_api[] = {
+ NULL, / reserved for api broker use, must be zero */
+ posix_winsync_agmt_init,
+ posix_winsync_dirsync_search_params_cb,
+ posix_winsync_pre_ad_search_cb,
+ posix_winsync_pre_ds_search_entry_cb,
+ posix_winsync_pre_ds_search_all_cb,
+ posix_winsync_pre_ad_mod_user_cb,
+ posix_winsync_pre_ad_mod_group_cb,
+ posix_winsync_pre_ds_mod_user_cb,
+ posix_winsync_pre_ds_mod_group_cb,
+ posix_winsync_pre_ds_add_user_cb,
+ posix_winsync_pre_ds_add_group_cb,
+ posix_winsync_get_new_ds_user_dn_cb,
+ posix_winsync_get_new_ds_group_dn_cb,
+ posix_winsync_pre_ad_mod_user_mods_cb,
+ posix_winsync_pre_ad_mod_group_mods_cb,
+ posix_winsync_can_add_entry_to_ad_cb,
+ posix_winsync_begin_update_cb,
+ posix_winsync_end_update_cb,
+ posix_winsync_destroy_agmt_cb
+};

2) ldap/servers/plugins/posix-winsync/posix-winsync-config.c
+static int
+dn_isparent( const Slapi_DN parentdn, const Slapi_DN childdn )
+{
We have an equivalent slapi API:
int slapi_sdn_isparent( const Slapi_DN parent, const Slapi_DN child )

Very minor issues:

/ldap/servers/plugins/posix-winsync/posix-group-func.c

• slapi_private.h is commented out. Should this be removed so not to expose slapi-private?

• overall -for performance we should not be declaring var/struct pointers inside of for/while loops.

/ldap/servers/plugins/posix-winsync/posix-winsync.c

• Declaring vars inside of loops

All of this is pretty picky, and I don't know winsync well enough to know if these loops are even long running loops. So it might not even matter, but it wouldn't hurt.

And indentation/spacing needs work.

Replying to [comment:10 nhosoi]:

1) ldap/servers/plugins/posix-winsync/posix-winsync.c

Will fix in upcoming new patch.

I'm just curious. Quite a long list of APIs is passed to slapi_apib_register and registered. Some of them are just noop callbacks (e.g., posix_winsync_dirsync_search_params_cb, posix_winsync_pre_ad_search_cb, posix_winsync_pre_ds_search_entry_cb, posix_winsync_get_new_ds_user_dn_cb, posix_winsync_get_new_ds_group_dn_cb, posix_winsync_can_add_entry_to_ad_cb) May I ask why they are needed?
+static void posix_winsync_api[] = {
+ NULL, / reserved for api broker use, must be zero */
+ posix_winsync_agmt_init,
+ posix_winsync_dirsync_search_params_cb,
+ posix_winsync_pre_ad_search_cb,
+ posix_winsync_pre_ds_search_entry_cb,
+ posix_winsync_pre_ds_search_all_cb,
+ posix_winsync_pre_ad_mod_user_cb,
+ posix_winsync_pre_ad_mod_group_cb,
+ posix_winsync_pre_ds_mod_user_cb,
+ posix_winsync_pre_ds_mod_group_cb,
+ posix_winsync_pre_ds_add_user_cb,
+ posix_winsync_pre_ds_add_group_cb,
+ posix_winsync_get_new_ds_user_dn_cb,
+ posix_winsync_get_new_ds_group_dn_cb,
+ posix_winsync_pre_ad_mod_user_mods_cb,
+ posix_winsync_pre_ad_mod_group_mods_cb,
+ posix_winsync_can_add_entry_to_ad_cb,
+ posix_winsync_begin_update_cb,
+ posix_winsync_end_update_cb,
+ posix_winsync_destroy_agmt_cb
+};

These are useful to have for debugging/debug logging.

2) ldap/servers/plugins/posix-winsync/posix-winsync-config.c
+static int
+dn_isparent( const Slapi_DN parentdn, const Slapi_DN childdn )
+{
We have an equivalent slapi API:
int slapi_sdn_isparent( const Slapi_DN parent, const Slapi_DN child )

Will fix in upcoming patch.

Replying to [comment:11 mreynolds]:

Very minor issues:

/ldap/servers/plugins/posix-winsync/posix-group-func.c

• slapi_private.h is commented out. Should this be removed so not to expose slapi-private?

Yep. Will remove.

• overall -for performance we should not be declaring var/struct pointers inside of for/while loops.

/ldap/servers/plugins/posix-winsync/posix-winsync.c

• Declaring vars inside of loops

We do this in many, many places in the server. I thought that the compiler optimized for this situation.

All of this is pretty picky, and I don't know winsync well enough to know if these loops are even long running loops. So it might not even matter, but it wouldn't hurt.

And indentation/spacing needs work.

I thought I had fixed that. Were you looking at patch https://fedorahosted.org/389/attachment/ticket/426/0001-Ticket-426-support-posix-schema-for-user-and-group-s.2.patch or later?

Declaring vars inside of loops

winsync performance is very slow to begin with - this is probably the least of our worries :P

fix issues found by nhosoi and mreynolds
0001-Ticket-426-support-posix-schema-for-user-and-group-s.4.patch

9f959f0..4335592 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit changeset:4335592/389-ds-base
Author: Rich Megginson rmeggins@redhat.com
Date: Thu Aug 16 17:34:05 2012 -0600
53c974f..34eea5d master -> master
commit changeset:34eea5d/389-ds-base
Author: Rich Megginson rmeggins@redhat.com
Date: Thu Aug 16 17:34:05 2012 -0600

Replying to [comment:13 rmeggins]:

Replying to [comment:11 mreynolds]:

Very minor issues:

/ldap/servers/plugins/posix-winsync/posix-group-func.c

• slapi_private.h is commented out. Should this be removed so not to expose slapi-private?

Yep. Will remove.

• overall -for performance we should not be declaring var/struct pointers inside of for/while loops.

/ldap/servers/plugins/posix-winsync/posix-winsync.c

• Declaring vars inside of loops

We do this in many, many places in the server. I thought that the compiler optimized for this situation.

I'm not sure about gcc, but this was strictly enforced at my last position. Guess it's just an old habit.

All of this is pretty picky, and I don't know winsync well enough to know if these loops are even long running loops. So it might not even matter, but it wouldn't hurt.

And indentation/spacing needs work.

I thought I had fixed that. Were you looking at patch https://fedorahosted.org/389/attachment/ticket/426/0001-Ticket-426-support-posix-schema-for-user-and-group-s.2.patch or later?
The latest patch looks good.

Added initial screened field value.

add v2 and v3 api - use precedence callback - add required attrs to ldif
0001-Ticket-426-support-posix-schema-for-user-and-group-s.5.patch

322523d..7ecddec 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit changeset:7ecddec/389-ds-base
Author: Rich Megginson rmeggins@redhat.com
Date: Tue Sep 4 08:25:59 2012 -0600
b542257..214f3a8 master -> master
commit changeset:214f3a8/389-ds-base
Author: Rich Megginson rmeggins@redhat.com
Date: Tue Sep 4 08:25:59 2012 -0600

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

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.