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.