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
$
I'm sending it again formatted via git format-patch -M -C --patience
--full-index and tarred.