On Tue, Jan 17, 2012 at 05:02:57PM +0100, Pavel Březina wrote:
Dne 17.1.2012 16:54, Jakub Hrozek napsal(a):
>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
Patches attached.
>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)
Yes, I've sent it by accident.
Ack to all four patches.