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

Stephen Gallagher sgallagh at redhat.com
Fri Jan 27 01:18:06 UTC 2012


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.

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).

> > 
> > #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.

> 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().

> 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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: servicesreview.tar.bz2
Type: application/x-bzip-compressed-tar
Size: 30274 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120126/038d28d3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120126/038d28d3/attachment.sig>


More information about the sssd-devel mailing list