On Mon, 2012-11-19 at 23:31 +0100, Jakub Hrozek wrote: > On Mon, Nov 19, 2012 at 05:51:53PM +0100, Pavel Březina wrote: > > On 11/18/2012 11:58 PM, Jakub Hrozek wrote: > > >[PATCH 1/2] SYSDB: Use the add_string convenience functions for managing > > >ghost user attribute > > > > > >Using the convenience function instead of low-level ldb calls makes the > > >code more compact and more readable. > > > > > >[PATCH 2/2] LDAP: Only convert direct parents' ghost attribute to member > > > > > >https://fedorahosted.org/sssd/ticket/1612 > > > > > >I'm not too happy with the originalDN attribute exposed in the > > >sysdb_add_user/store_user functions but it seemed like a better way > > >than grabbing the attribute from the extended attributes in the "attrs" > > >array. At the same time, we need to adjust the attributes during all user > > >adds, not just during sdap_store_user from the LDAP provider. Ideas on > > >how to make the process cleaner are welcome. > > > > > >This patch changes the handling of ghost attributes when saving the > > >actual user entry. Instead of always linking all groups that contained > > >the ghost attribute with the new user entry, the original member > > >attributes are now saved in the group object and the user entry is only > > >linked with its direct parents. > > > > > >As the member attribute is compared against the originalDN of the user, > > >if either the originalDN or the originalMember attributes are missing, > > >the user object is linked with all the groups as a fallback. > > > > > >The original member attributes are only saved if the LDAP schema > > >supports nesting. > > > > >@@ -235,6 +236,19 @@ sdap_process_ghost_members(struct sysdb_attrs *attrs, > > > return ret; > > > } > > > > > > > Hi, > > just one nitpick. > > > > >+ if (store_original_member) { > > >+ DEBUG(SSSDBG_TRACE_FUNC, ("The group has %d members\n", > > >memberel->num_values)); > > >+ for (i = 0; i < memberel->num_values; i++) { > > >+ ret = sysdb_attrs_add_string(sysdb_attrs, SYSDB_ORIG_MEMBER, > > >+ (const char *) > > >memberel->values[i].data); > > >+ if (ret) { > > >+ DEBUG(SSSDBG_OP_FAILURE, ("Could not add member [%s]\n", > > >+ (const char *) memberel->values[i].data)); > > > > wrong indentation^^ > > > > >+ return ret; > > >+ } > > >+ } > > >+ } > > >+ > > > > Otherwise it looks good to me. > > Thank you for the review. Simo requested some additional changes, in > particular: > * use "it" instead of "him" when talking about user > * don't misuse twopass, but use a separate variable > * fix code to adhere to 80-columns rule on several places > > New patches are attached.
I haven't been able to test the patch yet, but at least looking at the code I see no issues, so I would ack. Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
