On Mon, May 14, 2012 at 05:55:09PM +0200, Pavel Březina wrote:
> On 11.5.2012 12:23, Pavel Březina wrote:
>> On 05/07/2012 07:17 PM, Pavel Březina wrote:
>>> 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.
>>
Thank you, I'm not sure which one of the changes did it, but I'm able to
apply the patches now.
>> There was one big mistake in the fifth patch.
>>
>> I was passing wrong uid information into query_cache function.
>
> I have added a patch that increases protocol number. If client uses
> old protocol, responder will return error immediately.
>
Sorry, one last change..can you change --version to --version-info and
bump the "current" number in the version string to 1?
> I know the sudo part is not yet ready. But I would like to get
these
> patches pushed - I don't want to rebase them again :) It does not
> break sudo until sss is present in nsswitch.conf.
FWIW, I don't like breaking running systems. Yes, there is probably very
few people who use sudo and sssd and would be affected by this, but
still..
I don't have any issues with the code, I'll ack it when the version is
bumped.