[SSSD] [PATCH] Add idmap library

Stephen Gallagher sgallagh at redhat.com
Fri Mar 23 12:23:29 UTC 2012


On Thu, 2012-03-22 at 12:21 +0100, Sumit Bose wrote:
> On Thu, Mar 22, 2012 at 11:39:31AM +0100, Sumit Bose wrote:
> > On Wed, Mar 21, 2012 at 02:49:05PM -0400, Stephen Gallagher wrote:
> > > 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
> > 
> > done, moved to src/lib/idmap/
> > 
> > > somewhere? I'd like for us to drop the ipa_ suffix throughout as well,
> > > or replace it with an sss_ suffix.
> > 
> > I would prefer to use the sss_ siffix.
> > 
> > > 
> > > 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'?
> > 
> > done, it is now called idmap_domain_info
> > 
> > Thank you for the review, new version attached.
> > 
> 
> Sorry, forgot to rename the pc file content, new version attached.


Ack
-------------- 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/20120323/0b2e1b54/attachment.sig>


More information about the sssd-devel mailing list