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