[SSSD] [PATCHES][PRELIMINARY] sudo - new design

Pavel Březina pbrezina at redhat.com
Thu Jun 28 18:10:08 UTC 2012


On 28.6.2012 16:28, Jan Zelený wrote:
> Dne čtvrtek 28 června 2012 12:42:37, Pavel Březina napsal(a):
>> On 06/27/2012 04:28 PM, Jan Zelený wrote:
>>>>> Ack to patches #0001-#0018. I haven't tested them yet though.
>>>>>
>>>>> Thanks
>>>>> Jan
>>>>
>>>> I have found an issue in the timer API. I did not clear timeout after
>>>> the request has been completed which cause SIGABRT in talloc function.
>>>> Patches are attached.
>>>
>>> Ack to the fix, another part of review coming:
>>>
>>> Patch #0019: Ack
>>> Patch #0020: Nack,
>>> Just for safety, please test _usn for being NULL in sdap_sudo_get_usn().
>>> Also, as you already discovered, there will be a segfault caused by
>>> accessing el->
>>>> num_values because el is always NULL.
>>
>> Done.
>
> Ack
>
>>> Patch #0021: Nack,
>>> if srv_opts is NULL, you will get segfault caused by the debug statement
>>
>> Done.
>
> Ack
> although I'm not completely sure if the debug level is correct when srv_opts
> == NULL. If you think it is then I'm ok with that.
>
>>> Patch #0022: Ack
>>> Patch #0023: Nack,
>>> The first part of patch #0028 should be merged with this patch otherwise
>>> it
>>> breaks compilation.
>>
>> I'm sorry, I was unable to find out what do you want me to merge. The
>> patch #0023 compiles fine on my machine.
>
> Sorry, I thought #0026, you have done the merge.
>
> Ack
>
>>> Patch #0024: Tentative Ack,
>>> just one suggestion: when purging by name, I think if one fails we should
>>> just skip to the next one.
>>
>> Done.
>
> Ack
>
>>> Patch #0025: Ack
>>> Patch #0026: Nack,
>>> The first part of this patch (full refresh state ->   smart refresh state)
>>> should be merged with #0023.
>>
>> Done.
>
> Ack,
>
>>> Patch #0027: Nack,
>>> First of all, I have to repeat my previous comment about the schedule
>>> label, please add debug messages to both condition before the label.
>>
>> Done.
>>
>>> As per our previous discussion, please extend validations related to smart
>>> vs. full timer. First scenario you specifically want to handle was when
>>> both timers are set to zero. Also consider scenario when full refresh
>>> timeout will be lower than smart refresh timeout. My guess is that full
>>> refresh timeout should be at least 50% longer than smart refresh timeout.
>>
>> Done.
>> I have also updated manpage.
>
> Ack
>
>>> Patch #0028: Ack,
>>> but please add comment why the old enum is left there. Personally, I would
>>> remove it completely but I accept your reasoning about it.
>>
>> Done.
>
> Ack,
> but please change "breaked" in commit message to "broken"

Done.

>>> Patch #0029: Nack,
>>> you don't need the second part of the filter since if something has
>>> expiration timestamp the same as current time, it is as good as expired
>>> anyway.
>> Done.
>>
>> Also one
>>
>>> question: do you expect sysdb_get_sudo_filter() to recieve time other than
>>> current time in the future? If not then I think it is not necessary to add
>>> the argument.
>>
>> Probably not.
>
> Ok, would you then remove the "now" argument? Other than that the patch is
> fine.

Done.

>>> Patch #0030: Ack
>>>
>>>
>>> Thanks
>>> Jan
>>
>> New patches are attached.
>> In addition it contains support for auto-configuration of IP addresses.
>>
>> The last patch is a skeleton for auto-configuration of hostnames. It
>> currently does nothing because SSSD is missing an asynchronous API for
>> reverse DNS lookups (or did I miss something?). I'm probably won't be
>> able to create such API before I leave for my vacation, so either do a
>> review for this skeleton, add the requested API or ignore this patch at
>> all :-). The hostnames can be configured manually via
>> ldap_sudo_hostnames so this should not block the release.
>
>
> I'm also attaching another part of the review:
>
> Patch #0031: Ack
> Patch #0032:  Nack,
> please change the big comment as follows:
>
> +    /* fetch only expired rules
> +     * this is because sudo ask sssd two times - for defaults and for rules
> +     * when we refresh all expired rules (of this user) and defaults at once,
> +     * we will safe one provider call
> +     */
>
> +    /* Fetch all expired rules:
> +     * sudo asks sssd twice - for defaults and for rules. If we refresh all
> expired
> +     * rules for this user and defaults at once we will save one provider
> call
> +     */

Done.

> You seem to have wrong memory hierarchy if expired_rules_num>  0 in
> sudosrv_get_rules(). First of all you don't free tmp_ctx anywhere. You also
> allocate cb_ctx on top of tmp_ctx. I guess the return EAGAIN messed with your
> code flow because at the end of function you have steal statement for the
> cb_ctx and free for the tmp_ctx. This issue aside, are you certain that you
> want to alloc cb_ctx on top of cli_ctx? Right now I'm not sure how long does
> that context stay. If it is only for a limited amount of time (like just until
> the result is returned to the client), it's ok.

