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.