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