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