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.
>
> Even when the patches are acked, they shouldn't be pushed right?
The patches work so they can be pushed IMHO. And I will use them as a
starting point for the new design. [1]
The only problem is that Daniel did not find the time yet to change the
sudo sources. So we have no consumer that would understand the new
protocol.
[1]
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel