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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel