On Wed, Jul 23, 2014 at 12:34:32PM +0200, Sumit Bose wrote: > new version attached. > > bye, > Sumit
I admit I haven't reviewed the filed copied from Winbind too closely, I just quickly read them to know what's going on in these modules. In the review, I mostly compared the sssd implementation to the winbind one. Do you expect the files copied from Samba to change? Most of the files I check in the Samba tree didn't change since 2012 or even 2010 so I expect we're safe.. I think the dlopen to open the nss_sss module is perfectly fine, in fact I prefer it over linking. That's how the module is used by the glibc after all. A question -- in sssd's copy_group_entry() you only create the gr_mem array when there are some members. Winbind seems to always create an array which contains just a NULL sentinel if the group has no members. Does that matter? If not, let's leave the code as-is, I just wanted to check. In nss_to_wbc(), would it make sense to also translate NSS_STATUS_UNAVAIL to WBC_ERR_WINBIND_NOT_AVAILABLE ? Sure, there is no winbind, so the return code might better be called something like WBC_ERR_DATA_SOURCE_NOT_AVAILABLE or similar, but my question is more if handling the NSS_STATUS_UNAVAIL NSS code is expected. wbcGetgrnam and wbcGetgrnam have wrong comments, but this trivial bug is also in Samba. Are you sure the asprintf() call in wbcLookupName is safe? Could this enable someone to trash the stack with a long enough name? I agree with the comment in wbcLookupSid() about returning the name and domain separately. Do you want to file a ticket? Alternatively, we can make a function that parses a FQDN into a (name,domain) tuple public if you expect this functionality to be needed elsewhere? sss_client/libwbclient/wbc_sssd_internal.h seems to use a DEVELOPER macro, do you expect this macro to be enabled with modifying CFLAGS? It's OK to leave it there I guess, I'm not aware of any similar macro in SSSD.. In wbclient.pc.in can you mention this is wbclient API using SSSD? That's all I have. The code looks very solid and clean overall -- thanks! _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
