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

Stephen Gallagher sgallagh at redhat.com
Thu Jan 26 18:54:32 UTC 2012


On Thu, 2012-01-26 at 18:39 +0100, Jan Zelený wrote:
> #0001:
> Still Ack
> 
> #0002:
> Ack
> 
> #0003:
> I'm not sure why you moved the declarations of sysdb_svc_add() and 
> sysdb_svc_remove_alias() into this patch, but I guess it's just a cosmetic 
> issue, so I have no problem with that.
> 

Incorrect squashing of changes, but it's not harmful.

> Ack
> 
> #0004:
> I've got another segfault for you ;-) You fixed the original but you somehow 
> managed to do the exactly same mistake on the line 117 of the patch (I guess 
> the "c" should have been uint32_t).
> 

Yeah, looks like I converted the macro but forgot to change the
variable.

> Also please check line 314 of the patch - I believe the size of the padding 
> should be 4, not 6.

Yeah, that was wrong. (Though ultimately safe, as the extra two bytes
would have been overwritten anyway).

> 
> #0005:
> Ack
> 
> #0006:
> Ack
> 
> #0007:
> Ack
> 
> #0008:
> line 228: Just a nitpick I guess, but storing \0 instead of NULL to char * 
> field seems confusing

You're right, that's just wrong. I also changed it so that if
num_aliases == 0, we just set cased_aliases to NULL (this is safely
handled in the sysdb routines).

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

Only one is given in the proxy provider (which is a known limitation of
it if you see my original email). However, when saving LDAP entries,
multiple protocols can be available (such as "tcp" and "udp").

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

I hate that too, but I copied it as-is from the other proxy routines
because I was lazy and didn't want to change it. Can we get an ack on it
for these patches and a bug to fix it everywhere later?

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

No, the buffer is empty and statically allocated. getservbyname_r()
populates the buffer with its own internal representation statically and
then constructs the result object with pointers into that buffer. So
it's all static and only lives until the end of the current function
(which is fine, as it's immediately either saved or deleted from the
cache).

> #0009:
> line 7: s/requst/request/
> otherwise Ack (I only briefly read through this, but it's been acked earlier)
> 

Fixed.

I don't actually see an ack for this one anywhere on the list (and given
that it's probably the most contentious one, I'd really like to see a
thorough review).

> #0010:
> Ack
> 
> 
> #0011:
> Ack
> 
> #0012:
> jhrozek is doing the review
> 
> #0013:
> jhrozek is doing the review
> 
> 
> That's all I've got for now.
> Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: servicesreview.tar.bz2
Type: application/x-bzip-compressed-tar
Size: 30227 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120126/701e0324/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/701e0324/attachment.sig>


More information about the sssd-devel mailing list