[SSSD] [PATCHES] Support the 'services' NSS map in SSSD
Stephen Gallagher
sgallagh at redhat.com
Wed Jan 25 18:15:48 UTC 2012
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120125/5382d0fc/attachment.sig>
More information about the sssd-devel
mailing list