On Mon, Jan 16, 2012 at 05:34:01PM +0100, Pavel Březina wrote:
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.
0001: ack
0002: nack, please split this patch into one that would implement just
the get_bool/set_bool sysdb functions and one that implements the getter
and setter for the refreshed attribute
0003: ack
0004: this patch does not belong into this thread. It should be posted
separately (and we should hold posting it until we are in the clear on
the correct form of the filter)