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

Jan Zelený jzeleny at redhat.com
Thu Jun 28 14:28:44 UTC 2012


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"

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

> > 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
+     */

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.

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.

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.

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;

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.

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


Patch #0035: Ack
Patch #0036: Ack
Patch #0037: TODO Stephen, please ask him to do the review
Patch #0038: Nack
Beware of the tab in sdap_sudo_rules_refresh_state declaration. Other than 
that the patch is fine.

Patch #0039: Nack
beware of tabs in sdap_sudo_rules_refresh_recv(). Other than that the patch is 
fine, just moves stuff around

Patch #0040: Ack
Patch #0041: Ack
Just a suggestion: would it be possible to shorten those description string in 
__init__.py.in file?

Patch #0042: Full Nack
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.

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.

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.

Patch #0043: Ack
Patch #0044: Nack
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.

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

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

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/20120628/bb95cbc8/attachment.sig>


More information about the sssd-devel mailing list