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
> 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.
> 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.
> 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?
> 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!
Thanks
Jan