On Thu, Jan 26, 2012 at 06:39:40PM +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.
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).
Also please check line 314 of the patch - I believe the size of the padding
should be 4, not 6.
#0005:
Ack
#0006:
Ack
#0007:
Ack
Ack++
#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?
#0009:
line 7: s/requst/request/
otherwise Ack (I only briefly read through this, but it's been acked earlier)
Can't review, this one is mine.
#0010:
Ack
Ack++
#0011:
Ack
Ack++
#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.
#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.
My only other complaint is that the sysdb_transaction_cancel() call
should at least print a DEBUG() message on failure.
I had already reviewed the rest of the patches I haven't covered in this
review.