[SSSD] [PATCHES] SUDO Integration - periodical update

Stephen Gallagher sgallagh at redhat.com
Tue Jan 17 16:38:38 UTC 2012


On Tue, 2012-01-17 at 17:11 +0100, Jakub Hrozek wrote:
> 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.

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


More information about the sssd-devel mailing list