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.