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

Jan Zelený jzeleny at redhat.com
Fri Jun 29 14:01:29 UTC 2012


Dne pátek 29 června 2012 15:33:14, Pavel Březina napsal(a):
> 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.

I'm sorry, probably Friday afternoon is getting to me. You are indeed right.

> >>> 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;

Ok, I accept your reasoning on this one.

> >>> 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.

Ack to all patches except for the man page which I'd like to leave for native 
English speaker to check it.

Also, please note that I didn't have time to test everything working together 
so some testing should be performed before pushing.

Great job Pavel!

Thanks
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120629/b194b98b/attachment.sig>


More information about the sssd-devel mailing list