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

Pavel Březina pbrezina at redhat.com
Thu Jun 28 10:42:37 UTC 2012


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.

>
> Patch #0021: Nack,
> if srv_opts is NULL, you will get segfault caused by the debug statement

Done.

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

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

> Patch #0025: Ack
> Patch #0026: Nack,
> The first part of this patch (full refresh state ->  smart refresh state) should
> be merged with #0023.

Done.

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

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

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

> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tgz
Type: application/x-compressed-tar
Size: 58617 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120628/8fe2748d/attachment.bin>


More information about the sssd-devel mailing list