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.
bye,
Sumit