[SSSD] [PATCHES] SUDO Integration - periodical update

Pavel Březina pbrezina at redhat.com
Tue Jan 17 16:02:57 UTC 2012


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.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-SUDO-Integration-wrap-data-provider-with-tevent_req.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120117/98429edb/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-sysdb_get_bool-and-sysdb_get_bool-functions.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120117/98429edb/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0003-SUDO-Integration-functions-for-manipulating-with-ref.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120117/98429edb/attachment-0002.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0004-SUDO-Integration-periodical-update-of-rules-in-data-.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120117/98429edb/attachment-0003.ksh>


More information about the sssd-devel mailing list