[SSSD] [PATCHES] SUDO Integration - periodical update

Jan Zelený jzeleny at redhat.com
Wed Jan 11 12:47:16 UTC 2012


> https://fedorahosted.org/sssd/ticket/1110
> 
> [PATCH 1/4]
> I've removed FIXME and move the DEBUG message to a better place.

I would have Acked that, but I agree with Jakub that this should be done as a 
part of fix for #1125

> 
> [PATCH 2/4]
> I've wrapped the data provider part with tevent request so I can reuse
> the code in periodical updates.

Nack,
sdap_sudo.c:144 - please use sdap_handler_done() for better clarity

I'm not quuite sure why are you swicthing to EAGAIN in 
sdap_sudo_load_sudoers_next_base(). I'd understand if you used EOK as well, 
but this way I doubt it has any advantage (doesn't even add lucidity IMO).

> [PATCH 3/4]
> Adds two new functions in sudo sysdb api that can set/retrieve the value
> of 'refreshed' attribute (equivalent to 'has_enumerated' attribute in
> domain enumeration). If it is true, the first schedule will be delayed
> for a few seconds so we don't slow down the start up process.

Nack,

I think that re-using sysdb_set_enumerated() and sysdb_has_enumarated() and 
writing wrappers around them is far better than copying the code, which is 
almost identical.

> [PATCH 4/4]
> Periodical update itself. It adds two new options:
> ldap_sudo_refresh_enabled (bool) - equivalent to 'enumerate'
> ldap_sudo_refresh_timeout (number) - equivalent to
> 'ldap_enumeration_refresh_timeout'

Nack,
Add new options to config description in src/config/, also please review if all 
of your original SUDO-related options are there as well

I think that delaying the refresh for ldap_sudo_refresh_timeout should be used 
instead of hardcoded 10 seconds. Or do you think that default value for the 
option is too high for this?

Nitpick: in the definition of sdap_sudo_refresh_timer(), you miss the "static" 
keyword

You are forgetting to free sudo_ctx in sdap_sudo_refresh_timer, which could be 
potentially deadly on long-running systems, since the refresh_ctx isn't freed 
anywhere as weel

Please use the standard _send/_retry/_done/_recv function naming in 
sdap_sudo_timer.c

In sdap_sudo_refresh_send, you create request with struct 
sdap_sudo_refresh_state as its state, but in the timeout handling procedure, 
you retrieve callback data of that request as struct sdap_sudo_refresh_ctx. 
Please correct that and keep in mind that since this is entirely different 
struct assigned to the request, freeing the request won't free the sudo_ctx 
created in sdap_sudo_refresh_timer().


Thanks
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120111/62a28fcd/attachment.sig>


More information about the sssd-devel mailing list