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.