[PATCHES] SUDO Integration - periodical update

[Pavel Březina]

https://fedorahosted.org/sssd/ticket/1110

[PATCH 1/4]
I've removed FIXME and move the DEBUG message to a better place.

[PATCH 2/4]
I've wrapped the data provider part with tevent request so I can reuse 
the code in periodical updates.

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

[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'

[Jakub Hrozek]

On Tue, Dec 20, 2011 at 02:50:31PM +0100, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/1110
>
> [PATCH 1/4]
> I've removed FIXME and move the DEBUG message to a better place.

I know Jan is doing more in-depth review but I would prefer to scrap
this patch and make it part of the one that would fix review items
(#1125).

[Jan Zelený]

> 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
> [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?

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

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

Thanks
Jan

[Pavel Březina]

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.

[Jakub Hrozek]

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

In sdap_sudo_reply(), please call return after the first
sdap_handler_done()

In general, try to mark functions that are private to the module as
"static".

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

I think that's OK.
> > 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.

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.

[Pavel Březina]

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.

[Jan Zelený]

> 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

[Jakub Hrozek]

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

[Pavel Březina]

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.

[Jakub Hrozek]

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.

[Stephen Gallagher]

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.