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