Dne 16.1.2012 15:56, Jakub Hrozek napsal(a):
On Thu, Jan 12, 2012 at 01:06:27PM +0100, Pavel Březina wrote:
Dne 11.1.2012 13:47, Jan Zelený napsal(a):
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
Sure. Squashed to the review patch.
[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
Done.
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).
Done.
In sdap_sudo_reply(), please call return after the first sdap_handler_done()
Done.
The structure sdap_sudo_refresh_state does not have to be present in the sdap_sudo.h header, it is internal to the sdap_sudo_refresh request. It is better to define it next to the _send() function.
Done.
[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.
Done.
[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
Done.
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?
I've used the value which is hard coded in sdap_id_setup_tasks() so I assume that original author had some reason for it. Any opinions?1
I think that's OK.
Nitpick: in the definition of sdap_sudo_refresh_timer(), you miss the "static" keyword
Done.
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
I chose naming convention that is introduced in ldap_id_enum.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().
sdap_sudo_refresh_timer(): struct sdap_sudo_refresh_ctx *refresh_ctx; req = sdap_sudo_refresh_send(sudo_ctx, sudo_ctx); tevent_req_set_callback(req, sdap_sudo_refresh_reschedule, *refresh_ctx*);
sdap_sudo_refresh_send(TALLOC_CTX *mem_ctx, struct sdap_sudo_ctx *sudo_ctx): struct sdap_sudo_refresh_state *state; req = tevent_req_create(mem_ctx,&state, struct sdap_sudo_refresh_state);
Now in callback: sdap_sudo_refresh_reschedule(struct tevent_req *req): refresh_ctx = tevent_req_callback_data(req, struct sdap_sudo_refresh_ctx); *retrieves correct structure* ret = sdap_sudo_refresh_recv(req,&sudo_ctx,&dp_error,&error); *retrieves sdap_sudo_refresh_state and returns sudo_ctx* talloc_zfree(req); *sdap_sudo_refresh_state is freed* talloc_zfree(sudo_ctx); *sudo_ctx is freed*
refresh_ctx should be allocated for the lifetime of data provider.
The structure sdap_sudo_refresh_ctx should be internal to sdap_sudo_timer.c and the module should expose a one-time setup function that would allocate the context for future use.
Done.
Corrected patches are in the attachment.
Thank you both for the review!
Pavel.