On Sun, Aug 17, 2014 at 08:43:25PM +0200, Jakub Hrozek wrote: > 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 so, too. > > 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. I changed it so that an array with a single NULL is returned. > > 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. done > > 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 added some checks to prevent this. > > 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? But this would require parsing sssd.conf as well because in theory the full name format can be changed. So I would prefer to return them separately. > > 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.. With this flag calls to un-implemented calls are written to syslog. I found this option useful during development and since not all calls are implemented it might become useful later as well, this is why I kept it. > > In wbclient.pc.in can you mention this is wbclient API using SSSD? done > > That's all I have. The code looks very solid and clean overall -- > thanks! Thank you for the review, new version attached. bye, Sumit > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
