[SSSD] [PATCH] Add idmap library

Stephen Gallagher sgallagh at redhat.com
Wed Mar 21 18:49:05 UTC 2012


On Mon, 2012-03-19 at 17:18 +0100, Sumit Bose wrote:
> >> Hi,
> >> 
> >> this patch adds a library to map a Windows SID to a Unix uid or gid. My
> >> current plan is to used it for AD trusts on the client and the server
> >> side, this is why the interface allows different kind of memory
> >> allocators.
> >> 
> >> bye,
> >> Sumit
> 
> Jan's mail never made it into my mailbox, so I reply to my original
> mail.
> 
> >
> >Just a couple small things:
> >
> >ipa_idmap.c
> >line 98: the memset is redundant
> 
> Since the memory allocator can be supplied by the user I would like to
> make sure that the memory is initialized to 0 under all circumstances.
> So it might be redundant in some cases, but important in other cases.
> 
> >line 251: This is nitpicking on my side, but I would move this check
> >after the 
> >next two trivial - in case one of them fails, this one won't have to be 
> >executed
> 
> done
> 
> >line 321: I'm not completely sure about the design here. Is it possible
> >to 
> >have two domains with the same SID and different ranges? If yes, then
> >there 
> >should be continue on this line instead of return
> 
> no, this is not possible the SID is always assumed to be unique.
> 
> >
> >These warnings showed up when I tried to make docs:
> >/root/sssd/src/providers/ipa/ipa_idmap.h:185: warning: argument 'str' of 
> >command @param is not found in the argument list of is_domain_sid(const
> >char 
> >*sid)
> >/root/sssd/src/providers/ipa/ipa_idmap.h:185: warning: The following 
> >parameters of is_domain_sid(const char *sid) are not documented:
> >  parameter 'sid'
> 
> fixed
> 
> >
> >
> >Other than this, the patch is fine. I wasn't able to perform more
> >testing (like 
> >make check) since I just managed to delete my entire git repo and that
> >means 
> >it's time to go home ;-)
> >
> 
> Thank you for the review, new version attached.

Nack.

Can we move this out of src/providers/ipa and into common code
somewhere? I'd like for us to drop the ipa_ suffix throughout as well,
or replace it with an sss_ suffix.

I want to use this code for the AD provider that I'm working on in 1.9
as well (which is going to be similar to the IPA provider in that it
will wrap certain functions of the LDAP and KRB5 providers).

Also, I'd prefer if we came up with a different name for 'struct
domain_info' as it's too similar to 'struct sss_domain_info' which we
use everywhere in the code. Perhaps 'struct ad_domain_info'?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120321/0d59f5d5/attachment.sig>


More information about the sssd-devel mailing list