[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