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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
