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