Dropping the [PRELIMINARY] tag. These are now on full review. On Wed, 2012-01-25 at 18:31 +0100, Jan Zelený wrote: > #0001: > Ack. > > #0002: > - make sysdb_svc_add and sysdb_svc_update static -> there is a > sysdb_store_service as an interface for them. Also consider making > sysdb_svc_dn and sysdb_svc_remove_alias making static - I'm not sure about > those.
I removed sysdb_svc_add(), sysdb_svc_update() and sysdb_svc_remove_alias() from the public header (and made sysdb_svc_update() static), but I left the others exportable because I want the tests to be able to exercise them. sysdb_svc_dn I left public because it may prove useful in the LDAP provider (uncertain yet, but it doesn't hurt to have it available). > - if we are to be really defensive (as discussed on IRC), I'd change the > approach if detecting more services with the same name being already stored > in > sysdb - perhaps delete them all and store the new one instead of returning > EINVAL? I changed it so that it would delete the previous update_dn as well as the current entry and reset update_dn to NULL. This way it will just replace them. (If it hits a third, it will just go back to updating again). > - in sysdb_svc_delete: just in case both port and name is given, I'd try to > delete service by name and in case of failure fall back to the port (i.e. not > making them mutually exclusive). > I'm confused. I'm pretty sure that's exactly what it does right now. > #0003: > Ack > > #0004: > - just a reminder that there is that segfault inducing error, other than that > the patch is fine, so conditional Ack > > #0005: > Ack > > #0006: > - I think you should be freeing service_and_protocol variables after they are > used. They are allocated on top of negative cache, so they could potentially > live pretty long and take a lot of space You're right. That would have been a pretty serious memory leak. (Well, technically not a leak, but growth). > - I also think you should be checking the above mentioned variable for NULL > and returning ENOMEM if the operation failed. Otherwise I see potential > problems in utf8 library which would could receive NULL on input. And if > nothing else, you would be returning EINVAL from sss_ncache_set_service_int > instead of ENOMEM ;-) > Good idea. Done. > > That's all I've got right now. I will continue with the review tomorrow. > > Thanks > Jan
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel