On 3.5.2012 13:48, Jakub Hrozek wrote:
On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
The subdomains patches messed with one line which was causing troubles.
Here are the rebased patches.
On 04/23/2012 12:51 PM, Pavel Březina wrote:
On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1239
[PATCH 1/2]
Finally removes EOK constant from sudo api header. It is not used in
the SUDO code so it does not require their changes.
Looks good to me, it'w weird to use EOK in an external header (even
though it is correctly defined).
(Note: this patch shouldn't be pushed even though I don't have any
comments. It is an ABI break and we need to coordinate with Daniel K.)
[PATCH 2/2]
This does what is requested in the ticket. It seems to be very huge
but
in fact it is mainly changing the variable. Basically I tried to get
rid of domain ctx where possible, leave it only in initgroups part and
use command ctx elsewhere.
Still, it is hard to review the huge patch. Can you split it into
smaller ones? What about creating one that removes the duplication
between _get_sudorules and _get_defaults, one that converts to using
cmdctx and one that adds the uid support?
The patches are attached.
I want to get those patches acked because I need to deliver the updated
interface to Dan so he can update the sudo binary.
It does not implement the in-memory cache (yet? :-)) due to the
discussion with simo.
The patches expects "sudo api: check sss_status instead of errnop in
sss_sudo_send_recv_generic()" (already acked on the list and waiting
to be pushed).
Patch 0001: sudo api: remove EOK
Ack
Patch 0002: sudo responder: remove code duplication in commands
I think it would be a good idea to also check if rawname[rawname_len] ==
'\0' when parsing username, this would ensure that all the string checks
work OK.
Done.
Now that several sudosrv_response_append_* functions were removed from
header, can you make them static? In particular
sudosrv_response_append_string(), sudosrv_response_append_uint32(),
sudosrv_response_append_rule(), sudosrv_response_append_attr().
Done.
Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more configurable
Ack
Patch 0004: sudo api: send uid, username and domainname
Please get rid of the C++ comments in sudosrv_cmd(). If you need to
leave code in but disabled, please use #if 0/#endif
Thanks. I'm using keybord shortcut to comment out whole block with
these slashes and I fortgot that we have this rule :)
The whole hunk that
changes sudosrv_cmd() seems like it belongs to patch #2.
Not really. It requires the new protocol which is implemented in this
patch. It makes the two command more similar than they was before.
There's another // comment in sudosrv_get_sudorules_from_cache.
The logic looks good to me, though.
Thank you for the review.
I'm attaching a 6th patch that removes all code related to the
in-memory cache as it will not be used anymore.
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Ack
Patch 0004: Ack
Patch 0005: Ack
Patch 0006: Nack, does to apply on master:
$ git reset --hard origin/master
HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
$ git am *.patch
Applying: sudo api: remove EOK
Applying: sudo responder: remove code duplication in commands
Applying: sudo responder: get rid of dctx where possible
Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
Applying: sudo api: send uid, username and domainname
Applying: sudo responder: discard in-memory cache
error: patch failed: src/responder/sudo/sudosrv_cache.c:1
I'm sorry, but it works for me. Are you sure you have the correct
patches? What exactly fails to merge?
$ git reset --hard origin/master
HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
$ git am *.patch
Applying: sudo api: remove EOK
Applying: sudo responder: remove code duplication in commands
Applying: sudo responder: get rid of dctx where possible
Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
Applying: sudo api: send uid, username and domainname
Applying: sudo responder: discard in-memory cache
$
Even when the patches are acked, they shouldn't be pushed right?
The patches work so they can be pushed IMHO. And I will use them as a
starting point for the new design. [1]
The only problem is that Daniel did not find the time yet to change the
sudo sources. So we have no consumer that would understand the new
protocol.
No, that would break anyone that wants to use git HEAD with sudo that is
currently available. I think we have two options:
1) when the modified sudo binary is available, simply state that whoever
wants to use git HEAD with sudo, must use version xy of sudo. We
could also import sudo into our nightly repo for the time being.
2) Modify the code so that it accepts *both* protocols.
2) is the strongly preferred solution, btw
It would be possible to put the logic back. Though I wouldn't do that
in this case because this is not an extension or a fix of a simple bug.
The old protocol may mix the results from two different domains in a
multi-domain environment, thus it may be better to force the update of
the sudo.
Pavel.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
