On Tue, Jun 26, 2012 at 10:02:32AM +0200, Pavel Březina wrote:
On 06/21/2012 05:29 PM, Pavel Březina wrote:
>On 17.5.2012 13:03, Pavel Březina wrote:
>>On 14.5.2012 18:37, Jakub Hrozek wrote:
>>>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?
>>
>>Patches are attached.
>>
>>>>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.
>
>Rebased on the current master.
>
>The sudo client is finally ready:
>http://koji.fedoraproject.org/koji/rpminfo?rpmID=3115173
>
>I will test it tomorrow.
The sudo client has some problems with undefined symbols while
dlopen(sudoers.so). Daniel should look at it today.
I'm sending rebased patches one more time.
For the record, these patches were included in another thread, which
superseded this one.