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
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?
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
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
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.