[SSSD] [PATCHES] Support the 'services' NSS map in SSSD

Jan Zeleny jzeleny at redhat.com
Fri Jan 27 13:59:56 UTC 2012


Jakub Hrozek <jhrozek at redhat.com> wrote:
> Resending after a list outage.
> 
> On Fri, Jan 27, 2012 at 11:25:26AM +0100, Jakub Hrozek wrote:
> > On Thu, Jan 26, 2012 at 08:18:06PM -0500, Stephen Gallagher wrote:
> > > On Fri, 2012-01-27 at 01:12 +0100, Jakub Hrozek wrote:
> > > ...
> > > 
> > > > > #0008:
> > > > > line 228: Just a nitpick I guess, but storing \0 instead of NULL to
> > > > > char * field seems confusing
> > > > > - in relation to sysdb_store_service(): what is the purpose of
> > > > > supporting multiple protocols when only is always given to it? Is
> > > > > there some plan for the future?
> > > > > - I would like to point out the goto construct in the enum_services
> > > > > function. If anyone else wants to ack that, feel free, but I'm not
> > > > > going to.
> > > > > 
> > > > > The last thing is just a question: if you call getservbyname_r(),
> > > > > your variable 'result' is filled with some values. Are all of
> > > > > these values part of the buffer you are giving to that function?
> > > > > My concern is if the s_aliases array is freed somewhere (for
> > > > > example with the buffer being freed).
> > > > 
> > > > Seeing the BUFLEN being set to 1024 made me wonder if the maximum
> > > > service name and/or the maximum number of aliases is defined
> > > > somewhere?
> > > 
> > > No, it's not specified anywhere that we can rely on, unfortunately.
> > > 1024 is as reasonable a guess as any other.
> > > 
> > > ..
> > > 
> > > > > #0012:
> > > > > jhrozek is doing the review
> > > > 
> > > > There's one thing I don't understand -- why does setservent_send() go
> > > > directly to DP without ever checking the cache? I know there's the in
> > > > memory getent_ctx cache but going to sysdb would persist across sssd
> > > > restarts.
> > > > 
> > > > This is different from how setpwent works, for example.
> > > > 
> > > > The rest of the patch is looking good.
> > > 
> > > Actually, I modeled this off of setpwent. I agree it might be better to
> > > check if the enum timeout has expired (and I'm not exactly sure why we
> > > haven't been doing so). If you look at nss_cmd_setgrent_step(), we
> > > always set NEED_CHECK_PROVIDER and go to the DP before we call
> > > sysdb_enumpwent(). We're not checking it for expiration at all.
> > > 
> > > That said, the benefit would only really be there for the proxy
> > > provider, since the LDAP provider just always returns DP_ERR_OK
> > > immediately on set*ent (because it maintains its own timed update
> > > policy instead of request-driven). So In the end, the most we'd gain
> > > is swapping one LDB search for a trip over the socket.
> > 
> > ..actually I completely forgot about this.
> > 
> > > In essence, I think it's probably okay to not survive a reboot on
> > > enumeration here. (We can open a ticket to change this behaviour to be
> > > "more correct" later).
> > 
> > Nah, not worth it.
> > 
> > > > > #0013:
> > > > > jhrozek is doing the review
> > > > 
> > > > I realize we're doing it in proxy_id.c for example, but I really
> > > > don't like using goto to jump backwards. We don't need to fix it
> > > > now, but I think using a loop would be so much nicer.
> > > 
> > > I agree. As I said in my other reply to Jan, I just copy-pasted from
> > > proxy_id.c to save time. We should absolutely open a ticket to fix this
> > > globally.
> > 
> > https://fedorahosted.org/sssd/ticket/1156
> > 
> > > > My only other complaint is that the sysdb_transaction_cancel() call
> > > > should at least print a DEBUG() message on failure.
> > > 
> > > Fixed. I also noticed and fixed a bug with the in_transaction logic
> > > where I was setting it to false regardless of the return value from the
> > > sysdb_transaction_commit().
> > 
> > May a suggest a wrapper macro? :-)
> > 
> > > > I had already reviewed the rest of the patches I haven't covered in
> > > > this review.
> > > 
> > > Patches re-attached. To eliminate confusion, these contain the changes
> > > from Jan's review as well.
> > 
> > I'm fine with the patches now. Ack from me.

Ack from me as well. Great work Steve!

Jan



More information about the sssd-devel mailing list