[SSSD] [PATCHES] SUDO Integration - periodical update

Pavel Březina pbrezina at redhat.com
Thu Jan 12 12:06:27 UTC 2012


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.






More information about the sssd-devel mailing list