On 05/03/2012 10:30 PM, Pavel Březina wrote:
> 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.
There was one big mistake in the fifth patch.
I was passing wrong uid information into query_cache function.