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

Pavel Březina pbrezina at redhat.com
Fri Jun 29 13:33:14 UTC 2012


On 29.6.2012 14:31, Jan Zelený wrote:
> Patches #0001 - #0031:
>
> Ack, without further comments
>
>>> 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.
>
> Ack, thanks for the explanation.
>
>>> 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.
>
> I made some modifications to the commit message so it is better understandable:
> ==============
> sudo ldap provider: notify responder when an expired rule has been deleted
>
> When an expired rule is not present on the server server during specific rule
> refresh, the provider will notify the sudo responder that it has been deleted.
>
> Because there is a high probability that some other rules were deleted from
> the server as well, we want to remove them from sysdb as soon as possible.
>
> Once the responder is notified, it will schedule an out of band full refresh.
> This is issued by responder, because we already have a mechanism that
> prohibits creation of similar request (i.e. once the OOB full refresh is
> scheduled, there won't be another).
>
> The notification is done by returning:
> DP error = DP_ERR_OK, error = ENOENT

Thanks. 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.
>
> I should have been more specific about the state->num_rules. I meant that you
> should use i but if I understand the code correctly, you should substract 1
> from it. Otherwise you would say that the NULL at the end of array belongs to
> it as well.

Thanks to the zero-based indexing I don't have to subtract anything.

>>> Patch #0035: Ack
>>> Patch #0036: Ack
>>> 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.
>
> Ack,
> Currently patch #0037
>
>>> 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.
>
> Ack,
> Currently patch #0038
>
>>> 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.
>
> Nothing specific, never mind.
>
> Ack,
> Currently patch #0040
>
>>> 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.
>
> For the record I still don't like this approach. IMO it should be implemented
> synchronous in this patch and then modified in the later patch.
>
>>> 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.
>
> My point was that if you fail for example with ENOMEM, you probably won't
> recover from it that easily. See lines 234 and 235 of the patch.

Well, even if ENOMEM occurs at this point, everything the provider
needs for its initialization is already allocated. So the provider can
finish the initialization no matter what. The first rule-specific
refresh may occur hours later (when sudo is called), when there is
already enough memory available.

I noticed here that I forgot to disable host filter if
sdap_sudo_get_hostinfo_send() == NULL. So I added after the DEBUG
statement:
sudo_ctx->use_host_filter = false;

>>> 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.
>
> Sounds good. Will you be able to keep this on your radar so we can get back to
> this later?

Certainly.

>>> Patch #0043: Ack (In tar from 28. 6. #42)
>
> Ack,
> Currently patch #0042
>
>>> 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.
>
> Well, I don't like it but it's not a deal breaker for me.
>
>> Patch #44 from 28. 6. is missing here.
>
> I'm still working on this patch but I don't think anything will be wrong with
> it.
>
>>
>>> 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!

Patches attached.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tgz
Type: application/x-compressed-tar
Size: 57642 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120629/e5b8bb64/attachment.bin>


More information about the sssd-devel mailing list