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'
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).
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
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.
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.
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.
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
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)
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.
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.
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.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Pavel Březina -
Stephen Gallagher