[SSSD] [PATCHES] SUDO Integration - periodical update

Jan Zelený jzeleny at redhat.com
Tue Jan 17 13:52:44 UTC 2012


> 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.

Could you please rebase on top of current master?

Thanks
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120117/cbd41c75/attachment.sig>


More information about the sssd-devel mailing list