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!