Hello everybody, here is the first set of patches for the new sudo clothes. [1]
As it does not touch the responder, I believe it does not require the patches from the other preliminary sudo thread (already acked). However it was written atop them.
[2/10] This is the main change for the async processing of the sudo rules. It make sdap_sudo_refresh_send() more generic by adding there two filters: ldap_filter - used for search in the LDAP sysdb_filter - used for a deletion of the rules from sysdb
This way we can have many different refresh styles without touching this fundamental function.
From this patch further the provider will return an error to the responder making it unusable.
[...] The rest of the patches has a self-describing subject. Don't be afraid of the count, these patches are very small :-)
The next set of patches is where the fun begin, so be patient :-)
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
On 04/23/2012 05:04 PM, Pavel Březina wrote:
Hello everybody, here is the first set of patches for the new sudo clothes. [1]
As it does not touch the responder, I believe it does not require the patches from the other preliminary sudo thread (already acked). However it was written atop them.
[2/10] This is the main change for the async processing of the sudo rules. It make sdap_sudo_refresh_send() more generic by adding there two filters: ldap_filter - used for search in the LDAP sysdb_filter - used for a deletion of the rules from sysdb
This way we can have many different refresh styles without touching this fundamental function.
From this patch further the provider will return an error to the responder making it unusable.
[...] The rest of the patches has a self-describing subject. Don't be afraid of the count, these patches are very small :-)
The next set of patches is where the fun begin, so be patient :-)
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
Ok, I made a mistake already in the 3rd patch, I'm sending it again :)
On 04/23/2012 06:34 PM, Pavel Březina wrote:
On 04/23/2012 05:04 PM, Pavel Březina wrote:
Hello everybody, here is the first set of patches for the new sudo clothes. [1]
As it does not touch the responder, I believe it does not require the patches from the other preliminary sudo thread (already acked). However it was written atop them.
[2/10] This is the main change for the async processing of the sudo rules. It make sdap_sudo_refresh_send() more generic by adding there two filters: ldap_filter - used for search in the LDAP sysdb_filter - used for a deletion of the rules from sysdb
This way we can have many different refresh styles without touching this fundamental function.
From this patch further the provider will return an error to the responder making it unusable.
[...] The rest of the patches has a self-describing subject. Don't be afraid of the count, these patches are very small :-)
The next set of patches is where the fun begin, so be patient :-)
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
Ok, I made a mistake already in the 3rd patch, I'm sending it again :)
Sending new set of patches if you want to take a look.
Full periodical updates should run fine, there are preparations for the smart update. I'm going to ignore modifyTimestamp for now and use only entryUSN if available.
It also does not filter the rules by host, I plan to add it as one of the last things.
On 05/07/2012 07:13 PM, Pavel Březina wrote:
On 04/23/2012 06:34 PM, Pavel Březina wrote:
On 04/23/2012 05:04 PM, Pavel Březina wrote:
Hello everybody, here is the first set of patches for the new sudo clothes. [1]
As it does not touch the responder, I believe it does not require the patches from the other preliminary sudo thread (already acked). However it was written atop them.
[2/10] This is the main change for the async processing of the sudo rules. It make sdap_sudo_refresh_send() more generic by adding there two filters: ldap_filter - used for search in the LDAP sysdb_filter - used for a deletion of the rules from sysdb
This way we can have many different refresh styles without touching this fundamental function.
From this patch further the provider will return an error to the responder making it unusable.
[...] The rest of the patches has a self-describing subject. Don't be afraid of the count, these patches are very small :-)
The next set of patches is where the fun begin, so be patient :-)
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
Ok, I made a mistake already in the 3rd patch, I'm sending it again :)
Sending new set of patches if you want to take a look.
Full periodical updates should run fine, there are preparations for the smart update. I'm going to ignore modifyTimestamp for now and use only entryUSN if available.
It also does not filter the rules by host, I plan to add it as one of the last things.
The provider part is now complete except the above given limitations.
On 9.5.2012 17:07, Pavel Březina wrote:
On 05/07/2012 07:13 PM, Pavel Březina wrote:
On 04/23/2012 06:34 PM, Pavel Březina wrote:
On 04/23/2012 05:04 PM, Pavel Březina wrote:
Hello everybody, here is the first set of patches for the new sudo clothes. [1]
As it does not touch the responder, I believe it does not require the patches from the other preliminary sudo thread (already acked). However it was written atop them.
[2/10] This is the main change for the async processing of the sudo rules. It make sdap_sudo_refresh_send() more generic by adding there two filters: ldap_filter - used for search in the LDAP sysdb_filter - used for a deletion of the rules from sysdb
This way we can have many different refresh styles without touching this fundamental function.
From this patch further the provider will return an error to the responder making it unusable.
[...] The rest of the patches has a self-describing subject. Don't be afraid of the count, these patches are very small :-)
The next set of patches is where the fun begin, so be patient :-)
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
Ok, I made a mistake already in the 3rd patch, I'm sending it again :)
Sending new set of patches if you want to take a look.
Full periodical updates should run fine, there are preparations for the smart update. I'm going to ignore modifyTimestamp for now and use only entryUSN if available.
It also does not filter the rules by host, I plan to add it as one of the last things.
The provider part is now complete except the above given limitations.
Ok, the major changes are done in this patch set.
What left is: - download rules per host, this only appends a filter to the search so it should not invoke any logic change
- it ignores modifyTimestamp - simo suggested to use it in smart refresh when enabled in configuration and USN not available, but do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
On Mon, 2012-05-14 at 18:39 +0200, Pavel Březina wrote:
- it ignores modifyTimestamp - simo suggested to use it in smart refresh when enabled in configuration and USN not available, but
do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
We use it for smart updates in the user/group enumeration code if entryUSN is not available. IIRC.
Simo.
On Mon, 2012-05-14 at 13:16 -0400, Simo Sorce wrote:
On Mon, 2012-05-14 at 18:39 +0200, Pavel Březina wrote:
- it ignores modifyTimestamp - simo suggested to use it in smart refresh when enabled in configuration and USN not available, but
do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
We use it for smart updates in the user/group enumeration code if entryUSN is not available. IIRC.
Yes, it's our fallback for servers that don't support entryUSN.
On 14.5.2012 20:40, Stephen Gallagher wrote:
On Mon, 2012-05-14 at 13:16 -0400, Simo Sorce wrote:
On Mon, 2012-05-14 at 18:39 +0200, Pavel Březina wrote:
- it ignores modifyTimestamp - simo suggested to use it in smart refresh when enabled in configuration and USN not available, but
do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
We use it for smart updates in the user/group enumeration code if entryUSN is not available. IIRC.
Yes, it's our fallback for servers that don't support entryUSN.
Interesting, I would swear that I searched for it, but now I can clearly see it in the code. I'm sorry for the rumours. I'll prepare a patch.
FYI I'm not going to modify these patches again so they are safe to review.
On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote:
On 9.5.2012 17:07, Pavel Březina wrote:
On 05/07/2012 07:13 PM, Pavel Březina wrote:
On 04/23/2012 06:34 PM, Pavel Březina wrote:
On 04/23/2012 05:04 PM, Pavel Březina wrote:
Hello everybody, here is the first set of patches for the new sudo clothes. [1]
As it does not touch the responder, I believe it does not require the patches from the other preliminary sudo thread (already acked). However it was written atop them.
[2/10] This is the main change for the async processing of the sudo rules. It make sdap_sudo_refresh_send() more generic by adding there two filters: ldap_filter - used for search in the LDAP sysdb_filter - used for a deletion of the rules from sysdb
This way we can have many different refresh styles without touching this fundamental function.
From this patch further the provider will return an error to the responder making it unusable.
[...] The rest of the patches has a self-describing subject. Don't be afraid of the count, these patches are very small :-)
The next set of patches is where the fun begin, so be patient :-)
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
Ok, I made a mistake already in the 3rd patch, I'm sending it again :)
Sending new set of patches if you want to take a look.
Full periodical updates should run fine, there are preparations for the smart update. I'm going to ignore modifyTimestamp for now and use only entryUSN if available.
It also does not filter the rules by host, I plan to add it as one of the last things.
The provider part is now complete except the above given limitations.
Ok, the major changes are done in this patch set.
What left is:
download rules per host, this only appends a filter to the search so it should not invoke any logic change
it ignores modifyTimestamp - simo suggested to use it in smart refresh when enabled in configuration and USN not available, but do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
Pavel, I won't be able to review the huge patchset in a whole, so I started with the first couple of patches. Do you know offhand if there is a particular point where several patches could be applied and the provider would work so that when first N patches are acked, we can push them and decrease the amount of rebasing?
0001: sudo ldap provider: move async routines to sdap_async_sudo.c Ack, this patch just moves code around
0002: sudo ldap provider: give sdap_sudo_refresh_send() search and purge filters Ack. The sudo provider is not functional after this patch until patch #8, but I think that's OK. This split is as good as any.
0003: confdb: add entry_cache_sudo_timeout option Nack, the new option is missing from the configAPI Also, I assume the new option is going to be used when the responder changes land? So far, it's only read, not used.
0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state Ack, but I don't get the point?
0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state Same as 0004, what's the intent?
0006: sudo ldap provider: add expiration time to each rule Ack
0007: sysdb_sudo_set_refresh_time Looks good, but do you anticipate that sysdb_sudo_{get,set}_refresh_time would be called with any other attribute name than SYSDB_SUDO_AT_LAST_FULL_REFRESH?
0008: sudo ldap provider: provide API for full refresh + + /* free filters with the request */ + talloc_steal(subreq, ldap_filter); + talloc_steal(subreq, sysdb_filter); +
Do the filters need to outlive the top-level request? It doesn't look like it's the case, but if so, then I would say it's more natural to strdup them in the subreq, so that other potential consumers of sdap_sudo_refresh_send() don't need to remember to steal the filters.
_recv should come after _done in the source file, otherwise the code reads funny.
In sdap_sudo_full_refresh_done(), you read the req and state variables twice in the patch, but not in the resulting code, so I assume it was fixed in a follow-up patch. Is it too much trouble to fix? If it is, don't bother.
I think you meant to quit the request here with an error? If not, please put a comment saying that you ignore an error intentionally: + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of " + "a successful full refresh\n")); + }
0009: sudo ldap provider: add support for on demand full refresh + default: + DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n", + sudo_req->type)); + } I think you should abort the request with something like EINVAL here. I know that the code would not continue because of the following (req != NULL) check but the OOM error would be pretty confusing for the caller.
0010: provide API for refresh of specific rules Same comment with stealing the filters as with patch #9. Frankly, I don't really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be more readable to create both _done and _recv?
0011: sudo ldap provider: add support for on demand refresh of specific rules Ack
I'm going to continue tomorrow. I also haven't done any testing yet, just reading the code.
On 14.5.2012 22:37, Jakub Hrozek wrote:
On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote:
On 9.5.2012 17:07, Pavel Březina wrote:
On 05/07/2012 07:13 PM, Pavel Březina wrote:
On 04/23/2012 06:34 PM, Pavel Březina wrote:
On 04/23/2012 05:04 PM, Pavel Březina wrote:
Hello everybody, here is the first set of patches for the new sudo clothes. [1]
As it does not touch the responder, I believe it does not require the patches from the other preliminary sudo thread (already acked). However it was written atop them.
[2/10] This is the main change for the async processing of the sudo rules. It make sdap_sudo_refresh_send() more generic by adding there two filters: ldap_filter - used for search in the LDAP sysdb_filter - used for a deletion of the rules from sysdb
This way we can have many different refresh styles without touching this fundamental function.
From this patch further the provider will return an error to the responder making it unusable.
[...] The rest of the patches has a self-describing subject. Don't be afraid of the count, these patches are very small :-)
The next set of patches is where the fun begin, so be patient :-)
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
Ok, I made a mistake already in the 3rd patch, I'm sending it again :)
Sending new set of patches if you want to take a look.
Full periodical updates should run fine, there are preparations for the smart update. I'm going to ignore modifyTimestamp for now and use only entryUSN if available.
It also does not filter the rules by host, I plan to add it as one of the last things.
The provider part is now complete except the above given limitations.
Ok, the major changes are done in this patch set.
What left is:
download rules per host, this only appends a filter to the search so it should not invoke any logic change
it ignores modifyTimestamp - simo suggested to use it in smart refresh when enabled in configuration and USN not available, but do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
Pavel, I won't be able to review the huge patchset in a whole, so I started with the first couple of patches. Do you know offhand if there is a particular point where several patches could be applied and the provider would work so that when first N patches are acked, we can push them and decrease the amount of rebasing?
0001: sudo ldap provider: move async routines to sdap_async_sudo.c Ack, this patch just moves code around
0002: sudo ldap provider: give sdap_sudo_refresh_send() search and purge filters Ack. The sudo provider is not functional after this patch until patch #8, but I think that's OK. This split is as good as any.
0003: confdb: add entry_cache_sudo_timeout option Nack, the new option is missing from the configAPI Also, I assume the new option is going to be used when the responder changes land? So far, it's only read, not used.
Your assumption is correct. Done. I have also fixed it in subsequent patches. I really need to write it somewhere on my desk :-)
0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state Ack, but I don't get the point?
0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state Same as 0004, what's the intent?
Just to shorten access to these contexts.
0006: sudo ldap provider: add expiration time to each rule Ack
0007: sysdb_sudo_set_refresh_time Looks good, but do you anticipate that sysdb_sudo_{get,set}_refresh_time would be called with any other attribute name than SYSDB_SUDO_AT_LAST_FULL_REFRESH?
Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I found out later that it is not necessary.
The intention was to use that value to compute when the first smart refresh should be scheduled after sssd is started. However, it needs to know the current highest USN value (which would be a little tricky). Therefore I'm doing full refresh at first and then enable periodical smart updates (as we have discussed on site).
0008: sudo ldap provider: provide API for full refresh
- /* free filters with the request */
- talloc_steal(subreq, ldap_filter);
- talloc_steal(subreq, sysdb_filter);
Do the filters need to outlive the top-level request? It doesn't look like it's the case, but if so, then I would say it's more natural to strdup them in the subreq, so that other potential consumers of sdap_sudo_refresh_send() don't need to remember to steal the filters.
Well, you don't really need to steal them. But I don't have any use for them outside the subreq so I think it is correct to make them deallocate with subreq.
Those are constant input data, so there is no need to strdup them.
_recv should come after _done in the source file, otherwise the code reads funny.
Ok. There would be unworthy rebasing troubles, thus I have moved it in a new patch.
In sdap_sudo_full_refresh_done(), you read the req and state variables twice in the patch, but not in the resulting code, so I assume it was fixed in a follow-up patch. Is it too much trouble to fix? If it is, don't bother.
This is an error from one of many rebases I have done. Fixed.
I think you meant to quit the request here with an error? If not, please put a comment saying that you ignore an error intentionally:
- if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of ""a successful full refresh\n"));- }
I have added the following comment: /* this is only a minor error that does not affect the functionality, * therefore there is no need to report it with tevent_req_error() * which would cause problems in the consumers */
0009: sudo ldap provider: add support for on demand full refresh
- default:
DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n",sudo_req->type));- }
I think you should abort the request with something like EINVAL here. I know that the code would not continue because of the following (req != NULL) check but the OOM error would be pretty confusing for the caller.
Yes, definitely. The same problem was also in sdap_sudo_reply() where ret would be uninitialized in ret != EOK condition. Both fixed.
0010: provide API for refresh of specific rules Same comment with stealing the filters as with patch #9. Frankly, I don't really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be more readable to create both _done and _recv?
I found it necessary after all, because I needed to create there a new request. It is converted to it in one of the last patches.
0011: sudo ldap provider: add support for on demand refresh of specific rules Ack
I'm going to continue tomorrow. I also haven't done any testing yet, just reading the code.
I have also fixed typo in man page and added new patch that handles modification of the highest USN value after a specific rules refresh.
New patches are attached.
Thanks for the review. Keep coming :-)
On 3.6.2012 22:17, Pavel Březina wrote:
On 14.5.2012 22:37, Jakub Hrozek wrote:
On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote:
On 9.5.2012 17:07, Pavel Březina wrote:
On 05/07/2012 07:13 PM, Pavel Březina wrote:
On 04/23/2012 06:34 PM, Pavel Březina wrote:
On 04/23/2012 05:04 PM, Pavel Březina wrote: > Hello everybody, > here is the first set of patches for the new sudo clothes. [1] > > As it does not touch the responder, I believe it does not require > the > patches from the other preliminary sudo thread (already acked). > However > it was written atop them. > > [2/10] > This is the main change for the async processing of the sudo > rules. It > make sdap_sudo_refresh_send() more generic by adding there two > filters: > ldap_filter - used for search in the LDAP > sysdb_filter - used for a deletion of the rules from sysdb > > This way we can have many different refresh styles without touching > this fundamental function. > > From this patch further the provider will return an error to the > responder making it unusable. > > [...] > The rest of the patches has a self-describing subject. Don't be > afraid > of the count, these patches are very small :-) > > The next set of patches is where the fun begin, so be patient :-) > > [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
Ok, I made a mistake already in the 3rd patch, I'm sending it again :)
Sending new set of patches if you want to take a look.
Full periodical updates should run fine, there are preparations for the smart update. I'm going to ignore modifyTimestamp for now and use only entryUSN if available.
It also does not filter the rules by host, I plan to add it as one of the last things.
The provider part is now complete except the above given limitations.
Ok, the major changes are done in this patch set.
What left is:
- download rules per host, this only appends a filter to the search so
it should not invoke any logic change
- it ignores modifyTimestamp - simo suggested to use it in smart
refresh when enabled in configuration and USN not available, but do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
Pavel, I won't be able to review the huge patchset in a whole, so I started with the first couple of patches. Do you know offhand if there is a particular point where several patches could be applied and the provider would work so that when first N patches are acked, we can push them and decrease the amount of rebasing?
0001: sudo ldap provider: move async routines to sdap_async_sudo.c Ack, this patch just moves code around
0002: sudo ldap provider: give sdap_sudo_refresh_send() search and purge filters Ack. The sudo provider is not functional after this patch until patch #8, but I think that's OK. This split is as good as any.
0003: confdb: add entry_cache_sudo_timeout option Nack, the new option is missing from the configAPI Also, I assume the new option is going to be used when the responder changes land? So far, it's only read, not used.
Your assumption is correct. Done. I have also fixed it in subsequent patches. I really need to write it somewhere on my desk :-)
0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state Ack, but I don't get the point?
0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state Same as 0004, what's the intent?
Just to shorten access to these contexts.
0006: sudo ldap provider: add expiration time to each rule Ack
0007: sysdb_sudo_set_refresh_time Looks good, but do you anticipate that sysdb_sudo_{get,set}_refresh_time would be called with any other attribute name than SYSDB_SUDO_AT_LAST_FULL_REFRESH?
Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I found out later that it is not necessary.
The intention was to use that value to compute when the first smart refresh should be scheduled after sssd is started. However, it needs to know the current highest USN value (which would be a little tricky). Therefore I'm doing full refresh at first and then enable periodical smart updates (as we have discussed on site).
0008: sudo ldap provider: provide API for full refresh
- /* free filters with the request */
- talloc_steal(subreq, ldap_filter);
- talloc_steal(subreq, sysdb_filter);
Do the filters need to outlive the top-level request? It doesn't look like it's the case, but if so, then I would say it's more natural to strdup them in the subreq, so that other potential consumers of sdap_sudo_refresh_send() don't need to remember to steal the filters.
Well, you don't really need to steal them. But I don't have any use for them outside the subreq so I think it is correct to make them deallocate with subreq.
Those are constant input data, so there is no need to strdup them.
_recv should come after _done in the source file, otherwise the code reads funny.
Ok. There would be unworthy rebasing troubles, thus I have moved it in a new patch.
In sdap_sudo_full_refresh_done(), you read the req and state variables twice in the patch, but not in the resulting code, so I assume it was fixed in a follow-up patch. Is it too much trouble to fix? If it is, don't bother.
This is an error from one of many rebases I have done. Fixed.
I think you meant to quit the request here with an error? If not, please put a comment saying that you ignore an error intentionally:
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of "
- "a successful full refresh\n"));
- }
I have added the following comment: /* this is only a minor error that does not affect the functionality,
- therefore there is no need to report it with tevent_req_error()
- which would cause problems in the consumers */
0009: sudo ldap provider: add support for on demand full refresh
- default:
- DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n",
- sudo_req->type));
- }
I think you should abort the request with something like EINVAL here. I know that the code would not continue because of the following (req != NULL) check but the OOM error would be pretty confusing for the caller.
Yes, definitely. The same problem was also in sdap_sudo_reply() where ret would be uninitialized in ret != EOK condition. Both fixed.
0010: provide API for refresh of specific rules Same comment with stealing the filters as with patch #9. Frankly, I don't really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be more readable to create both _done and _recv?
I found it necessary after all, because I needed to create there a new request. It is converted to it in one of the last patches.
0011: sudo ldap provider: add support for on demand refresh of specific rules Ack
I'm going to continue tomorrow. I also haven't done any testing yet, just reading the code.
I have also fixed typo in man page and added new patch that handles modification of the highest USN value after a specific rules refresh.
New patches are attached.
Thanks for the review. Keep coming :-)
Rebased on the current master and patches from "sudo: send username and uid while requesting default options" thread.
On 06/21/2012 05:32 PM, Pavel Březina wrote:
On 3.6.2012 22:17, Pavel Březina wrote:
On 14.5.2012 22:37, Jakub Hrozek wrote:
On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote:
On 9.5.2012 17:07, Pavel Březina wrote:
On 05/07/2012 07:13 PM, Pavel Březina wrote:
On 04/23/2012 06:34 PM, Pavel Březina wrote: > On 04/23/2012 05:04 PM, Pavel Březina wrote: >> Hello everybody, >> here is the first set of patches for the new sudo clothes. [1] >> >> As it does not touch the responder, I believe it does not require >> the >> patches from the other preliminary sudo thread (already acked). >> However >> it was written atop them. >> >> [2/10] >> This is the main change for the async processing of the sudo >> rules. It >> make sdap_sudo_refresh_send() more generic by adding there two >> filters: >> ldap_filter - used for search in the LDAP >> sysdb_filter - used for a deletion of the rules from sysdb >> >> This way we can have many different refresh styles without touching >> this fundamental function. >> > > From this patch further the provider will return an error to the >> responder making it unusable. >> >> [...] >> The rest of the patches has a self-describing subject. Don't be >> afraid >> of the count, these patches are very small :-) >> >> The next set of patches is where the fun begin, so be patient :-) >> >> [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules > > Ok, I made a mistake already in the 3rd patch, I'm sending it > again :)
Sending new set of patches if you want to take a look.
Full periodical updates should run fine, there are preparations for the smart update. I'm going to ignore modifyTimestamp for now and use only entryUSN if available.
It also does not filter the rules by host, I plan to add it as one of the last things.
The provider part is now complete except the above given limitations.
Ok, the major changes are done in this patch set.
What left is:
- download rules per host, this only appends a filter to the search so
it should not invoke any logic change
- it ignores modifyTimestamp - simo suggested to use it in smart
refresh when enabled in configuration and USN not available, but do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
Pavel, I won't be able to review the huge patchset in a whole, so I started with the first couple of patches. Do you know offhand if there is a particular point where several patches could be applied and the provider would work so that when first N patches are acked, we can push them and decrease the amount of rebasing?
0001: sudo ldap provider: move async routines to sdap_async_sudo.c Ack, this patch just moves code around
0002: sudo ldap provider: give sdap_sudo_refresh_send() search and purge filters Ack. The sudo provider is not functional after this patch until patch #8, but I think that's OK. This split is as good as any.
0003: confdb: add entry_cache_sudo_timeout option Nack, the new option is missing from the configAPI Also, I assume the new option is going to be used when the responder changes land? So far, it's only read, not used.
Your assumption is correct. Done. I have also fixed it in subsequent patches. I really need to write it somewhere on my desk :-)
0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state Ack, but I don't get the point?
0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state Same as 0004, what's the intent?
Just to shorten access to these contexts.
0006: sudo ldap provider: add expiration time to each rule Ack
0007: sysdb_sudo_set_refresh_time Looks good, but do you anticipate that sysdb_sudo_{get,set}_refresh_time would be called with any other attribute name than SYSDB_SUDO_AT_LAST_FULL_REFRESH?
Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I found out later that it is not necessary.
The intention was to use that value to compute when the first smart refresh should be scheduled after sssd is started. However, it needs to know the current highest USN value (which would be a little tricky). Therefore I'm doing full refresh at first and then enable periodical smart updates (as we have discussed on site).
0008: sudo ldap provider: provide API for full refresh
- /* free filters with the request */
- talloc_steal(subreq, ldap_filter);
- talloc_steal(subreq, sysdb_filter);
Do the filters need to outlive the top-level request? It doesn't look like it's the case, but if so, then I would say it's more natural to strdup them in the subreq, so that other potential consumers of sdap_sudo_refresh_send() don't need to remember to steal the filters.
Well, you don't really need to steal them. But I don't have any use for them outside the subreq so I think it is correct to make them deallocate with subreq.
Those are constant input data, so there is no need to strdup them.
_recv should come after _done in the source file, otherwise the code reads funny.
Ok. There would be unworthy rebasing troubles, thus I have moved it in a new patch.
In sdap_sudo_full_refresh_done(), you read the req and state variables twice in the patch, but not in the resulting code, so I assume it was fixed in a follow-up patch. Is it too much trouble to fix? If it is, don't bother.
This is an error from one of many rebases I have done. Fixed.
I think you meant to quit the request here with an error? If not, please put a comment saying that you ignore an error intentionally:
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of "
- "a successful full refresh\n"));
- }
I have added the following comment: /* this is only a minor error that does not affect the functionality,
- therefore there is no need to report it with tevent_req_error()
- which would cause problems in the consumers */
0009: sudo ldap provider: add support for on demand full refresh
- default:
- DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n",
- sudo_req->type));
- }
I think you should abort the request with something like EINVAL here. I know that the code would not continue because of the following (req != NULL) check but the OOM error would be pretty confusing for the caller.
Yes, definitely. The same problem was also in sdap_sudo_reply() where ret would be uninitialized in ret != EOK condition. Both fixed.
0010: provide API for refresh of specific rules Same comment with stealing the filters as with patch #9. Frankly, I don't really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be more readable to create both _done and _recv?
I found it necessary after all, because I needed to create there a new request. It is converted to it in one of the last patches.
0011: sudo ldap provider: add support for on demand refresh of specific rules Ack
I'm going to continue tomorrow. I also haven't done any testing yet, just reading the code.
I have also fixed typo in man page and added new patch that handles modification of the highest USN value after a specific rules refresh.
New patches are attached.
Thanks for the review. Keep coming :-)
Rebased on the current master and patches from "sudo: send username and uid while requesting default options" thread.
Added support for modifyTimestamp as a fallback when USN attributes are not available (squashed to #21).
And I have also noticed that 'entry_cache_sudo_timeout' broke the config test. The fix is squashed to #3.
Dne úterý 26 června 2012 10:08:15, Pavel Březina napsal(a):
On 06/21/2012 05:32 PM, Pavel Březina wrote:
On 3.6.2012 22:17, Pavel Březina wrote:
On 14.5.2012 22:37, Jakub Hrozek wrote:
On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote:
On 9.5.2012 17:07, Pavel Březina wrote:
On 05/07/2012 07:13 PM, Pavel Březina wrote: > On 04/23/2012 06:34 PM, Pavel Březina wrote: >> On 04/23/2012 05:04 PM, Pavel Březina wrote: >>> Hello everybody, >>> here is the first set of patches for the new sudo clothes. [1] >>> >>> As it does not touch the responder, I believe it does not require >>> the >>> patches from the other preliminary sudo thread (already acked). >>> However >>> it was written atop them. >>> >>> [2/10] >>> This is the main change for the async processing of the sudo >>> rules. It >>> make sdap_sudo_refresh_send() more generic by adding there two >>> filters: >>> ldap_filter - used for search in the LDAP >>> sysdb_filter - used for a deletion of the rules from sysdb >>> >>> This way we can have many different refresh styles without touching >>> this fundamental function. >>> >> > From this patch further the provider will return an error to the >>> >>> responder making it unusable. >>> >>> [...] >>> The rest of the patches has a self-describing subject. Don't be >>> afraid >>> of the count, these patches are very small :-) >>> >>> The next set of patches is where the fun begin, so be patient :-) >>> >>> [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules >> >> Ok, I made a mistake already in the 3rd patch, I'm sending it >> again :) > > Sending new set of patches if you want to take a look. > > Full periodical updates should run fine, there are preparations for > the > smart update. I'm going to ignore modifyTimestamp for now and use > only > entryUSN if available. > > It also does not filter the rules by host, I plan to add it as one of > the last things.
The provider part is now complete except the above given limitations.
Ok, the major changes are done in this patch set.
What left is:
- download rules per host, this only appends a filter to the search so
it should not invoke any logic change
- it ignores modifyTimestamp - simo suggested to use it in smart
refresh when enabled in configuration and USN not available, but do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
Pavel, I won't be able to review the huge patchset in a whole, so I started with the first couple of patches. Do you know offhand if there is a particular point where several patches could be applied and the provider would work so that when first N patches are acked, we can push them and decrease the amount of rebasing?
0001: sudo ldap provider: move async routines to sdap_async_sudo.c Ack, this patch just moves code around
0002: sudo ldap provider: give sdap_sudo_refresh_send() search and purge filters Ack. The sudo provider is not functional after this patch until patch #8, but I think that's OK. This split is as good as any.
0003: confdb: add entry_cache_sudo_timeout option Nack, the new option is missing from the configAPI Also, I assume the new option is going to be used when the responder changes land? So far, it's only read, not used.
Your assumption is correct. Done. I have also fixed it in subsequent patches. I really need to write it somewhere on my desk :-)
0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state Ack, but I don't get the point?
0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state Same as 0004, what's the intent?
Just to shorten access to these contexts.
0006: sudo ldap provider: add expiration time to each rule Ack
0007: sysdb_sudo_set_refresh_time Looks good, but do you anticipate that sysdb_sudo_{get,set}_refresh_time would be called with any other attribute name than SYSDB_SUDO_AT_LAST_FULL_REFRESH?
Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I found out later that it is not necessary.
The intention was to use that value to compute when the first smart refresh should be scheduled after sssd is started. However, it needs to know the current highest USN value (which would be a little tricky). Therefore I'm doing full refresh at first and then enable periodical smart updates (as we have discussed on site).
0008: sudo ldap provider: provide API for full refresh
- /* free filters with the request */
- talloc_steal(subreq, ldap_filter);
- talloc_steal(subreq, sysdb_filter);
Do the filters need to outlive the top-level request? It doesn't look like it's the case, but if so, then I would say it's more natural to strdup them in the subreq, so that other potential consumers of sdap_sudo_refresh_send() don't need to remember to steal the filters.
Well, you don't really need to steal them. But I don't have any use for them outside the subreq so I think it is correct to make them deallocate with subreq.
Those are constant input data, so there is no need to strdup them.
_recv should come after _done in the source file, otherwise the code reads funny.
Ok. There would be unworthy rebasing troubles, thus I have moved it in a new patch.
In sdap_sudo_full_refresh_done(), you read the req and state variables twice in the patch, but not in the resulting code, so I assume it was fixed in a follow-up patch. Is it too much trouble to fix? If it is, don't bother.
This is an error from one of many rebases I have done. Fixed.
I think you meant to quit the request here with an error? If not, please put a comment saying that you ignore an error intentionally:
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of "
- "a successful full refresh\n"));
- }
I have added the following comment: /* this is only a minor error that does not affect the functionality,
- therefore there is no need to report it with tevent_req_error()
- which would cause problems in the consumers */
0009: sudo ldap provider: add support for on demand full refresh
- default:
- DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n",
- sudo_req->type));
- }
I think you should abort the request with something like EINVAL here. I know that the code would not continue because of the following (req != NULL) check but the OOM error would be pretty confusing for the caller.
Yes, definitely. The same problem was also in sdap_sudo_reply() where ret would be uninitialized in ret != EOK condition. Both fixed.
0010: provide API for refresh of specific rules Same comment with stealing the filters as with patch #9. Frankly, I don't really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be more readable to create both _done and _recv?
I found it necessary after all, because I needed to create there a new request. It is converted to it in one of the last patches.
0011: sudo ldap provider: add support for on demand refresh of specific rules Ack
I'm going to continue tomorrow. I also haven't done any testing yet, just reading the code.
I have also fixed typo in man page and added new patch that handles modification of the highest USN value after a specific rules refresh.
New patches are attached.
Thanks for the review. Keep coming :-)
Rebased on the current master and patches from "sudo: send username and uid while requesting default options" thread.
Added support for modifyTimestamp as a fallback when USN attributes are not available (squashed to #21).
And I have also noticed that 'entry_cache_sudo_timeout' broke the config test. The fix is squashed to #3.
As a general comment: please make all commit messages more verbose. They should completely describe the new functionality and explain motivation for major changes.
Also please try to merge some patches together. It is very difficult to undertand how are patches bound together and some are clearly closely bound together. I specifically pointed it out in couple places.
Patch #0001: Ack (Jakub) Patch #0002: Ack (Jakub) Patch #0003: Ack Patch #0004: Ack (Jakub) Patch #0005: Ack (Jakub) Patch #0006: Ack (Jakub) Patch #0007: Ack Patch #0008: Nack, I don't like stealing those two variables either. The correct approach would be to strdup them in sdap_sudo_refresh_send() and free both originals instead of stealing them. Also it is better (at least for the sake of consistency) to use state as the original context instead of the req.
Patch #0009: Ack Patch #0010: Nack, The comment about stealing filter at the end of sdap_sudo_rules_refresh_send() still stands, even after the subsequent patch.
At the end of the patch you have two constants with the same content. If you don't anticipate them to have different values, please merge them back into one.
Patch #0011: Ack Patch #0012: Nack, I don't get why you set sudo_req->rules = NULL while deleting similar statements for uid and groups. I'd prefer if you deleted this completely since the structure is zeroed anyway.
Patch #0013: Nack, I don't like skipping the first argument with dbus_message_iter_next(). Better approach would be to use the iterator right away and read the type with it as well. This will be a solution less susceptible to future errors if for some reason another argument would come right after type.
Nitpick: According to DBus documentation, the type of rule should be const char *
Patch #0014: Ack Patch #0015: Tentative Ack Patch #0016: Nack, Please don't free req in case of error performing tevent_add_timer() in sdap_sudo_timer_send(), the practice we use is to set an error on that request and return it if possible.
Patch #0017: Tentative Ack Patch #0018: Nack, I'm not sure what is the first part (before the "schedule" label) supposed to do exactly but I suspect those ifs are somewhat wrong. Isn't the function supposed to not schedule another round if the previous was ok? If not then I suppose you don't need them at all.
Patch #0019: Nack, please merge with #0018 Patch #0020: Nack, please merge this patch with #0015
This is the first logical part of SUDO patch set. I suggest we push it after it is finished. It is logically independent on the second part, the only thing missing would be documentation. Pushing this independently will make review a little bit more easy.
Thanks Jan
On 06/26/2012 03:45 PM, Jan Zelený wrote:
Dne úterý 26 června 2012 10:08:15, Pavel Březina napsal(a):
On 06/21/2012 05:32 PM, Pavel Březina wrote:
On 3.6.2012 22:17, Pavel Březina wrote:
On 14.5.2012 22:37, Jakub Hrozek wrote:
On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote:
On 9.5.2012 17:07, Pavel Březina wrote: > On 05/07/2012 07:13 PM, Pavel Březina wrote: >> On 04/23/2012 06:34 PM, Pavel Březina wrote: >>> On 04/23/2012 05:04 PM, Pavel Březina wrote: >>>> Hello everybody, >>>> here is the first set of patches for the new sudo clothes. [1] >>>> >>>> As it does not touch the responder, I believe it does not require >>>> the >>>> patches from the other preliminary sudo thread (already acked). >>>> However >>>> it was written atop them. >>>> >>>> [2/10] >>>> This is the main change for the async processing of the sudo >>>> rules. It >>>> make sdap_sudo_refresh_send() more generic by adding there two >>>> filters: >>>> ldap_filter - used for search in the LDAP >>>> sysdb_filter - used for a deletion of the rules from sysdb >>>> >>>> This way we can have many different refresh styles without touching >>>> this fundamental function. >>>> >>>> From this patch further the provider will return an error to the >>>> >>>> responder making it unusable. >>>> >>>> [...] >>>> The rest of the patches has a self-describing subject. Don't be >>>> afraid >>>> of the count, these patches are very small :-) >>>> >>>> The next set of patches is where the fun begin, so be patient :-) >>>> >>>> [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules >>> >>> Ok, I made a mistake already in the 3rd patch, I'm sending it >>> again :) >> >> Sending new set of patches if you want to take a look. >> >> Full periodical updates should run fine, there are preparations for >> the >> smart update. I'm going to ignore modifyTimestamp for now and use >> only >> entryUSN if available. >> >> It also does not filter the rules by host, I plan to add it as one of >> the last things. > > The provider part is now complete except the above given limitations.
Ok, the major changes are done in this patch set.
What left is:
- download rules per host, this only appends a filter to the search so
it should not invoke any logic change
- it ignores modifyTimestamp - simo suggested to use it in smart
refresh when enabled in configuration and USN not available, but do we really want to use it? There is no other part in sssd that uses it (or am I wrong?).
Pavel, I won't be able to review the huge patchset in a whole, so I started with the first couple of patches. Do you know offhand if there is a particular point where several patches could be applied and the provider would work so that when first N patches are acked, we can push them and decrease the amount of rebasing?
0001: sudo ldap provider: move async routines to sdap_async_sudo.c Ack, this patch just moves code around
0002: sudo ldap provider: give sdap_sudo_refresh_send() search and purge filters Ack. The sudo provider is not functional after this patch until patch #8, but I think that's OK. This split is as good as any.
0003: confdb: add entry_cache_sudo_timeout option Nack, the new option is missing from the configAPI Also, I assume the new option is going to be used when the responder changes land? So far, it's only read, not used.
Your assumption is correct. Done. I have also fixed it in subsequent patches. I really need to write it somewhere on my desk :-)
0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state Ack, but I don't get the point?
0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state Same as 0004, what's the intent?
Just to shorten access to these contexts.
0006: sudo ldap provider: add expiration time to each rule Ack
0007: sysdb_sudo_set_refresh_time Looks good, but do you anticipate that sysdb_sudo_{get,set}_refresh_time would be called with any other attribute name than SYSDB_SUDO_AT_LAST_FULL_REFRESH?
Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I found out later that it is not necessary.
The intention was to use that value to compute when the first smart refresh should be scheduled after sssd is started. However, it needs to know the current highest USN value (which would be a little tricky). Therefore I'm doing full refresh at first and then enable periodical smart updates (as we have discussed on site).
0008: sudo ldap provider: provide API for full refresh
- /* free filters with the request */
- talloc_steal(subreq, ldap_filter);
- talloc_steal(subreq, sysdb_filter);
Do the filters need to outlive the top-level request? It doesn't look like it's the case, but if so, then I would say it's more natural to strdup them in the subreq, so that other potential consumers of sdap_sudo_refresh_send() don't need to remember to steal the filters.
Well, you don't really need to steal them. But I don't have any use for them outside the subreq so I think it is correct to make them deallocate with subreq.
Those are constant input data, so there is no need to strdup them.
_recv should come after _done in the source file, otherwise the code reads funny.
Ok. There would be unworthy rebasing troubles, thus I have moved it in a new patch.
In sdap_sudo_full_refresh_done(), you read the req and state variables twice in the patch, but not in the resulting code, so I assume it was fixed in a follow-up patch. Is it too much trouble to fix? If it is, don't bother.
This is an error from one of many rebases I have done. Fixed.
I think you meant to quit the request here with an error? If not, please put a comment saying that you ignore an error intentionally:
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of "
- "a successful full refresh\n"));
- }
I have added the following comment: /* this is only a minor error that does not affect the functionality,
- therefore there is no need to report it with tevent_req_error()
- which would cause problems in the consumers */
0009: sudo ldap provider: add support for on demand full refresh
- default:
- DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n",
- sudo_req->type));
- }
I think you should abort the request with something like EINVAL here. I know that the code would not continue because of the following (req != NULL) check but the OOM error would be pretty confusing for the caller.
Yes, definitely. The same problem was also in sdap_sudo_reply() where ret would be uninitialized in ret != EOK condition. Both fixed.
0010: provide API for refresh of specific rules Same comment with stealing the filters as with patch #9. Frankly, I don't really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be more readable to create both _done and _recv?
I found it necessary after all, because I needed to create there a new request. It is converted to it in one of the last patches.
0011: sudo ldap provider: add support for on demand refresh of specific rules Ack
I'm going to continue tomorrow. I also haven't done any testing yet, just reading the code.
I have also fixed typo in man page and added new patch that handles modification of the highest USN value after a specific rules refresh.
New patches are attached.
Thanks for the review. Keep coming :-)
Rebased on the current master and patches from "sudo: send username and uid while requesting default options" thread.
Added support for modifyTimestamp as a fallback when USN attributes are not available (squashed to #21).
And I have also noticed that 'entry_cache_sudo_timeout' broke the config test. The fix is squashed to #3.
As a general comment: please make all commit messages more verbose. They should completely describe the new functionality and explain motivation for major changes.
Ok. I often forget that you can't see in my head. Next time :-)
Also please try to merge some patches together. It is very difficult to undertand how are patches bound together and some are clearly closely bound together. I specifically pointed it out in couple places.
Patch #0001: Ack (Jakub) Patch #0002: Ack (Jakub) Patch #0003: Ack Patch #0004: Ack (Jakub) Patch #0005: Ack (Jakub) Patch #0006: Ack (Jakub) Patch #0007: Ack Patch #0008: Nack, I don't like stealing those two variables either. The correct approach would be to strdup them in sdap_sudo_refresh_send() and free both originals instead of stealing them. Also it is better (at least for the sake of consistency) to use state as the original context instead of the req.
Done and done.
Patch #0009: Ack Patch #0010: Nack, The comment about stealing filter at the end of sdap_sudo_rules_refresh_send() still stands, even after the subsequent patch.
Done.
At the end of the patch you have two constants with the same content. If you don't anticipate them to have different values, please merge them back into one.
Done.
Patch #0011: Ack Patch #0012: Nack, I don't get why you set sudo_req->rules = NULL while deleting similar statements for uid and groups. I'd prefer if you deleted this completely since the structure is zeroed anyway.
Because uid and groups are deprecated (I had to left them in the struct so it can be still compiled, but they are deleted in the clean up patch). But yes, it can be removed, so - done.
Patch #0013: Nack, I don't like skipping the first argument with dbus_message_iter_next(). Better approach would be to use the iterator right away and read the type with it as well. This will be a solution less susceptible to future errors if for some reason another argument would come right after type.
Done.
Nitpick: According to DBus documentation, the type of rule should be const char *
Done.
Patch #0014: Ack Patch #0015: Tentative Ack Patch #0016: Nack, Please don't free req in case of error performing tevent_add_timer() in sdap_sudo_timer_send(), the practice we use is to set an error on that request and return it if possible.
Done.
Patch #0017: Tentative Ack Patch #0018: Nack, I'm not sure what is the first part (before the "schedule" label) supposed to do exactly but I suspect those ifs are somewhat wrong. Isn't the function supposed to not schedule another round if the previous was ok? If not then I suppose you don't need them at all.
If the timer went wrong, we don't care and we will just schedule a new one. But it is missing a debug message. Added.
Patch #0019: Nack, please merge with #0018
Done.
Patch #0020: Nack, please merge this patch with #0015
Done.
This is the first logical part of SUDO patch set. I suggest we push it after it is finished. It is logically independent on the second part, the only thing missing would be documentation. Pushing this independently will make review a little bit more easy.
Thanks Jan
Thank you for the review! Corrected patches are attached.
The tar ball also contains foundations for host based filtering. It introduces several new options to configure this feature manually (hostnames, ip addresses, ...) as an addition to auto configuration which I will hopefully implement tomorrow or the day after. (FYI: That will be the end of the new implementation.)
Dne úterý 26 června 2012 18:22:15, Pavel Březina napsal(a):
On 06/26/2012 03:45 PM, Jan Zelený wrote:
Dne úterý 26 června 2012 10:08:15, Pavel Březina napsal(a):
On 06/21/2012 05:32 PM, Pavel Březina wrote:
On 3.6.2012 22:17, Pavel Březina wrote:
On 14.5.2012 22:37, Jakub Hrozek wrote:
On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote: > On 9.5.2012 17:07, Pavel Březina wrote: >> On 05/07/2012 07:13 PM, Pavel Březina wrote: >>> On 04/23/2012 06:34 PM, Pavel Březina wrote: >>>> On 04/23/2012 05:04 PM, Pavel Březina wrote: >>>>> Hello everybody, >>>>> here is the first set of patches for the new sudo clothes. [1] >>>>> >>>>> As it does not touch the responder, I believe it does not require >>>>> the >>>>> patches from the other preliminary sudo thread (already acked). >>>>> However >>>>> it was written atop them. >>>>> >>>>> [2/10] >>>>> This is the main change for the async processing of the sudo >>>>> rules. It >>>>> make sdap_sudo_refresh_send() more generic by adding there two >>>>> filters: >>>>> ldap_filter - used for search in the LDAP >>>>> sysdb_filter - used for a deletion of the rules from sysdb >>>>> >>>>> This way we can have many different refresh styles without >>>>> touching >>>>> this fundamental function. >>>>> >>>>> From this patch further the provider will return an error to the >>>>> >>>>> responder making it unusable. >>>>> >>>>> [...] >>>>> The rest of the patches has a self-describing subject. Don't be >>>>> afraid >>>>> of the count, these patches are very small :-) >>>>> >>>>> The next set of patches is where the fun begin, so be patient :-) >>>>> >>>>> [1] >>>>> https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules >>>> >>>> Ok, I made a mistake already in the 3rd patch, I'm sending it >>>> again :) >>> >>> Sending new set of patches if you want to take a look. >>> >>> Full periodical updates should run fine, there are preparations for >>> the >>> smart update. I'm going to ignore modifyTimestamp for now and use >>> only >>> entryUSN if available. >>> >>> It also does not filter the rules by host, I plan to add it as one >>> of >>> the last things. >> >> The provider part is now complete except the above given >> limitations. > > Ok, the major changes are done in this patch set. > > What left is: > - download rules per host, this only appends a filter to the search > so > it should not invoke any logic change > > - it ignores modifyTimestamp - simo suggested to use it in smart > refresh when enabled in configuration and USN not available, but do > we really want to use it? There is no other part in sssd that uses it > (or am I wrong?).
Pavel, I won't be able to review the huge patchset in a whole, so I started with the first couple of patches. Do you know offhand if there is a particular point where several patches could be applied and the provider would work so that when first N patches are acked, we can push them and decrease the amount of rebasing?
0001: sudo ldap provider: move async routines to sdap_async_sudo.c Ack, this patch just moves code around
0002: sudo ldap provider: give sdap_sudo_refresh_send() search and purge filters Ack. The sudo provider is not functional after this patch until patch #8, but I think that's OK. This split is as good as any.
0003: confdb: add entry_cache_sudo_timeout option Nack, the new option is missing from the configAPI Also, I assume the new option is going to be used when the responder changes land? So far, it's only read, not used.
Your assumption is correct. Done. I have also fixed it in subsequent patches. I really need to write it somewhere on my desk :-)
0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state Ack, but I don't get the point?
0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state Same as 0004, what's the intent?
Just to shorten access to these contexts.
0006: sudo ldap provider: add expiration time to each rule Ack
0007: sysdb_sudo_set_refresh_time Looks good, but do you anticipate that sysdb_sudo_{get,set}_refresh_time would be called with any other attribute name than SYSDB_SUDO_AT_LAST_FULL_REFRESH?
Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I found out later that it is not necessary.
The intention was to use that value to compute when the first smart refresh should be scheduled after sssd is started. However, it needs to know the current highest USN value (which would be a little tricky). Therefore I'm doing full refresh at first and then enable periodical smart updates (as we have discussed on site).
0008: sudo ldap provider: provide API for full refresh
- /* free filters with the request */
- talloc_steal(subreq, ldap_filter);
- talloc_steal(subreq, sysdb_filter);
Do the filters need to outlive the top-level request? It doesn't look like it's the case, but if so, then I would say it's more natural to strdup them in the subreq, so that other potential consumers of sdap_sudo_refresh_send() don't need to remember to steal the filters.
Well, you don't really need to steal them. But I don't have any use for them outside the subreq so I think it is correct to make them deallocate with subreq.
Those are constant input data, so there is no need to strdup them.
_recv should come after _done in the source file, otherwise the code reads funny.
Ok. There would be unworthy rebasing troubles, thus I have moved it in a new patch.
In sdap_sudo_full_refresh_done(), you read the req and state variables twice in the patch, but not in the resulting code, so I assume it was fixed in a follow-up patch. Is it too much trouble to fix? If it is, don't bother.
This is an error from one of many rebases I have done. Fixed.
I think you meant to quit the request here with an error? If not, please put a comment saying that you ignore an error intentionally:
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of "
- "a successful full refresh\n"));
- }
I have added the following comment: /* this is only a minor error that does not affect the functionality,
- therefore there is no need to report it with tevent_req_error()
- which would cause problems in the consumers */
0009: sudo ldap provider: add support for on demand full refresh
- default:
- DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n",
- sudo_req->type));
- }
I think you should abort the request with something like EINVAL here. I know that the code would not continue because of the following (req != NULL) check but the OOM error would be pretty confusing for the caller.
Yes, definitely. The same problem was also in sdap_sudo_reply() where ret would be uninitialized in ret != EOK condition. Both fixed.
0010: provide API for refresh of specific rules Same comment with stealing the filters as with patch #9. Frankly, I don't really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be more readable to create both _done and _recv?
I found it necessary after all, because I needed to create there a new request. It is converted to it in one of the last patches.
0011: sudo ldap provider: add support for on demand refresh of specific rules Ack
I'm going to continue tomorrow. I also haven't done any testing yet, just reading the code.
I have also fixed typo in man page and added new patch that handles modification of the highest USN value after a specific rules refresh.
New patches are attached.
Thanks for the review. Keep coming :-)
Rebased on the current master and patches from "sudo: send username and uid while requesting default options" thread.
Added support for modifyTimestamp as a fallback when USN attributes are not available (squashed to #21).
And I have also noticed that 'entry_cache_sudo_timeout' broke the config test. The fix is squashed to #3.
As a general comment: please make all commit messages more verbose. They should completely describe the new functionality and explain motivation for major changes.
Ok. I often forget that you can't see in my head. Next time :-)
Also please try to merge some patches together. It is very difficult to undertand how are patches bound together and some are clearly closely bound together. I specifically pointed it out in couple places.
Patch #0001: Ack (Jakub) Patch #0002: Ack (Jakub)
Nack, I don't think it's a good idea to continue operations if strdup() on sysdb_filter failed. You should probably return with an error i such case.
Patch #0003: Ack Patch #0004: Ack (Jakub) Patch #0005: Ack (Jakub) Patch #0006: Ack (Jakub) Patch #0007: Ack Patch #0008: Nack, I don't like stealing those two variables either. The correct approach would be to strdup them in sdap_sudo_refresh_send() and free both originals instead of stealing them. Also it is better (at least for the sake of consistency) to use state as the original context instead of the req.
Done and done.
Nack, I'm sorry I didn't catch this in previous review but as I already commented in other patches, please don't free req upon an error, rather set an error and return the req. If you have suspicion you have similar code elsewhere, please change it there as well.
Patch #0009: Ack Patch #0010: Nack, The comment about stealing filter at the end of sdap_sudo_rules_refresh_send() still stands, even after the subsequent patch.
Done.
At the end of the patch you have two constants with the same content. If you don't anticipate them to have different values, please merge them back into one.
Done.
Ack
Patch #0011: Ack Patch #0012: Nack, I don't get why you set sudo_req->rules = NULL while deleting similar statements for uid and groups. I'd prefer if you deleted this completely since the structure is zeroed anyway.
Because uid and groups are deprecated (I had to left them in the struct so it can be still compiled, but they are deleted in the clean up patch). But yes, it can be removed, so - done.
Ack
Patch #0013: Nack, I don't like skipping the first argument with dbus_message_iter_next(). Better approach would be to use the iterator right away and read the type with it as well. This will be a solution less susceptible to future errors if for some reason another argument would come right after type.
Done.
Nitpick: According to DBus documentation, the type of rule should be const char *
Done.
Ack
Patch #0014: Ack Patch #0015: Tentative Ack
Ack
Patch #0016: Nack, Please don't free req in case of error performing tevent_add_timer() in sdap_sudo_timer_send(), the practice we use is to set an error on that request and return it if possible.
Done.
Ack
Patch #0017: Tentative Ack
Ack
Patch #0018: Nack, I'm not sure what is the first part (before the "schedule" label) supposed to do exactly but I suspect those ifs are somewhat wrong. Isn't the function supposed to not schedule another round if the previous was ok? If not then I suppose you don't need them at all.
If the timer went wrong, we don't care and we will just schedule a new one. But it is missing a debug message. Added.
Ack
Patch #0019: Nack, please merge with #0018
Done.
Patch #0020: Nack, please merge this patch with #0015
Done.
This is the first logical part of SUDO patch set. I suggest we push it after it is finished. It is logically independent on the second part, the only thing missing would be documentation. Pushing this independently will make review a little bit more easy.
Thanks Jan
Thank you for the review! Corrected patches are attached.
The tar ball also contains foundations for host based filtering. It introduces several new options to configure this feature manually (hostnames, ip addresses, ...) as an addition to auto configuration which I will hopefully implement tomorrow or the day after. (FYI: That will be the end of the new implementation.)
Thanks Jan
On 06/27/2012 11:05 AM, Jan Zelený wrote:
Dne úterý 26 června 2012 18:22:15, Pavel Březina napsal(a):
On 06/26/2012 03:45 PM, Jan Zelený wrote:
Dne úterý 26 června 2012 10:08:15, Pavel Březina napsal(a):
On 06/21/2012 05:32 PM, Pavel Březina wrote:
On 3.6.2012 22:17, Pavel Březina wrote:
On 14.5.2012 22:37, Jakub Hrozek wrote: > On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote: >> On 9.5.2012 17:07, Pavel Březina wrote: >>> On 05/07/2012 07:13 PM, Pavel Březina wrote: >>>> On 04/23/2012 06:34 PM, Pavel Březina wrote: >>>>> On 04/23/2012 05:04 PM, Pavel Březina wrote: >>>>>> Hello everybody, >>>>>> here is the first set of patches for the new sudo clothes. [1] >>>>>> >>>>>> As it does not touch the responder, I believe it does not require >>>>>> the >>>>>> patches from the other preliminary sudo thread (already acked). >>>>>> However >>>>>> it was written atop them. >>>>>> >>>>>> [2/10] >>>>>> This is the main change for the async processing of the sudo >>>>>> rules. It >>>>>> make sdap_sudo_refresh_send() more generic by adding there two >>>>>> filters: >>>>>> ldap_filter - used for search in the LDAP >>>>>> sysdb_filter - used for a deletion of the rules from sysdb >>>>>> >>>>>> This way we can have many different refresh styles without >>>>>> touching >>>>>> this fundamental function. >>>>>> >>>>>> From this patch further the provider will return an error to the >>>>>> >>>>>> responder making it unusable. >>>>>> >>>>>> [...] >>>>>> The rest of the patches has a self-describing subject. Don't be >>>>>> afraid >>>>>> of the count, these patches are very small :-) >>>>>> >>>>>> The next set of patches is where the fun begin, so be patient :-) >>>>>> >>>>>> [1] >>>>>> https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules >>>>> >>>>> Ok, I made a mistake already in the 3rd patch, I'm sending it >>>>> again :) >>>> >>>> Sending new set of patches if you want to take a look. >>>> >>>> Full periodical updates should run fine, there are preparations for >>>> the >>>> smart update. I'm going to ignore modifyTimestamp for now and use >>>> only >>>> entryUSN if available. >>>> >>>> It also does not filter the rules by host, I plan to add it as one >>>> of >>>> the last things. >>> >>> The provider part is now complete except the above given >>> limitations. >> >> Ok, the major changes are done in this patch set. >> >> What left is: >> - download rules per host, this only appends a filter to the search >> so >> it should not invoke any logic change >> >> - it ignores modifyTimestamp - simo suggested to use it in smart >> refresh when enabled in configuration and USN not available, but do >> we really want to use it? There is no other part in sssd that uses it >> (or am I wrong?). > > Pavel, I won't be able to review the huge patchset in a whole, so I > started with the first couple of patches. Do you know offhand if > there is > a particular point where several patches could be applied and the > provider > would work so that when first N patches are acked, we can push them > and > decrease the amount of rebasing? > > 0001: sudo ldap provider: move async routines to sdap_async_sudo.c > Ack, this patch just moves code around > > 0002: sudo ldap provider: give sdap_sudo_refresh_send() search and > purge filters > Ack. The sudo provider is not functional after this patch until patch > #8, > but I think that's OK. This split is as good as any. > > 0003: confdb: add entry_cache_sudo_timeout option > Nack, the new option is missing from the configAPI > Also, I assume the new option is going to be used when the responder > changes land? So far, it's only read, not used.
Your assumption is correct. Done. I have also fixed it in subsequent patches. I really need to write it somewhere on my desk :-)
> 0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state > Ack, but I don't get the point? > > 0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state > Same as 0004, what's the intent?
Just to shorten access to these contexts.
> 0006: sudo ldap provider: add expiration time to each rule > Ack > > 0007: sysdb_sudo_set_refresh_time > Looks good, but do you anticipate that > sysdb_sudo_{get,set}_refresh_time > would be called with any other attribute name than > SYSDB_SUDO_AT_LAST_FULL_REFRESH?
Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I found out later that it is not necessary.
The intention was to use that value to compute when the first smart refresh should be scheduled after sssd is started. However, it needs to know the current highest USN value (which would be a little tricky). Therefore I'm doing full refresh at first and then enable periodical smart updates (as we have discussed on site).
> 0008: sudo ldap provider: provide API for full refresh > + > + /* free filters with the request */ > + talloc_steal(subreq, ldap_filter); > + talloc_steal(subreq, sysdb_filter); > + > > Do the filters need to outlive the top-level request? It doesn't look > like > it's the case, but if so, then I would say it's more natural to strdup > them > in the subreq, so that other potential consumers of > sdap_sudo_refresh_send() > don't need to remember to steal the filters.
Well, you don't really need to steal them. But I don't have any use for them outside the subreq so I think it is correct to make them deallocate with subreq.
Those are constant input data, so there is no need to strdup them.
> _recv should come after _done in the source file, otherwise the code > reads funny.
Ok. There would be unworthy rebasing troubles, thus I have moved it in a new patch.
> In sdap_sudo_full_refresh_done(), you read the req and state variables > twice > in the patch, but not in the resulting code, so I assume it was fixed > in > a follow-up patch. Is it too much trouble to fix? If it is, don't > bother.
This is an error from one of many rebases I have done. Fixed.
> I think you meant to quit the request here with an error? If not, > please > put a comment saying that you ignore an error intentionally: > + if (ret != EOK) { > + DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of " > + "a successful full refresh\n")); > + }
I have added the following comment: /* this is only a minor error that does not affect the functionality,
- therefore there is no need to report it with tevent_req_error()
- which would cause problems in the consumers */
> 0009: sudo ldap provider: add support for on demand full refresh > + default: > + DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n", > + sudo_req->type)); > + } > I think you should abort the request with something like EINVAL here. > I know that the code would not continue because of the following (req > != NULL) > check but the OOM error would be pretty confusing for the caller.
Yes, definitely. The same problem was also in sdap_sudo_reply() where ret would be uninitialized in ret != EOK condition. Both fixed.
> 0010: provide API for refresh of specific rules > Same comment with stealing the filters as with patch #9. Frankly, I > don't > really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be > more > readable to create both _done and _recv?
I found it necessary after all, because I needed to create there a new request. It is converted to it in one of the last patches.
> 0011: sudo ldap provider: add support for on demand refresh of > specific rules > Ack > > I'm going to continue tomorrow. I also haven't done any testing yet, > just > reading the code.
I have also fixed typo in man page and added new patch that handles modification of the highest USN value after a specific rules refresh.
New patches are attached.
Thanks for the review. Keep coming :-)
Rebased on the current master and patches from "sudo: send username and uid while requesting default options" thread.
Added support for modifyTimestamp as a fallback when USN attributes are not available (squashed to #21).
And I have also noticed that 'entry_cache_sudo_timeout' broke the config test. The fix is squashed to #3.
As a general comment: please make all commit messages more verbose. They should completely describe the new functionality and explain motivation for major changes.
Ok. I often forget that you can't see in my head. Next time :-)
Also please try to merge some patches together. It is very difficult to undertand how are patches bound together and some are clearly closely bound together. I specifically pointed it out in couple places.
Patch #0001: Ack (Jakub) Patch #0002: Ack (Jakub)
Nack, I don't think it's a good idea to continue operations if strdup() on sysdb_filter failed. You should probably return with an error i such case.
Done.
Patch #0003: Ack Patch #0004: Ack (Jakub) Patch #0005: Ack (Jakub) Patch #0006: Ack (Jakub) Patch #0007: Ack Patch #0008: Nack, I don't like stealing those two variables either. The correct approach would be to strdup them in sdap_sudo_refresh_send() and free both originals instead of stealing them. Also it is better (at least for the sake of consistency) to use state as the original context instead of the req.
Done and done.
Nack, I'm sorry I didn't catch this in previous review but as I already commented in other patches, please don't free req upon an error, rather set an error and return the req. If you have suspicion you have similar code elsewhere, please change it there as well.
Done. And also fixed in other places.
Patch #0009: Ack Patch #0010: Nack, The comment about stealing filter at the end of sdap_sudo_rules_refresh_send() still stands, even after the subsequent patch.
Done.
At the end of the patch you have two constants with the same content. If you don't anticipate them to have different values, please merge them back into one.
Done.
Ack
Patch #0011: Ack Patch #0012: Nack, I don't get why you set sudo_req->rules = NULL while deleting similar statements for uid and groups. I'd prefer if you deleted this completely since the structure is zeroed anyway.
Because uid and groups are deprecated (I had to left them in the struct so it can be still compiled, but they are deleted in the clean up patch). But yes, it can be removed, so - done.
Ack
Patch #0013: Nack, I don't like skipping the first argument with dbus_message_iter_next(). Better approach would be to use the iterator right away and read the type with it as well. This will be a solution less susceptible to future errors if for some reason another argument would come right after type.
Done.
Nitpick: According to DBus documentation, the type of rule should be const char *
Done.
Ack
Patch #0014: Ack Patch #0015: Tentative Ack
Ack
Patch #0016: Nack, Please don't free req in case of error performing tevent_add_timer() in sdap_sudo_timer_send(), the practice we use is to set an error on that request and return it if possible.
Done.
Ack
Patch #0017: Tentative Ack
Ack
Patch #0018: Nack, I'm not sure what is the first part (before the "schedule" label) supposed to do exactly but I suspect those ifs are somewhat wrong. Isn't the function supposed to not schedule another round if the previous was ok? If not then I suppose you don't need them at all.
If the timer went wrong, we don't care and we will just schedule a new one. But it is missing a debug message. Added.
Ack
Patch #0019: Nack, please merge with #0018
Done.
Patch #0020: Nack, please merge this patch with #0015
Done.
This is the first logical part of SUDO patch set. I suggest we push it after it is finished. It is logically independent on the second part, the only thing missing would be documentation. Pushing this independently will make review a little bit more easy.
Thanks Jan
Thank you for the review! Corrected patches are attached.
The tar ball also contains foundations for host based filtering. It introduces several new options to configure this feature manually (hostnames, ip addresses, ...) as an addition to auto configuration which I will hopefully implement tomorrow or the day after. (FYI: That will be the end of the new implementation.)
Thanks Jan
Thank you. Pavel.
Dne středa 27 června 2012 13:31:42, Pavel Březina napsal(a):
On 06/27/2012 11:05 AM, Jan Zelený wrote:
Dne úterý 26 června 2012 18:22:15, Pavel Březina napsal(a):
On 06/26/2012 03:45 PM, Jan Zelený wrote:
Dne úterý 26 června 2012 10:08:15, Pavel Březina napsal(a):
On 06/21/2012 05:32 PM, Pavel Březina wrote:
On 3.6.2012 22:17, Pavel Březina wrote: > On 14.5.2012 22:37, Jakub Hrozek wrote: >> On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote: >>> On 9.5.2012 17:07, Pavel Březina wrote: >>>> On 05/07/2012 07:13 PM, Pavel Březina wrote: >>>>> On 04/23/2012 06:34 PM, Pavel Březina wrote: >>>>>> On 04/23/2012 05:04 PM, Pavel Březina wrote: >>>>>>> Hello everybody, >>>>>>> here is the first set of patches for the new sudo clothes. [1] >>>>>>> >>>>>>> As it does not touch the responder, I believe it does not >>>>>>> require >>>>>>> the >>>>>>> patches from the other preliminary sudo thread (already acked). >>>>>>> However >>>>>>> it was written atop them. >>>>>>> >>>>>>> [2/10] >>>>>>> This is the main change for the async processing of the sudo >>>>>>> rules. It >>>>>>> make sdap_sudo_refresh_send() more generic by adding there two >>>>>>> filters: >>>>>>> ldap_filter - used for search in the LDAP >>>>>>> sysdb_filter - used for a deletion of the rules from sysdb >>>>>>> >>>>>>> This way we can have many different refresh styles without >>>>>>> touching >>>>>>> this fundamental function. >>>>>>> >>>>>>> From this patch further the provider will return an error to >>>>>>> the >>>>>>> >>>>>>> responder making it unusable. >>>>>>> >>>>>>> [...] >>>>>>> The rest of the patches has a self-describing subject. Don't be >>>>>>> afraid >>>>>>> of the count, these patches are very small :-) >>>>>>> >>>>>>> The next set of patches is where the fun begin, so be patient >>>>>>> :-) >>>>>>> >>>>>>> [1] >>>>>>> https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules >>>>>> >>>>>> Ok, I made a mistake already in the 3rd patch, I'm sending it >>>>>> again :) >>>>> >>>>> Sending new set of patches if you want to take a look. >>>>> >>>>> Full periodical updates should run fine, there are preparations >>>>> for >>>>> the >>>>> smart update. I'm going to ignore modifyTimestamp for now and use >>>>> only >>>>> entryUSN if available. >>>>> >>>>> It also does not filter the rules by host, I plan to add it as >>>>> one >>>>> of >>>>> the last things. >>>> >>>> The provider part is now complete except the above given >>>> limitations. >>> >>> Ok, the major changes are done in this patch set. >>> >>> What left is: >>> - download rules per host, this only appends a filter to the search >>> so >>> it should not invoke any logic change >>> >>> - it ignores modifyTimestamp - simo suggested to use it in smart >>> refresh when enabled in configuration and USN not available, but do >>> we really want to use it? There is no other part in sssd that uses >>> it >>> (or am I wrong?). >> >> Pavel, I won't be able to review the huge patchset in a whole, so I >> started with the first couple of patches. Do you know offhand if >> there is >> a particular point where several patches could be applied and the >> provider >> would work so that when first N patches are acked, we can push them >> and >> decrease the amount of rebasing? >> >> 0001: sudo ldap provider: move async routines to sdap_async_sudo.c >> Ack, this patch just moves code around >> >> 0002: sudo ldap provider: give sdap_sudo_refresh_send() search and >> purge filters >> Ack. The sudo provider is not functional after this patch until >> patch >> #8, >> but I think that's OK. This split is as good as any. >> >> 0003: confdb: add entry_cache_sudo_timeout option >> Nack, the new option is missing from the configAPI >> Also, I assume the new option is going to be used when the responder >> changes land? So far, it's only read, not used. > > Your assumption is correct. > Done. I have also fixed it in subsequent patches. I really need to > write it somewhere on my desk :-) > >> 0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state >> Ack, but I don't get the point? >> >> 0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state >> Same as 0004, what's the intent? > > Just to shorten access to these contexts. > >> 0006: sudo ldap provider: add expiration time to each rule >> Ack >> >> 0007: sysdb_sudo_set_refresh_time >> Looks good, but do you anticipate that >> sysdb_sudo_{get,set}_refresh_time >> would be called with any other attribute name than >> SYSDB_SUDO_AT_LAST_FULL_REFRESH? > > Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I > found out later that it is not necessary. > > The intention was to use that value to compute when the first smart > refresh should be scheduled after sssd is started. However, it needs > to > know the current highest USN value (which would be a little tricky). > Therefore I'm doing full refresh at first and then enable periodical > smart updates (as we have discussed on site). > >> 0008: sudo ldap provider: provide API for full refresh >> + >> + /* free filters with the request */ >> + talloc_steal(subreq, ldap_filter); >> + talloc_steal(subreq, sysdb_filter); >> + >> >> Do the filters need to outlive the top-level request? It doesn't >> look >> like >> it's the case, but if so, then I would say it's more natural to >> strdup >> them >> in the subreq, so that other potential consumers of >> sdap_sudo_refresh_send() >> don't need to remember to steal the filters. > > Well, you don't really need to steal them. But I don't have any use > for > them outside the subreq so I think it is correct to make them > deallocate with subreq. > > Those are constant input data, so there is no need to strdup them. > >> _recv should come after _done in the source file, otherwise the code >> reads funny. > > Ok. There would be unworthy rebasing troubles, thus I have moved it > in > a new patch. > >> In sdap_sudo_full_refresh_done(), you read the req and state >> variables >> twice >> in the patch, but not in the resulting code, so I assume it was >> fixed >> in >> a follow-up patch. Is it too much trouble to fix? If it is, don't >> bother. > > This is an error from one of many rebases I have done. > Fixed. > >> I think you meant to quit the request here with an error? If not, >> please >> put a comment saying that you ignore an error intentionally: >> + if (ret != EOK) { >> + DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of " >> + "a successful full refresh\n")); >> + } > > I have added the following comment: > /* this is only a minor error that does not affect the functionality, > * therefore there is no need to report it with tevent_req_error() > * which would cause problems in the consumers */ > >> 0009: sudo ldap provider: add support for on demand full refresh >> + default: >> + DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n", >> + sudo_req->type)); >> + } >> I think you should abort the request with something like EINVAL >> here. >> I know that the code would not continue because of the following >> (req >> != NULL) >> check but the OOM error would be pretty confusing for the caller. > > Yes, definitely. The same problem was also in sdap_sudo_reply() where > ret would be uninitialized in ret != EOK condition. > Both fixed. > >> 0010: provide API for refresh of specific rules >> Same comment with stealing the filters as with patch #9. Frankly, I >> don't >> really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be >> more >> readable to create both _done and _recv? > > I found it necessary after all, because I needed to create there a > new > request. It is converted to it in one of the last patches. > >> 0011: sudo ldap provider: add support for on demand refresh of >> specific rules >> Ack >> >> I'm going to continue tomorrow. I also haven't done any testing yet, >> just >> reading the code. > > I have also fixed typo in man page and added new patch that handles > modification of the highest USN value after a specific rules refresh. > > New patches are attached. > > Thanks for the review. Keep coming :-)
Rebased on the current master and patches from "sudo: send username and uid while requesting default options" thread.
Added support for modifyTimestamp as a fallback when USN attributes are not available (squashed to #21).
And I have also noticed that 'entry_cache_sudo_timeout' broke the config test. The fix is squashed to #3.
As a general comment: please make all commit messages more verbose. They should completely describe the new functionality and explain motivation for major changes.
Ok. I often forget that you can't see in my head. Next time :-)
Also please try to merge some patches together. It is very difficult to undertand how are patches bound together and some are clearly closely bound together. I specifically pointed it out in couple places.
Patch #0001: Ack (Jakub) Patch #0002: Ack (Jakub)
Nack, I don't think it's a good idea to continue operations if strdup() on sysdb_filter failed. You should probably return with an error i such case.
Done.
Patch #0003: Ack Patch #0004: Ack (Jakub) Patch #0005: Ack (Jakub) Patch #0006: Ack (Jakub) Patch #0007: Ack Patch #0008: Nack, I don't like stealing those two variables either. The correct approach would be to strdup them in sdap_sudo_refresh_send() and free both originals instead of stealing them. Also it is better (at least for the sake of consistency) to use state as the original context instead of the req.
Done and done.
Nack, I'm sorry I didn't catch this in previous review but as I already commented in other patches, please don't free req upon an error, rather set an error and return the req. If you have suspicion you have similar code elsewhere, please change it there as well.
Done. And also fixed in other places.
Patch #0009: Ack Patch #0010: Nack, The comment about stealing filter at the end of sdap_sudo_rules_refresh_send() still stands, even after the subsequent patch.
Done.
At the end of the patch you have two constants with the same content. If you don't anticipate them to have different values, please merge them back into one.
Done.
Ack
Patch #0011: Ack Patch #0012: Nack, I don't get why you set sudo_req->rules = NULL while deleting similar statements for uid and groups. I'd prefer if you deleted this completely since the structure is zeroed anyway.
Because uid and groups are deprecated (I had to left them in the struct so it can be still compiled, but they are deleted in the clean up patch). But yes, it can be removed, so - done.
Ack
Patch #0013: Nack, I don't like skipping the first argument with dbus_message_iter_next(). Better approach would be to use the iterator right away and read the type with it as well. This will be a solution less susceptible to future errors if for some reason another argument would come right after type.
Done.
Nitpick: According to DBus documentation, the type of rule should be const char *
Done.
Ack
Patch #0014: Ack Patch #0015: Tentative Ack
Ack
Patch #0016: Nack, Please don't free req in case of error performing tevent_add_timer() in sdap_sudo_timer_send(), the practice we use is to set an error on that request and return it if possible.
Done.
Ack
Patch #0017: Tentative Ack
Ack
Patch #0018: Nack, I'm not sure what is the first part (before the "schedule" label) supposed to do exactly but I suspect those ifs are somewhat wrong. Isn't the function supposed to not schedule another round if the previous was ok? If not then I suppose you don't need them at all.
If the timer went wrong, we don't care and we will just schedule a new one. But it is missing a debug message. Added.
Ack
Patch #0019: Nack, please merge with #0018
Done.
Patch #0020: Nack, please merge this patch with #0015
Done.
This is the first logical part of SUDO patch set. I suggest we push it after it is finished. It is logically independent on the second part, the only thing missing would be documentation. Pushing this independently will make review a little bit more easy.
Thanks Jan
Thank you for the review! Corrected patches are attached.
The tar ball also contains foundations for host based filtering. It introduces several new options to configure this feature manually (hostnames, ip addresses, ...) as an addition to auto configuration which I will hopefully implement tomorrow or the day after. (FYI: That will be the end of the new implementation.)
Thanks Jan
Thank you. Pavel.
Ack to patches #0001-#0018. I haven't tested them yet though.
Thanks Jan
On 06/27/2012 02:46 PM, Jan Zelený wrote:
Dne středa 27 června 2012 13:31:42, Pavel Březina napsal(a):
On 06/27/2012 11:05 AM, Jan Zelený wrote:
Dne úterý 26 června 2012 18:22:15, Pavel Březina napsal(a):
On 06/26/2012 03:45 PM, Jan Zelený wrote:
Dne úterý 26 června 2012 10:08:15, Pavel Březina napsal(a):
On 06/21/2012 05:32 PM, Pavel Březina wrote: > On 3.6.2012 22:17, Pavel Březina wrote: >> On 14.5.2012 22:37, Jakub Hrozek wrote: >>> On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote: >>>> On 9.5.2012 17:07, Pavel Březina wrote: >>>>> On 05/07/2012 07:13 PM, Pavel Březina wrote: >>>>>> On 04/23/2012 06:34 PM, Pavel Březina wrote: >>>>>>> On 04/23/2012 05:04 PM, Pavel Březina wrote: >>>>>>>> Hello everybody, >>>>>>>> here is the first set of patches for the new sudo clothes. [1] >>>>>>>> >>>>>>>> As it does not touch the responder, I believe it does not >>>>>>>> require >>>>>>>> the >>>>>>>> patches from the other preliminary sudo thread (already acked). >>>>>>>> However >>>>>>>> it was written atop them. >>>>>>>> >>>>>>>> [2/10] >>>>>>>> This is the main change for the async processing of the sudo >>>>>>>> rules. It >>>>>>>> make sdap_sudo_refresh_send() more generic by adding there two >>>>>>>> filters: >>>>>>>> ldap_filter - used for search in the LDAP >>>>>>>> sysdb_filter - used for a deletion of the rules from sysdb >>>>>>>> >>>>>>>> This way we can have many different refresh styles without >>>>>>>> touching >>>>>>>> this fundamental function. >>>>>>>> >>>>>>>> From this patch further the provider will return an error to >>>>>>>> the >>>>>>>> >>>>>>>> responder making it unusable. >>>>>>>> >>>>>>>> [...] >>>>>>>> The rest of the patches has a self-describing subject. Don't be >>>>>>>> afraid >>>>>>>> of the count, these patches are very small :-) >>>>>>>> >>>>>>>> The next set of patches is where the fun begin, so be patient >>>>>>>> :-) >>>>>>>> >>>>>>>> [1] >>>>>>>> https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules >>>>>>> >>>>>>> Ok, I made a mistake already in the 3rd patch, I'm sending it >>>>>>> again :) >>>>>> >>>>>> Sending new set of patches if you want to take a look. >>>>>> >>>>>> Full periodical updates should run fine, there are preparations >>>>>> for >>>>>> the >>>>>> smart update. I'm going to ignore modifyTimestamp for now and use >>>>>> only >>>>>> entryUSN if available. >>>>>> >>>>>> It also does not filter the rules by host, I plan to add it as >>>>>> one >>>>>> of >>>>>> the last things. >>>>> >>>>> The provider part is now complete except the above given >>>>> limitations. >>>> >>>> Ok, the major changes are done in this patch set. >>>> >>>> What left is: >>>> - download rules per host, this only appends a filter to the search >>>> so >>>> it should not invoke any logic change >>>> >>>> - it ignores modifyTimestamp - simo suggested to use it in smart >>>> refresh when enabled in configuration and USN not available, but do >>>> we really want to use it? There is no other part in sssd that uses >>>> it >>>> (or am I wrong?). >>> >>> Pavel, I won't be able to review the huge patchset in a whole, so I >>> started with the first couple of patches. Do you know offhand if >>> there is >>> a particular point where several patches could be applied and the >>> provider >>> would work so that when first N patches are acked, we can push them >>> and >>> decrease the amount of rebasing? >>> >>> 0001: sudo ldap provider: move async routines to sdap_async_sudo.c >>> Ack, this patch just moves code around >>> >>> 0002: sudo ldap provider: give sdap_sudo_refresh_send() search and >>> purge filters >>> Ack. The sudo provider is not functional after this patch until >>> patch >>> #8, >>> but I think that's OK. This split is as good as any. >>> >>> 0003: confdb: add entry_cache_sudo_timeout option >>> Nack, the new option is missing from the configAPI >>> Also, I assume the new option is going to be used when the responder >>> changes land? So far, it's only read, not used. >> >> Your assumption is correct. >> Done. I have also fixed it in subsequent patches. I really need to >> write it somewhere on my desk :-) >> >>> 0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state >>> Ack, but I don't get the point? >>> >>> 0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state >>> Same as 0004, what's the intent? >> >> Just to shorten access to these contexts. >> >>> 0006: sudo ldap provider: add expiration time to each rule >>> Ack >>> >>> 0007: sysdb_sudo_set_refresh_time >>> Looks good, but do you anticipate that >>> sysdb_sudo_{get,set}_refresh_time >>> would be called with any other attribute name than >>> SYSDB_SUDO_AT_LAST_FULL_REFRESH? >> >> Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I >> found out later that it is not necessary. >> >> The intention was to use that value to compute when the first smart >> refresh should be scheduled after sssd is started. However, it needs >> to >> know the current highest USN value (which would be a little tricky). >> Therefore I'm doing full refresh at first and then enable periodical >> smart updates (as we have discussed on site). >> >>> 0008: sudo ldap provider: provide API for full refresh >>> + >>> + /* free filters with the request */ >>> + talloc_steal(subreq, ldap_filter); >>> + talloc_steal(subreq, sysdb_filter); >>> + >>> >>> Do the filters need to outlive the top-level request? It doesn't >>> look >>> like >>> it's the case, but if so, then I would say it's more natural to >>> strdup >>> them >>> in the subreq, so that other potential consumers of >>> sdap_sudo_refresh_send() >>> don't need to remember to steal the filters. >> >> Well, you don't really need to steal them. But I don't have any use >> for >> them outside the subreq so I think it is correct to make them >> deallocate with subreq. >> >> Those are constant input data, so there is no need to strdup them. >> >>> _recv should come after _done in the source file, otherwise the code >>> reads funny. >> >> Ok. There would be unworthy rebasing troubles, thus I have moved it >> in >> a new patch. >> >>> In sdap_sudo_full_refresh_done(), you read the req and state >>> variables >>> twice >>> in the patch, but not in the resulting code, so I assume it was >>> fixed >>> in >>> a follow-up patch. Is it too much trouble to fix? If it is, don't >>> bother. >> >> This is an error from one of many rebases I have done. >> Fixed. >> >>> I think you meant to quit the request here with an error? If not, >>> please >>> put a comment saying that you ignore an error intentionally: >>> + if (ret != EOK) { >>> + DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of " >>> + "a successful full refresh\n")); >>> + } >> >> I have added the following comment: >> /* this is only a minor error that does not affect the functionality, >> * therefore there is no need to report it with tevent_req_error() >> * which would cause problems in the consumers */ >> >>> 0009: sudo ldap provider: add support for on demand full refresh >>> + default: >>> + DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n", >>> + sudo_req->type)); >>> + } >>> I think you should abort the request with something like EINVAL >>> here. >>> I know that the code would not continue because of the following >>> (req >>> != NULL) >>> check but the OOM error would be pretty confusing for the caller. >> >> Yes, definitely. The same problem was also in sdap_sudo_reply() where >> ret would be uninitialized in ret != EOK condition. >> Both fixed. >> >>> 0010: provide API for refresh of specific rules >>> Same comment with stealing the filters as with patch #9. Frankly, I >>> don't >>> really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be >>> more >>> readable to create both _done and _recv? >> >> I found it necessary after all, because I needed to create there a >> new >> request. It is converted to it in one of the last patches. >> >>> 0011: sudo ldap provider: add support for on demand refresh of >>> specific rules >>> Ack >>> >>> I'm going to continue tomorrow. I also haven't done any testing yet, >>> just >>> reading the code. >> >> I have also fixed typo in man page and added new patch that handles >> modification of the highest USN value after a specific rules refresh. >> >> New patches are attached. >> >> Thanks for the review. Keep coming :-) > > Rebased on the current master and patches from "sudo: send username > and > uid while requesting default options" thread.
Added support for modifyTimestamp as a fallback when USN attributes are not available (squashed to #21).
And I have also noticed that 'entry_cache_sudo_timeout' broke the config test. The fix is squashed to #3.
As a general comment: please make all commit messages more verbose. They should completely describe the new functionality and explain motivation for major changes.
Ok. I often forget that you can't see in my head. Next time :-)
Also please try to merge some patches together. It is very difficult to undertand how are patches bound together and some are clearly closely bound together. I specifically pointed it out in couple places.
Patch #0001: Ack (Jakub) Patch #0002: Ack (Jakub)
Nack, I don't think it's a good idea to continue operations if strdup() on sysdb_filter failed. You should probably return with an error i such case.
Done.
Patch #0003: Ack Patch #0004: Ack (Jakub) Patch #0005: Ack (Jakub) Patch #0006: Ack (Jakub) Patch #0007: Ack Patch #0008: Nack, I don't like stealing those two variables either. The correct approach would be to strdup them in sdap_sudo_refresh_send() and free both originals instead of stealing them. Also it is better (at least for the sake of consistency) to use state as the original context instead of the req.
Done and done.
Nack, I'm sorry I didn't catch this in previous review but as I already commented in other patches, please don't free req upon an error, rather set an error and return the req. If you have suspicion you have similar code elsewhere, please change it there as well.
Done. And also fixed in other places.
Patch #0009: Ack Patch #0010: Nack, The comment about stealing filter at the end of sdap_sudo_rules_refresh_send() still stands, even after the subsequent patch.
Done.
At the end of the patch you have two constants with the same content. If you don't anticipate them to have different values, please merge them back into one.
Done.
Ack
Patch #0011: Ack Patch #0012: Nack, I don't get why you set sudo_req->rules = NULL while deleting similar statements for uid and groups. I'd prefer if you deleted this completely since the structure is zeroed anyway.
Because uid and groups are deprecated (I had to left them in the struct so it can be still compiled, but they are deleted in the clean up patch). But yes, it can be removed, so - done.
Ack
Patch #0013: Nack, I don't like skipping the first argument with dbus_message_iter_next(). Better approach would be to use the iterator right away and read the type with it as well. This will be a solution less susceptible to future errors if for some reason another argument would come right after type.
Done.
Nitpick: According to DBus documentation, the type of rule should be const char *
Done.
Ack
Patch #0014: Ack Patch #0015: Tentative Ack
Ack
Patch #0016: Nack, Please don't free req in case of error performing tevent_add_timer() in sdap_sudo_timer_send(), the practice we use is to set an error on that request and return it if possible.
Done.
Ack
Patch #0017: Tentative Ack
Ack
Patch #0018: Nack, I'm not sure what is the first part (before the "schedule" label) supposed to do exactly but I suspect those ifs are somewhat wrong. Isn't the function supposed to not schedule another round if the previous was ok? If not then I suppose you don't need them at all.
If the timer went wrong, we don't care and we will just schedule a new one. But it is missing a debug message. Added.
Ack
Patch #0019: Nack, please merge with #0018
Done.
Patch #0020: Nack, please merge this patch with #0015
Done.
This is the first logical part of SUDO patch set. I suggest we push it after it is finished. It is logically independent on the second part, the only thing missing would be documentation. Pushing this independently will make review a little bit more easy.
Thanks Jan
Thank you for the review! Corrected patches are attached.
The tar ball also contains foundations for host based filtering. It introduces several new options to configure this feature manually (hostnames, ip addresses, ...) as an addition to auto configuration which I will hopefully implement tomorrow or the day after. (FYI: That will be the end of the new implementation.)
Thanks Jan
Thank you. Pavel.
Ack to patches #0001-#0018. I haven't tested them yet though.
Thanks Jan
I have found an issue in the timer API. I did not clear timeout after the request has been completed which cause SIGABRT in talloc function. Patches are attached.
Ack to patches #0001-#0018. I haven't tested them yet though.
Thanks Jan
I have found an issue in the timer API. I did not clear timeout after the request has been completed which cause SIGABRT in talloc function. Patches are attached.
Ack to the fix, another part of review coming:
Patch #0019: Ack Patch #0020: Nack, Just for safety, please test _usn for being NULL in sdap_sudo_get_usn(). Also, as you already discovered, there will be a segfault caused by accessing el-
num_values because el is always NULL.
Patch #0021: Nack, if srv_opts is NULL, you will get segfault caused by the debug statement
Patch #0022: Ack Patch #0023: Nack, The first part of patch #0028 should be merged with this patch otherwise it breaks compilation.
Patch #0024: Tentative Ack, just one suggestion: when purging by name, I think if one fails we should just skip to the next one.
Patch #0025: Ack Patch #0026: Nack, The first part of this patch (full refresh state -> smart refresh state) should be merged with #0023.
Patch #0027: Nack, First of all, I have to repeat my previous comment about the schedule label, please add debug messages to both condition before the label.
As per our previous discussion, please extend validations related to smart vs. full timer. First scenario you specifically want to handle was when both timers are set to zero. Also consider scenario when full refresh timeout will be lower than smart refresh timeout. My guess is that full refresh timeout should be at least 50% longer than smart refresh timeout.
Patch #0028: Ack, but please add comment why the old enum is left there. Personally, I would remove it completely but I accept your reasoning about it.
Patch #0029: Nack, you don't need the second part of the filter since if something has expiration timestamp the same as current time, it is as good as expired anyway. Also one question: do you expect sysdb_get_sudo_filter() to recieve time other than current time in the future? If not then I think it is not necessary to add the argument.
Patch #0030: Ack
Thanks Jan
On 06/27/2012 04:28 PM, Jan Zelený wrote:
Ack to patches #0001-#0018. I haven't tested them yet though.
Thanks Jan
I have found an issue in the timer API. I did not clear timeout after the request has been completed which cause SIGABRT in talloc function. Patches are attached.
Ack to the fix, another part of review coming:
Patch #0019: Ack Patch #0020: Nack, Just for safety, please test _usn for being NULL in sdap_sudo_get_usn(). Also, as you already discovered, there will be a segfault caused by accessing el-
num_values because el is always NULL.
Done.
Patch #0021: Nack, if srv_opts is NULL, you will get segfault caused by the debug statement
Done.
Patch #0022: Ack Patch #0023: Nack, The first part of patch #0028 should be merged with this patch otherwise it breaks compilation.
I'm sorry, I was unable to find out what do you want me to merge. The patch #0023 compiles fine on my machine.
Patch #0024: Tentative Ack, just one suggestion: when purging by name, I think if one fails we should just skip to the next one.
Done.
Patch #0025: Ack Patch #0026: Nack, The first part of this patch (full refresh state -> smart refresh state) should be merged with #0023.
Done.
Patch #0027: Nack, First of all, I have to repeat my previous comment about the schedule label, please add debug messages to both condition before the label.
Done.
As per our previous discussion, please extend validations related to smart vs. full timer. First scenario you specifically want to handle was when both timers are set to zero. Also consider scenario when full refresh timeout will be lower than smart refresh timeout. My guess is that full refresh timeout should be at least 50% longer than smart refresh timeout.
Done. I have also updated manpage.
Patch #0028: Ack, but please add comment why the old enum is left there. Personally, I would remove it completely but I accept your reasoning about it.
Done.
Patch #0029: Nack, you don't need the second part of the filter since if something has expiration timestamp the same as current time, it is as good as expired anyway.
Done.
Also one
question: do you expect sysdb_get_sudo_filter() to recieve time other than current time in the future? If not then I think it is not necessary to add the argument.
Probably not.
Patch #0030: Ack
Thanks Jan
New patches are attached. In addition it contains support for auto-configuration of IP addresses.
The last patch is a skeleton for auto-configuration of hostnames. It currently does nothing because SSSD is missing an asynchronous API for reverse DNS lookups (or did I miss something?). I'm probably won't be able to create such API before I leave for my vacation, so either do a review for this skeleton, add the requested API or ignore this patch at all :-). The hostnames can be configured manually via ldap_sudo_hostnames so this should not block the release.
Dne čtvrtek 28 června 2012 12:42:37, Pavel Březina napsal(a):
On 06/27/2012 04:28 PM, Jan Zelený wrote:
Ack to patches #0001-#0018. I haven't tested them yet though.
Thanks Jan
I have found an issue in the timer API. I did not clear timeout after the request has been completed which cause SIGABRT in talloc function. Patches are attached.
Ack to the fix, another part of review coming:
Patch #0019: Ack Patch #0020: Nack, Just for safety, please test _usn for being NULL in sdap_sudo_get_usn(). Also, as you already discovered, there will be a segfault caused by accessing el->
num_values because el is always NULL.
Done.
Ack
Patch #0021: Nack, if srv_opts is NULL, you will get segfault caused by the debug statement
Done.
Ack although I'm not completely sure if the debug level is correct when srv_opts == NULL. If you think it is then I'm ok with that.
Patch #0022: Ack Patch #0023: Nack, The first part of patch #0028 should be merged with this patch otherwise it breaks compilation.
I'm sorry, I was unable to find out what do you want me to merge. The patch #0023 compiles fine on my machine.
Sorry, I thought #0026, you have done the merge.
Ack
Patch #0024: Tentative Ack, just one suggestion: when purging by name, I think if one fails we should just skip to the next one.
Done.
Ack
Patch #0025: Ack Patch #0026: Nack, The first part of this patch (full refresh state -> smart refresh state) should be merged with #0023.
Done.
Ack,
Patch #0027: Nack, First of all, I have to repeat my previous comment about the schedule label, please add debug messages to both condition before the label.
Done.
As per our previous discussion, please extend validations related to smart vs. full timer. First scenario you specifically want to handle was when both timers are set to zero. Also consider scenario when full refresh timeout will be lower than smart refresh timeout. My guess is that full refresh timeout should be at least 50% longer than smart refresh timeout.
Done. I have also updated manpage.
Ack
Patch #0028: Ack, but please add comment why the old enum is left there. Personally, I would remove it completely but I accept your reasoning about it.
Done.
Ack, but please change "breaked" in commit message to "broken"
Patch #0029: Nack, you don't need the second part of the filter since if something has expiration timestamp the same as current time, it is as good as expired anyway.
Done.
Also one
question: do you expect sysdb_get_sudo_filter() to recieve time other than current time in the future? If not then I think it is not necessary to add the argument.
Probably not.
Ok, would you then remove the "now" argument? Other than that the patch is fine.
Patch #0030: Ack
Thanks Jan
New patches are attached. In addition it contains support for auto-configuration of IP addresses.
The last patch is a skeleton for auto-configuration of hostnames. It currently does nothing because SSSD is missing an asynchronous API for reverse DNS lookups (or did I miss something?). I'm probably won't be able to create such API before I leave for my vacation, so either do a review for this skeleton, add the requested API or ignore this patch at all :-). The hostnames can be configured manually via ldap_sudo_hostnames so this should not block the release.
I'm also attaching another part of the review:
Patch #0031: Ack Patch #0032: Nack, please change the big comment as follows:
+ /* fetch only expired rules + * this is because sudo ask sssd two times - for defaults and for rules + * when we refresh all expired rules (of this user) and defaults at once, + * we will safe one provider call + */
+ /* Fetch all expired rules: + * sudo asks sssd twice - for defaults and for rules. If we refresh all expired + * rules for this user and defaults at once we will save one provider call + */
You seem to have wrong memory hierarchy if expired_rules_num > 0 in sudosrv_get_rules(). First of all you don't free tmp_ctx anywhere. You also allocate cb_ctx on top of tmp_ctx. I guess the return EAGAIN messed with your code flow because at the end of function you have steal statement for the cb_ctx and free for the tmp_ctx. This issue aside, are you certain that you want to alloc cb_ctx on top of cli_ctx? Right now I'm not sure how long does that context stay. If it is only for a limited amount of time (like just until the result is returned to the client), it's ok.
Patch #0033: Ack Patch #0034: Nack this patch desperately needs better commit message because I don't think the main point of the patch is what the message declares.
You don't need talloc_array_length() to tell you the size of rules array in sdap_sudo_rules_refresh_send(). If you use value if i right after the for cycle earlier, it will do the trick I think.
Since you use the label immediately in sdap_sudo_rules_refresh_send() only for errors, I suggest following modification:
+ tevent_req_set_callback(subreq, sdap_sudo_rules_refresh_done, req); + +immediately: talloc_free(tmp_ctx); + + if (ret != EOK) { + tevent_req_error(req, ret); + tevent_req_post(req, be_ctx->ev); + } + return req;
In sdap_sudo_rules_refresh_done() you should use sdap_sudo_refresh_recv() with state->dp_error and state->error instead of dp_error and error respectively. Otherwise the information won't be received by recv function in case of error. Also doing this change will make the assignment couple lines below redundant.
Beware of two stray tabs you have in sdap_sudo_rules_refresh_recv().
Patch #0035: Ack Patch #0036: Ack Patch #0037: TODO Stephen, please ask him to do the review Patch #0038: Nack Beware of the tab in sdap_sudo_rules_refresh_state declaration. Other than that the patch is fine.
Patch #0039: Nack beware of tabs in sdap_sudo_rules_refresh_recv(). Other than that the patch is fine, just moves stuff around
Patch #0040: Ack Patch #0041: Ack Just a suggestion: would it be possible to shorten those description string in __init__.py.in file?
Patch #0042: Full Nack I don't get why sdap_sudo_get_hostinfo_send() creates and returns new tevent_req. It is a synchronous function, please treat it so. Since this will be major change, another full review of this patch will be necessary.
Are you sure about your statement that updates of specific rules will work if sdap_sudo_setup_periodical_refresh() failed? I highly doubt that and I't rather end with an error. Perhaps you can distinguish situations when it will fail and when not but I'm not sure it will be possible for all situations.
Just a thought here but how about giving sudo_ctx directly as an argument to ldap_get_sudo_options() instead of adding those last three.
Patch #0043: Ack Patch #0044: Nack If you don't have any objections, I'd like you to merge sdap_sudo_build_host_filter() and sdap_sudo_get_filter() into one, called for example sdap_sudo_get_full_filter(), since the latter one is only a wrapper with one additional operation performed. Not to mention two levels of tmp_ctx basically for nothing. Other than that the patch is fine.
Patch #0045: TODO Stephen, please ask him to do the review. There are some trailing whitespace warnings
Patch #0046: since it is marked as incomplete I recommend taking it out of current patch set and work on it separately.
Thanks Jan
On 28.6.2012 16:28, Jan Zelený wrote:
Dne čtvrtek 28 června 2012 12:42:37, Pavel Březina napsal(a):
On 06/27/2012 04:28 PM, Jan Zelený wrote:
Ack to patches #0001-#0018. I haven't tested them yet though.
Thanks Jan
I have found an issue in the timer API. I did not clear timeout after the request has been completed which cause SIGABRT in talloc function. Patches are attached.
Ack to the fix, another part of review coming:
Patch #0019: Ack Patch #0020: Nack, Just for safety, please test _usn for being NULL in sdap_sudo_get_usn(). Also, as you already discovered, there will be a segfault caused by accessing el->
num_values because el is always NULL.
Done.
Ack
Patch #0021: Nack, if srv_opts is NULL, you will get segfault caused by the debug statement
Done.
Ack although I'm not completely sure if the debug level is correct when srv_opts == NULL. If you think it is then I'm ok with that.
Patch #0022: Ack Patch #0023: Nack, The first part of patch #0028 should be merged with this patch otherwise it breaks compilation.
I'm sorry, I was unable to find out what do you want me to merge. The patch #0023 compiles fine on my machine.
Sorry, I thought #0026, you have done the merge.
Ack
Patch #0024: Tentative Ack, just one suggestion: when purging by name, I think if one fails we should just skip to the next one.
Done.
Ack
Patch #0025: Ack Patch #0026: Nack, The first part of this patch (full refresh state -> smart refresh state) should be merged with #0023.
Done.
Ack,
Patch #0027: Nack, First of all, I have to repeat my previous comment about the schedule label, please add debug messages to both condition before the label.
Done.
As per our previous discussion, please extend validations related to smart vs. full timer. First scenario you specifically want to handle was when both timers are set to zero. Also consider scenario when full refresh timeout will be lower than smart refresh timeout. My guess is that full refresh timeout should be at least 50% longer than smart refresh timeout.
Done. I have also updated manpage.
Ack
Patch #0028: Ack, but please add comment why the old enum is left there. Personally, I would remove it completely but I accept your reasoning about it.
Done.
Ack, but please change "breaked" in commit message to "broken"
Done.
Patch #0029: Nack, you don't need the second part of the filter since if something has expiration timestamp the same as current time, it is as good as expired anyway.
Done.
Also one
question: do you expect sysdb_get_sudo_filter() to recieve time other than current time in the future? If not then I think it is not necessary to add the argument.
Probably not.
Ok, would you then remove the "now" argument? Other than that the patch is fine.
Done.
Patch #0030: Ack
Thanks Jan
New patches are attached. In addition it contains support for auto-configuration of IP addresses.
The last patch is a skeleton for auto-configuration of hostnames. It currently does nothing because SSSD is missing an asynchronous API for reverse DNS lookups (or did I miss something?). I'm probably won't be able to create such API before I leave for my vacation, so either do a review for this skeleton, add the requested API or ignore this patch at all :-). The hostnames can be configured manually via ldap_sudo_hostnames so this should not block the release.
I'm also attaching another part of the review:
Patch #0031: Ack Patch #0032: Nack, please change the big comment as follows:
/* fetch only expired rules
* this is because sudo ask sssd two times - for defaults and for rules* when we refresh all expired rules (of this user) and defaults at once,* we will safe one provider call*//* Fetch all expired rules:
* sudo asks sssd twice - for defaults and for rules. If we refresh allexpired
* rules for this user and defaults at once we will save one providercall
*/
Done.
You seem to have wrong memory hierarchy if expired_rules_num> 0 in sudosrv_get_rules(). First of all you don't free tmp_ctx anywhere. You also allocate cb_ctx on top of tmp_ctx. I guess the return EAGAIN messed with your code flow because at the end of function you have steal statement for the cb_ctx and free for the tmp_ctx. This issue aside, are you certain that you want to alloc cb_ctx on top of cli_ctx? Right now I'm not sure how long does that context stay. If it is only for a limited amount of time (like just until the result is returned to the client), it's ok.
Fixed.
cli_ctx is a context of the current query from a client to a responder, thus it stays as long as the connection is alive (which should be a very short time).
cb_ctx is attached to cmd_ctx so I assume you meant dpreq wich is attached to cli_ctx.
I think it can be attached to cmd_ctx as well. Hovewer, this part of code was originally written by Jakub so maybe there is some purpose of using cli_ctx instead which I don't see at the moment.
cmd_ctx and cli_ctx should have the same lifetime so it is not a big deal either way.
Patch #0033: Ack Patch #0034: Nack this patch desperately needs better commit message because I don't think the main point of the patch is what the message declares.
Done.
You don't need talloc_array_length() to tell you the size of rules array in sdap_sudo_rules_refresh_send(). If you use value if i right after the for cycle earlier, it will do the trick I think.
Done.
Since you use the label immediately in sdap_sudo_rules_refresh_send() only for errors, I suggest following modification:
- tevent_req_set_callback(subreq, sdap_sudo_rules_refresh_done, req);
+immediately: talloc_free(tmp_ctx);
- if (ret != EOK) {
tevent_req_error(req, ret);tevent_req_post(req, be_ctx->ev);- }
return req;
Done.
In sdap_sudo_rules_refresh_done() you should use sdap_sudo_refresh_recv() with state->dp_error and state->error instead of dp_error and error respectively. Otherwise the information won't be received by recv function in case of error. Also doing this change will make the assignment couple lines below redundant.
Done.
Beware of two stray tabs you have in sdap_sudo_rules_refresh_recv().
Done.
Patch #0035: Ack Patch #0036: Ack Patch #0037: TODO Stephen, please ask him to do the review
I believe this is a manpage update which I have moved to the end after adding some more options. It has number #45 in patch set from 28. 6.
The following patches are moved by one. I have added the current patch numbers.
Patch #0038: Nack (In tar from 28. 6. #37) Beware of the tab in sdap_sudo_rules_refresh_state declaration. Other than that the patch is fine.
Done.
Patch #0039: Nack (In tar from 28. 6. #38) beware of tabs in sdap_sudo_rules_refresh_recv(). Other than that the patch is fine, just moves stuff around
Done.
Patch #0040: Ack (In tar from 28. 6. #39) Patch #0041: Ack (In tar from 28. 6. #40) Just a suggestion: would it be possible to shorten those description string in __init__.py.in file?
If you have any specific suggestion, just say it. I have fixed there at least a typo.
Patch #0042: Full Nack (In tar from 28. 6. #41) I don't get why sdap_sudo_get_hostinfo_send() creates and returns new tevent_req. It is a synchronous function, please treat it so. Since this will be major change, another full review of this patch will be necessary.
As it is said in the commit message, it will not stay synchronous for a long time. Knowing this, I have implemented it in tevent style from the beginning to avoid later reimplementation.
Are you sure about your statement that updates of specific rules will work if sdap_sudo_setup_periodical_refresh() failed? I highly doubt that and I't rather end with an error. Perhaps you can distinguish situations when it will fail and when not but I'm not sure it will be possible for all situations.
Are you pointing at specific line? Once the provider is initialized the specific-rule update should be functional, the code should not depend on the periodical updates. If you see such dependency, please tell me.
Just a thought here but how about giving sudo_ctx directly as an argument to ldap_get_sudo_options() instead of adding those last three.
ldap_get_sudo_options() is located in ldap_common.c - we can't afford to use any sudo specific symbol in this file because sudo is build conditionally.
I was thinking about moving this function to sdap_sudo.c, but Jakub already did several relocations of similar functions and left this function in ldap_common.c. I wanted to ask him why he decide to leave this function on its place before it is moved.
Patch #0043: Ack (In tar from 28. 6. #42) Patch #0044: Nack (In tar from 28. 6. #43) If you don't have any objections, I'd like you to merge sdap_sudo_build_host_filter() and sdap_sudo_get_filter() into one, called for example sdap_sudo_get_full_filter(), since the latter one is only a wrapper with one additional operation performed. Not to mention two levels of tmp_ctx basically for nothing. Other than that the patch is fine.
I personally like it more this way. So if you don't have a strong opinion on that, I would leave it separated.
Patch #44 from 28. 6. is missing here.
Patch #0045: TODO Stephen, please ask him to do the review. There are some trailing whitespace warnings
Warnings removed.
Patch #0046: since it is marked as incomplete I recommend taking it out of current patch set and work on it separately.
Ok. But you can see there why I made #41 in tevent_req.
Thank you for the review. Great job!
Patches #0001 - #0031:
Ack, without further comments
Patch #0032: Nack, please change the big comment as follows:
- /* fetch only expired rules
* this is because sudo ask sssd two times - for defaults and forrules + * when we refresh all expired rules (of this user) and defaults at once, + * we will safe one provider call
*//* Fetch all expired rules:
* sudo asks sssd twice - for defaults and for rules. If we refreshall expired
* rules for this user and defaults at once we will save one providercall
*/Done.
You seem to have wrong memory hierarchy if expired_rules_num> 0 in sudosrv_get_rules(). First of all you don't free tmp_ctx anywhere. You also allocate cb_ctx on top of tmp_ctx. I guess the return EAGAIN messed with your code flow because at the end of function you have steal statement for the cb_ctx and free for the tmp_ctx. This issue aside, are you certain that you want to alloc cb_ctx on top of cli_ctx? Right now I'm not sure how long does that context stay. If it is only for a limited amount of time (like just until the result is returned to the client), it's ok.
Fixed.
cli_ctx is a context of the current query from a client to a responder, thus it stays as long as the connection is alive (which should be a very short time).
cb_ctx is attached to cmd_ctx so I assume you meant dpreq wich is attached to cli_ctx.
I think it can be attached to cmd_ctx as well. Hovewer, this part of code was originally written by Jakub so maybe there is some purpose of using cli_ctx instead which I don't see at the moment.
cmd_ctx and cli_ctx should have the same lifetime so it is not a big deal either way.
Ack, thanks for the explanation.
Patch #0033: Ack Patch #0034: Nack this patch desperately needs better commit message because I don't think the main point of the patch is what the message declares.
Done.
I made some modifications to the commit message so it is better understandable: ============== sudo ldap provider: notify responder when an expired rule has been deleted
When an expired rule is not present on the server server during specific rule refresh, the provider will notify the sudo responder that it has been deleted.
Because there is a high probability that some other rules were deleted from the server as well, we want to remove them from sysdb as soon as possible.
Once the responder is notified, it will schedule an out of band full refresh. This is issued by responder, because we already have a mechanism that prohibits creation of similar request (i.e. once the OOB full refresh is scheduled, there won't be another).
The notification is done by returning: DP error = DP_ERR_OK, error = ENOENT
You don't need talloc_array_length() to tell you the size of rules array in sdap_sudo_rules_refresh_send(). If you use value if i right after the for cycle earlier, it will do the trick I think.
Done.
I should have been more specific about the state->num_rules. I meant that you should use i but if I understand the code correctly, you should substract 1 from it. Otherwise you would say that the NULL at the end of array belongs to it as well.
Patch #0035: Ack Patch #0036: Ack Patch #0038: Nack (In tar from 28. 6. #37) Beware of the tab in sdap_sudo_rules_refresh_state declaration. Other than that the patch is fine.
Done.
Ack, Currently patch #0037
Patch #0039: Nack (In tar from 28. 6. #38) beware of tabs in sdap_sudo_rules_refresh_recv(). Other than that the patch is fine, just moves stuff around
Done.
Ack, Currently patch #0038
Patch #0040: Ack (In tar from 28. 6. #39) Patch #0041: Ack (In tar from 28. 6. #40) Just a suggestion: would it be possible to shorten those description string in __init__.py.in file?
If you have any specific suggestion, just say it. I have fixed there at least a typo.
Nothing specific, never mind.
Ack, Currently patch #0040
Patch #0042: Full Nack (In tar from 28. 6. #41) I don't get why sdap_sudo_get_hostinfo_send() creates and returns new tevent_req. It is a synchronous function, please treat it so. Since this will be major change, another full review of this patch will be necessary.
As it is said in the commit message, it will not stay synchronous for a long time. Knowing this, I have implemented it in tevent style from the beginning to avoid later reimplementation.
For the record I still don't like this approach. IMO it should be implemented synchronous in this patch and then modified in the later patch.
Are you sure about your statement that updates of specific rules will work if sdap_sudo_setup_periodical_refresh() failed? I highly doubt that and I't rather end with an error. Perhaps you can distinguish situations when it will fail and when not but I'm not sure it will be possible for all situations.
Are you pointing at specific line? Once the provider is initialized the specific-rule update should be functional, the code should not depend on the periodical updates. If you see such dependency, please tell me.
My point was that if you fail for example with ENOMEM, you probably won't recover from it that easily. See lines 234 and 235 of the patch.
Just a thought here but how about giving sudo_ctx directly as an argument to ldap_get_sudo_options() instead of adding those last three.
ldap_get_sudo_options() is located in ldap_common.c - we can't afford to use any sudo specific symbol in this file because sudo is build conditionally.
I was thinking about moving this function to sdap_sudo.c, but Jakub already did several relocations of similar functions and left this function in ldap_common.c. I wanted to ask him why he decide to leave this function on its place before it is moved.
Sounds good. Will you be able to keep this on your radar so we can get back to this later?
Patch #0043: Ack (In tar from 28. 6. #42)
Ack, Currently patch #0042
Patch #0044: Nack (In tar from 28. 6. #43) If you don't have any objections, I'd like you to merge sdap_sudo_build_host_filter() and sdap_sudo_get_filter() into one, called for example sdap_sudo_get_full_filter(), since the latter one is only a wrapper with one additional operation performed. Not to mention two levels of tmp_ctx basically for nothing. Other than that the patch is fine.
I personally like it more this way. So if you don't have a strong opinion on that, I would leave it separated.
Well, I don't like it but it's not a deal breaker for me.
Patch #44 from 28. 6. is missing here.
I'm still working on this patch but I don't think anything will be wrong with it.
Patch #0045: TODO Stephen, please ask him to do the review. There are some trailing whitespace warnings
Warnings removed.
Patch #0046: since it is marked as incomplete I recommend taking it out of current patch set and work on it separately.
Ok. But you can see there why I made #41 in tevent_req.
Thank you for the review. Great job!
Thanks Jan
On 29.6.2012 14:31, Jan Zelený wrote:
Patches #0001 - #0031:
Ack, without further comments
Patch #0032: Nack, please change the big comment as follows:
- /* fetch only expired rules
* this is because sudo ask sssd two times - for defaults and forrules + * when we refresh all expired rules (of this user) and defaults at once, + * we will safe one provider call
*//* Fetch all expired rules:
* sudo asks sssd twice - for defaults and for rules. If we refreshall expired
* rules for this user and defaults at once we will save one providercall
*/Done.
You seem to have wrong memory hierarchy if expired_rules_num> 0 in sudosrv_get_rules(). First of all you don't free tmp_ctx anywhere. You also allocate cb_ctx on top of tmp_ctx. I guess the return EAGAIN messed with your code flow because at the end of function you have steal statement for the cb_ctx and free for the tmp_ctx. This issue aside, are you certain that you want to alloc cb_ctx on top of cli_ctx? Right now I'm not sure how long does that context stay. If it is only for a limited amount of time (like just until the result is returned to the client), it's ok.
Fixed.
cli_ctx is a context of the current query from a client to a responder, thus it stays as long as the connection is alive (which should be a very short time).
cb_ctx is attached to cmd_ctx so I assume you meant dpreq wich is attached to cli_ctx.
I think it can be attached to cmd_ctx as well. Hovewer, this part of code was originally written by Jakub so maybe there is some purpose of using cli_ctx instead which I don't see at the moment.
cmd_ctx and cli_ctx should have the same lifetime so it is not a big deal either way.
Ack, thanks for the explanation.
Patch #0033: Ack Patch #0034: Nack this patch desperately needs better commit message because I don't think the main point of the patch is what the message declares.
Done.
I made some modifications to the commit message so it is better understandable:
sudo ldap provider: notify responder when an expired rule has been deleted
When an expired rule is not present on the server server during specific rule refresh, the provider will notify the sudo responder that it has been deleted.
Because there is a high probability that some other rules were deleted from the server as well, we want to remove them from sysdb as soon as possible.
Once the responder is notified, it will schedule an out of band full refresh. This is issued by responder, because we already have a mechanism that prohibits creation of similar request (i.e. once the OOB full refresh is scheduled, there won't be another).
The notification is done by returning: DP error = DP_ERR_OK, error = ENOENT
Thanks. Done.
You don't need talloc_array_length() to tell you the size of rules array in sdap_sudo_rules_refresh_send(). If you use value if i right after the for cycle earlier, it will do the trick I think.
Done.
I should have been more specific about the state->num_rules. I meant that you should use i but if I understand the code correctly, you should substract 1 from it. Otherwise you would say that the NULL at the end of array belongs to it as well.
Thanks to the zero-based indexing I don't have to subtract anything.
Patch #0035: Ack Patch #0036: Ack Patch #0038: Nack (In tar from 28. 6. #37) Beware of the tab in sdap_sudo_rules_refresh_state declaration. Other than that the patch is fine.
Done.
Ack, Currently patch #0037
Patch #0039: Nack (In tar from 28. 6. #38) beware of tabs in sdap_sudo_rules_refresh_recv(). Other than that the patch is fine, just moves stuff around
Done.
Ack, Currently patch #0038
Patch #0040: Ack (In tar from 28. 6. #39) Patch #0041: Ack (In tar from 28. 6. #40) Just a suggestion: would it be possible to shorten those description string in __init__.py.in file?
If you have any specific suggestion, just say it. I have fixed there at least a typo.
Nothing specific, never mind.
Ack, Currently patch #0040
Patch #0042: Full Nack (In tar from 28. 6. #41) I don't get why sdap_sudo_get_hostinfo_send() creates and returns new tevent_req. It is a synchronous function, please treat it so. Since this will be major change, another full review of this patch will be necessary.
As it is said in the commit message, it will not stay synchronous for a long time. Knowing this, I have implemented it in tevent style from the beginning to avoid later reimplementation.
For the record I still don't like this approach. IMO it should be implemented synchronous in this patch and then modified in the later patch.
Are you sure about your statement that updates of specific rules will work if sdap_sudo_setup_periodical_refresh() failed? I highly doubt that and I't rather end with an error. Perhaps you can distinguish situations when it will fail and when not but I'm not sure it will be possible for all situations.
Are you pointing at specific line? Once the provider is initialized the specific-rule update should be functional, the code should not depend on the periodical updates. If you see such dependency, please tell me.
My point was that if you fail for example with ENOMEM, you probably won't recover from it that easily. See lines 234 and 235 of the patch.
Well, even if ENOMEM occurs at this point, everything the provider needs for its initialization is already allocated. So the provider can finish the initialization no matter what. The first rule-specific refresh may occur hours later (when sudo is called), when there is already enough memory available.
I noticed here that I forgot to disable host filter if sdap_sudo_get_hostinfo_send() == NULL. So I added after the DEBUG statement: sudo_ctx->use_host_filter = false;
Just a thought here but how about giving sudo_ctx directly as an argument to ldap_get_sudo_options() instead of adding those last three.
ldap_get_sudo_options() is located in ldap_common.c - we can't afford to use any sudo specific symbol in this file because sudo is build conditionally.
I was thinking about moving this function to sdap_sudo.c, but Jakub already did several relocations of similar functions and left this function in ldap_common.c. I wanted to ask him why he decide to leave this function on its place before it is moved.
Sounds good. Will you be able to keep this on your radar so we can get back to this later?
Certainly.
Patch #0043: Ack (In tar from 28. 6. #42)
Ack, Currently patch #0042
Patch #0044: Nack (In tar from 28. 6. #43) If you don't have any objections, I'd like you to merge sdap_sudo_build_host_filter() and sdap_sudo_get_filter() into one, called for example sdap_sudo_get_full_filter(), since the latter one is only a wrapper with one additional operation performed. Not to mention two levels of tmp_ctx basically for nothing. Other than that the patch is fine.
I personally like it more this way. So if you don't have a strong opinion on that, I would leave it separated.
Well, I don't like it but it's not a deal breaker for me.
Patch #44 from 28. 6. is missing here.
I'm still working on this patch but I don't think anything will be wrong with it.
Patch #0045: TODO Stephen, please ask him to do the review. There are some trailing whitespace warnings
Warnings removed.
Patch #0046: since it is marked as incomplete I recommend taking it out of current patch set and work on it separately.
Ok. But you can see there why I made #41 in tevent_req.
Thank you for the review. Great job!
Patches attached.
On 29.6.2012 14:31, Jan Zelený wrote:
Patches #0001 - #0031:
Ack, without further comments
Patch #0032: Nack, please change the big comment as follows:
- /* fetch only expired rules
* this is because sudo ask sssd two times - for defaults and forrules + * when we refresh all expired rules (of this user) and defaults at once, + * we will safe one provider call
*//* Fetch all expired rules:
* sudo asks sssd twice - for defaults and for rules. If we refreshall expired
* rules for this user and defaults at once we will save one providercall
*/Done.
You seem to have wrong memory hierarchy if expired_rules_num> 0 in sudosrv_get_rules(). First of all you don't free tmp_ctx anywhere. You also allocate cb_ctx on top of tmp_ctx. I guess the return EAGAIN messed with your code flow because at the end of function you have steal statement for the cb_ctx and free for the tmp_ctx. This issue aside, are you certain that you want to alloc cb_ctx on top of cli_ctx? Right now I'm not sure how long does that context stay. If it is only for a limited amount of time (like just until the result is returned to the client), it's ok.
Fixed.
cli_ctx is a context of the current query from a client to a responder, thus it stays as long as the connection is alive (which should be a very short time).
cb_ctx is attached to cmd_ctx so I assume you meant dpreq wich is attached to cli_ctx.
I think it can be attached to cmd_ctx as well. Hovewer, this part of code was originally written by Jakub so maybe there is some purpose of using cli_ctx instead which I don't see at the moment.
cmd_ctx and cli_ctx should have the same lifetime so it is not a big deal either way.
Ack, thanks for the explanation.
Patch #0033: Ack Patch #0034: Nack this patch desperately needs better commit message because I don't think the main point of the patch is what the message declares.
Done.
I made some modifications to the commit message so it is better understandable:
sudo ldap provider: notify responder when an expired rule has been deleted
When an expired rule is not present on the server server during specific rule refresh, the provider will notify the sudo responder that it has been deleted.
Because there is a high probability that some other rules were deleted from the server as well, we want to remove them from sysdb as soon as possible.
Once the responder is notified, it will schedule an out of band full refresh. This is issued by responder, because we already have a mechanism that prohibits creation of similar request (i.e. once the OOB full refresh is scheduled, there won't be another).
The notification is done by returning: DP error = DP_ERR_OK, error = ENOENT
Thanks. Done.
You don't need talloc_array_length() to tell you the size of rules array in sdap_sudo_rules_refresh_send(). If you use value if i right after the for cycle earlier, it will do the trick I think.
Done.
I should have been more specific about the state->num_rules. I meant that you should use i but if I understand the code correctly, you should substract 1 from it. Otherwise you would say that the NULL at the end of array belongs to it as well.
Thanks to the zero-based indexing I don't have to subtract anything.
Patch #0035: Ack Patch #0036: Ack Patch #0038: Nack (In tar from 28. 6. #37) Beware of the tab in sdap_sudo_rules_refresh_state declaration. Other than that the patch is fine.
Done.
Ack, Currently patch #0037
Patch #0039: Nack (In tar from 28. 6. #38) beware of tabs in sdap_sudo_rules_refresh_recv(). Other than that the patch is fine, just moves stuff around
Done.
Ack, Currently patch #0038
Patch #0040: Ack (In tar from 28. 6. #39) Patch #0041: Ack (In tar from 28. 6. #40) Just a suggestion: would it be possible to shorten those description string in __init__.py.in file?
If you have any specific suggestion, just say it. I have fixed there at least a typo.
Nothing specific, never mind.
Ack, Currently patch #0040
Patch #0042: Full Nack (In tar from 28. 6. #41) I don't get why sdap_sudo_get_hostinfo_send() creates and returns new tevent_req. It is a synchronous function, please treat it so. Since this will be major change, another full review of this patch will be necessary.
As it is said in the commit message, it will not stay synchronous for a long time. Knowing this, I have implemented it in tevent style from the beginning to avoid later reimplementation.
For the record I still don't like this approach. IMO it should be implemented synchronous in this patch and then modified in the later patch.
Are you sure about your statement that updates of specific rules will work if sdap_sudo_setup_periodical_refresh() failed? I highly doubt that and I't rather end with an error. Perhaps you can distinguish situations when it will fail and when not but I'm not sure it will be possible for all situations.
Are you pointing at specific line? Once the provider is initialized the specific-rule update should be functional, the code should not depend on the periodical updates. If you see such dependency, please tell me.
My point was that if you fail for example with ENOMEM, you probably won't recover from it that easily. See lines 234 and 235 of the patch.
Well, even if ENOMEM occurs at this point, everything the provider needs for its initialization is already allocated. So the provider can finish the initialization no matter what. The first rule-specific refresh may occur hours later (when sudo is called), when there is already enough memory available.
I noticed here that I forgot to disable host filter if sdap_sudo_get_hostinfo_send() == NULL. So I added after the DEBUG statement: sudo_ctx->use_host_filter = false;
Just a thought here but how about giving sudo_ctx directly as an argument to ldap_get_sudo_options() instead of adding those last three.
ldap_get_sudo_options() is located in ldap_common.c - we can't afford to use any sudo specific symbol in this file because sudo is build conditionally.
I was thinking about moving this function to sdap_sudo.c, but Jakub already did several relocations of similar functions and left this function in ldap_common.c. I wanted to ask him why he decide to leave this function on its place before it is moved.
Sounds good. Will you be able to keep this on your radar so we can get back to this later?
Certainly.
Patch #0043: Ack (In tar from 28. 6. #42)
Ack, Currently patch #0042
Patch #0044: Nack (In tar from 28. 6. #43) If you don't have any objections, I'd like you to merge sdap_sudo_build_host_filter() and sdap_sudo_get_filter() into one, called for example sdap_sudo_get_full_filter(), since the latter one is only a wrapper with one additional operation performed. Not to mention two levels of tmp_ctx basically for nothing. Other than that the patch is fine.
I personally like it more this way. So if you don't have a strong opinion on that, I would leave it separated.
Well, I don't like it but it's not a deal breaker for me.
Patch #44 from 28. 6. is missing here.
I'm still working on this patch but I don't think anything will be wrong with it.
Patch #0045: TODO Stephen, please ask him to do the review. There are some trailing whitespace warnings
Warnings removed.
Patch #0046: since it is marked as incomplete I recommend taking it out of current patch set and work on it separately.
Ok. But you can see there why I made #41 in tevent_req.
Thank you for the review. Great job!
Patches attached.
Dne pátek 29 června 2012 15:33:14, Pavel Březina napsal(a):
On 29.6.2012 14:31, Jan Zelený wrote:
Patches #0001 - #0031:
Ack, without further comments
Patch #0032: Nack, please change the big comment as follows:
- /* fetch only expired rules
* this is because sudo ask sssd two times - for defaults and forrules + * when we refresh all expired rules (of this user) and defaults at once, + * we will safe one provider call
*//* Fetch all expired rules:
* sudo asks sssd twice - for defaults and for rules. If we refreshall expired
* rules for this user and defaults at once we will save oneprovider call
*/Done.
You seem to have wrong memory hierarchy if expired_rules_num> 0 in sudosrv_get_rules(). First of all you don't free tmp_ctx anywhere. You also allocate cb_ctx on top of tmp_ctx. I guess the return EAGAIN messed with your code flow because at the end of function you have steal statement for the cb_ctx and free for the tmp_ctx. This issue aside, are you certain that you want to alloc cb_ctx on top of cli_ctx? Right now I'm not sure how long does that context stay. If it is only for a limited amount of time (like just until the result is returned to the client), it's ok.
Fixed.
cli_ctx is a context of the current query from a client to a responder, thus it stays as long as the connection is alive (which should be a very short time).
cb_ctx is attached to cmd_ctx so I assume you meant dpreq wich is attached to cli_ctx.
I think it can be attached to cmd_ctx as well. Hovewer, this part of code was originally written by Jakub so maybe there is some purpose of using cli_ctx instead which I don't see at the moment.
cmd_ctx and cli_ctx should have the same lifetime so it is not a big deal either way.
Ack, thanks for the explanation.
Patch #0033: Ack Patch #0034: Nack this patch desperately needs better commit message because I don't think the main point of the patch is what the message declares.
Done.
I made some modifications to the commit message so it is better understandable: ============== sudo ldap provider: notify responder when an expired rule has been deleted
When an expired rule is not present on the server server during specific rule refresh, the provider will notify the sudo responder that it has been deleted.
Because there is a high probability that some other rules were deleted from the server as well, we want to remove them from sysdb as soon as possible.
Once the responder is notified, it will schedule an out of band full refresh. This is issued by responder, because we already have a mechanism that prohibits creation of similar request (i.e. once the OOB full refresh is scheduled, there won't be another).
The notification is done by returning: DP error = DP_ERR_OK, error = ENOENT
Thanks. Done.
You don't need talloc_array_length() to tell you the size of rules array in sdap_sudo_rules_refresh_send(). If you use value if i right after the for cycle earlier, it will do the trick I think.
Done.
I should have been more specific about the state->num_rules. I meant that you should use i but if I understand the code correctly, you should substract 1 from it. Otherwise you would say that the NULL at the end of array belongs to it as well.
Thanks to the zero-based indexing I don't have to subtract anything.
I'm sorry, probably Friday afternoon is getting to me. You are indeed right.
Patch #0035: Ack Patch #0036: Ack Patch #0038: Nack (In tar from 28. 6. #37) Beware of the tab in sdap_sudo_rules_refresh_state declaration. Other than that the patch is fine.
Done.
Ack, Currently patch #0037
Patch #0039: Nack (In tar from 28. 6. #38) beware of tabs in sdap_sudo_rules_refresh_recv(). Other than that the patch is fine, just moves stuff around
Done.
Ack, Currently patch #0038
Patch #0040: Ack (In tar from 28. 6. #39) Patch #0041: Ack (In tar from 28. 6. #40) Just a suggestion: would it be possible to shorten those description string in __init__.py.in file?
If you have any specific suggestion, just say it. I have fixed there at least a typo.
Nothing specific, never mind.
Ack, Currently patch #0040
Patch #0042: Full Nack (In tar from 28. 6. #41) I don't get why sdap_sudo_get_hostinfo_send() creates and returns new tevent_req. It is a synchronous function, please treat it so. Since this will be major change, another full review of this patch will be necessary.
As it is said in the commit message, it will not stay synchronous for a long time. Knowing this, I have implemented it in tevent style from the beginning to avoid later reimplementation.
For the record I still don't like this approach. IMO it should be implemented synchronous in this patch and then modified in the later patch.
Are you sure about your statement that updates of specific rules will work if sdap_sudo_setup_periodical_refresh() failed? I highly doubt that and I't rather end with an error. Perhaps you can distinguish situations when it will fail and when not but I'm not sure it will be possible for all situations.
Are you pointing at specific line? Once the provider is initialized the specific-rule update should be functional, the code should not depend on the periodical updates. If you see such dependency, please tell me.
My point was that if you fail for example with ENOMEM, you probably won't recover from it that easily. See lines 234 and 235 of the patch.
Well, even if ENOMEM occurs at this point, everything the provider needs for its initialization is already allocated. So the provider can finish the initialization no matter what. The first rule-specific refresh may occur hours later (when sudo is called), when there is already enough memory available.
I noticed here that I forgot to disable host filter if sdap_sudo_get_hostinfo_send() == NULL. So I added after the DEBUG statement: sudo_ctx->use_host_filter = false;
Ok, I accept your reasoning on this one.
Just a thought here but how about giving sudo_ctx directly as an argument to ldap_get_sudo_options() instead of adding those last three.
ldap_get_sudo_options() is located in ldap_common.c - we can't afford to use any sudo specific symbol in this file because sudo is build conditionally.
I was thinking about moving this function to sdap_sudo.c, but Jakub already did several relocations of similar functions and left this function in ldap_common.c. I wanted to ask him why he decide to leave this function on its place before it is moved.
Sounds good. Will you be able to keep this on your radar so we can get back to this later?
Certainly.
Patch #0043: Ack (In tar from 28. 6. #42)
Ack, Currently patch #0042
Patch #0044: Nack (In tar from 28. 6. #43) If you don't have any objections, I'd like you to merge sdap_sudo_build_host_filter() and sdap_sudo_get_filter() into one, called for example sdap_sudo_get_full_filter(), since the latter one is only a wrapper with one additional operation performed. Not to mention two levels of tmp_ctx basically for nothing. Other than that the patch is fine.
I personally like it more this way. So if you don't have a strong opinion on that, I would leave it separated.
Well, I don't like it but it's not a deal breaker for me.
Patch #44 from 28. 6. is missing here.
I'm still working on this patch but I don't think anything will be wrong with it.
Patch #0045: TODO Stephen, please ask him to do the review. There are some trailing whitespace warnings
Warnings removed.
Patch #0046: since it is marked as incomplete I recommend taking it out of current patch set and work on it separately.
Ok. But you can see there why I made #41 in tevent_req.
Thank you for the review. Great job!
Patches attached.
Ack to all patches except for the man page which I'd like to leave for native English speaker to check it.
Also, please note that I didn't have time to test everything working together so some testing should be performed before pushing.
Great job Pavel!
Thanks Jan
On Fri, 2012-06-29 at 16:01 +0200, Jan Zelený wrote:
Ack to all patches except for the man page which I'd like to leave for native English speaker to check it.
Also, please note that I didn't have time to test everything working together so some testing should be performed before pushing.
Great job Pavel!
Yes, great job indeed!
I've pushed these patches to master, with some minor grammatical fixes to the manpages.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Pavel Březina -
Simo Sorce -
Stephen Gallagher