On Wed, 2012-04-25 at 15:41 +0200, Jan Zelený wrote: > Ok, here is the updated patch set.
117: Ack 118: Nack In sdap_fill_memberships(), please do not ever store HASH_* value in 'ret'. Create a new variable (I suggest 'hret'). 'ret' is assumed to be of type errno_t in pretty much all of our code and I don't want to conflate the two. It's probably academic, but ghostel->values[k].length should probably be strlen()+1, since the buffer technically includes the NULL-terminator. In practice, it will likely never matter. I don't understand why you look up SYSDB_MEMBER a second time if ghostel->num_values == 0. Why would it have changed? In sdap_save_group() you assign the ghost name to el->values[el->num_values].data but you don't set its length. 119: Ack 120: Nack I know it's not strictly the fault of this patch, but could you please fix the ldb_transaction logic in this function. Right now, it won't cancel if the commit fails. Please allocate 'msg' on tmp_ctx. Right now it will not be freed if any of the ldb_msg_add_* functions fail. Please rename 'el' to 'alias_el' so it's clear what it's being used for in the cleanout code. A few more comments there would be handy too, for future maintainers. 121: Looks good, but I'd much prefer if you would add some comments. The memberOf plugin is very complex code. 122: Nack I disagree with your assertion in the comment that this is rare. In many (maybe most) existing environments, there will be fake users in the DB. Please do the search within the transaction, not before it. Please rename 'memberof' to 'memberof_el' so it's clear it contains all of them. We know that 'name' is a single-valued attribute. Just use ldb_msg_find_attr_as_string() and make 'name' a 'const char *'. We should also ensure that it's non-NULL. Use ldb_dn_validate() on the msg->dn, just in case the memberOf reference is broken. In that case, we should just log a message, ignore it and hope for the best. Please prefer talloc_zfree() for variables that get reused (such as in a loop). Specifically, please talloc_zfree(msg) at the end of the deletion loop. 123: Nack I don't see the point of this patch (and I think it's guaranteeing duplicates). Unless I completely misread Patch 121, you've already got the memberOf plugin creating memberUID attributes for ghost users. So with this patch, we're adding all memberUID values as group members, and then going and adding the ghost users a second time. That doesn't make any sense. Please tell me if I've misinterpreted something here. 124: Ack 125: Ack 126: Ack
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