Fixed.

cli_ctx is a context of the current query from a client to a responder,
thus it stays as long as the connection is alive (which should be a
very short time).

cb_ctx is attached to cmd_ctx so I assume you meant dpreq wich is
attached to cli_ctx.

I think it can be attached to cmd_ctx as well. Hovewer, this part of
code was originally written by Jakub so maybe there is some purpose of
using cli_ctx instead which I don't see at the moment.

cmd_ctx and cli_ctx should have the same lifetime so it is not a big
deal either way.

> Patch #0033: Ack
> Patch #0034: Nack
> this patch desperately needs better commit message because I don't think the
> main point of the patch is what the message declares.

Done.

> You don't need talloc_array_length() to tell you the size of rules array in
> sdap_sudo_rules_refresh_send(). If you use value if i right after the for
> cycle earlier, it will do the trick I think.

Done.

> Since you use the label immediately in sdap_sudo_rules_refresh_send() only for
> errors, I suggest following modification:
>
> +    tevent_req_set_callback(subreq, sdap_sudo_rules_refresh_done, req);
> +
> +immediately:
>       talloc_free(tmp_ctx);
> +
> +    if (ret != EOK) {
> +        tevent_req_error(req, ret);
> +        tevent_req_post(req, be_ctx->ev);
> +    }
> +
>       return req;

Done.

>
> In sdap_sudo_rules_refresh_done() you should use sdap_sudo_refresh_recv() with
> state->dp_error and state->error instead of dp_error and error respectively.
> Otherwise the information won't be received by recv function in case of error.
> Also doing this change will make the assignment couple lines below redundant.

Done.

> Beware of two stray tabs you have in sdap_sudo_rules_refresh_recv().

Done.

> Patch #0035: Ack
> Patch #0036: Ack
> Patch #0037: TODO Stephen, please ask him to do the review

I believe this is a manpage update which I have moved to the end after
adding some more options. It has number #45 in patch set from 28. 6.

The following patches are moved by one. I have added the current patch
numbers.

> Patch #0038: Nack (In tar from 28. 6. #37)
> Beware of the tab in sdap_sudo_rules_refresh_state declaration. Other than
> that the patch is fine.

Done.

> Patch #0039: Nack (In tar from 28. 6. #38)
> beware of tabs in sdap_sudo_rules_refresh_recv(). Other than that the patch is
> fine, just moves stuff around

Done.

> Patch #0040: Ack  (In tar from 28. 6. #39)
> Patch #0041: Ack (In tar from 28. 6. #40)
> Just a suggestion: would it be possible to shorten those description string in
> __init__.py.in file?

If you have any specific suggestion, just say it.
I have fixed there at least a typo.

> Patch #0042: Full Nack (In tar from 28. 6. #41)
> I don't get why sdap_sudo_get_hostinfo_send() creates and returns new
> tevent_req. It is a synchronous function, please treat it so. Since this will
> be major change, another full review of this patch will be necessary.

As it is said in the commit message, it will not stay synchronous for a
long time. Knowing this, I have implemented it in tevent style from the
beginning to avoid later reimplementation.

> Are you sure about your statement that updates of specific rules will work if
> sdap_sudo_setup_periodical_refresh() failed? I highly doubt that and I't
> rather end with an error. Perhaps you can distinguish situations when it will
> fail and when not but I'm not sure it will be possible for all situations.

Are you pointing at specific line? Once the provider is initialized the
specific-rule update should be functional, the code should not depend
on the periodical updates. If you see such dependency, please tell me.

> Just a thought here but how about giving sudo_ctx directly as an argument to
> ldap_get_sudo_options() instead of adding those last three.

ldap_get_sudo_options() is located in ldap_common.c - we can't afford
to use any sudo specific symbol in this file because sudo is build
conditionally.

I was thinking about moving this function to sdap_sudo.c, but Jakub
already did several relocations of similar functions and left this
function in ldap_common.c. I wanted to ask him why he decide to leave
this function on its place before it is moved.

> Patch #0043: Ack (In tar from 28. 6. #42)
> Patch #0044: Nack (In tar from 28. 6. #43)
> If you don't have any objections, I'd like you to merge
> sdap_sudo_build_host_filter() and sdap_sudo_get_filter() into one, called for
> example sdap_sudo_get_full_filter(), since the latter one is only a wrapper
> with one additional operation performed. Not to mention two levels of tmp_ctx
> basically for nothing. Other than that the patch is fine.

I personally like it more this way. So if you don't have a strong
opinion on that, I would leave it separated.


Patch #44 from 28. 6. is missing here.

> Patch #0045: TODO Stephen, please ask him to do the review. There are some
> trailing whitespace warnings

Warnings removed.

>
> Patch #0046: since it is marked as incomplete I recommend taking it out of
> current patch set and work on it separately.

Ok. But you can see there why I made #41 in tevent_req.

Thank you for the review. Great job!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tgz
Type: application/x-compressed-tar
Size: 58758 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120628/77271455/attachment.bin>


More information about the sssd-devel mailing list