On Fri, 2012-03-23 at 08:23 -0400, Stephen Gallagher wrote:
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
Pushed to master.