[SSSD] [PATCH] NSS: disable midpoint refresh for netgroups

Pavel Reichl preichl at redhat.com
Fri Oct 31 14:02:32 UTC 2014


On 10/31/2014 02:44 PM, Pavel Březina wrote:
> On 10/30/2014 05:12 PM, Pavel Reichl wrote:
>>
>> On 10/30/2014 04:18 PM, Pavel Březina wrote:
>>> On 10/01/2014 03:19 PM, Pavel Reichl wrote:
>>>> Hello,
>>>>
>>>> please see attached patch.
>>>>
>>>> This patch is updated version of patch which was introduced in now
>>>> stalled thread: NSS: disable midpoint refresh for netgroups if ptask
>>>> refresh is enabled
>>>>
>>>> I have picked some (from my POV) important comments from the previous
>>>> thread and pasted them here.
>>>>
>>>> Thanks!
>>>>
>>>> On Mon, 2014-04-07 at 19:07 +0200, Jakub Hrozek wrote:
>>>>
>>>>> What I meant is that this addition:
>>>>> +        if (req_type == SSS_DP_NETGR &&
>>>>> +            dctx->domain->refresh_expired_interval != 0) {
>>>>> +            ret = EOK;
>>>>>
>>>>> Makes sense as well to me, because this way neither midpoint refresh
>>>>> nor
>>>>> expired record request would be scheduled. What I'm not sure about
>>>>> is --
>>>>> would we expand this condition in the future when we add more
>>>>> background
>>>>> refresh types?
>>>>
>>>> Jakub Hrozek wrote:
>>>>
>>>>> >>> So how about checking that:
>>>>> >>>
>>>>> >>>    refresh_expired_interval < entry_cache_timeout
>>>>> >>
>>>>> >> Are you proposing to abort startup in this case? That sounds 
>>>>> like a
>>>>> bit
>>>>> >> too heavy solution..
>>>>> >
>>>>> > No, I would rather log loudly about this problem and then I 
>>>>> would set
>>>>> > refresh_expired_interval to entry_cache_timeout * 3/4 as is
>>>>> advised in
>>>>> > man pages. And again I would log this action loudly.
>>>>>
>>>>> That would work for me. I don't think we should over-engineer the
>>>>> solution, after all, the goal was to make it clear to the admin that
>>>>> they need to tune the config themselves..
>>>
>>> Hi,
>>>
>>> can you please move the first part of the patch that detects
>>> misconfiguration to a separate commit? It is not really related to the
>>> ticket nor the rest of the changes.
>> Sure.
>>>
>>>> +/* Currently only refreshing expired netgroups is supported. */
>>>> +bool is_refreshed_on_bg(int req_type, uint32_t
>>>> refresh_expired_interval)
>>>> +{
>>>> +    if (req_type == SSS_DP_NETGR && refresh_expired_interval != 0) {
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>
>>> It doesn't really matter since there is only one supported req_type
>>> now, but I'd like to see it more like this:
>>>
>>> if (refresh_expired_interval == 0) return false
>>> switch (req_type) ...
>> OK, I'm not sure it looks better now, but we can always change it
>> if/when we add another req_type
>
> Hi, sorry, maybe I should have been probably more specific.
>
> req_type is an enum type, so please use 'enum sss_dp_acct_type' as the 
> parameter type instead of int. Then we will have compiler check that 
> all values are considered in switch. What I wanted to see is:
>
> switch (req_type) {
>   case SSS_DP_NETGR:
>     return true;
>   case SSS_DP*: /* all false cases */
>     return false;
> }
OK, new patches attached. I hope you don't mine I use 'default' for all 
values except for SSS_DP_NETGR. I really don't see any benefit in 
listing all the values...
>
> But maybe you are right that it is not necessary at this time, since 
> we don't plan to expand this type of refresh to other objects in near 
> future. I will leave it up to your decision whether to use proper 
> switch or just the simple condition from the first version.
>
>>>
>>>>                "getpwXXX call returned more than one result!"
>>>> -                  " DB Corrupted?\n");
>>>> +              " DB Corrupted?\n");
>>>>          return ENOENT;
>>>
>>> This debug message can actually fit onto one line only.
>> Fixed.
>>>
>>> But those are just minor nitpicks, otherwise it looks good. I like the
>>> simplicity.
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-CONFDB-Detect-fix-misconf-opt-refresh_expired_interv.patch
Type: text/x-patch
Size: 1551 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141031/5160bdec/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-NSS-disable-midpoint-refresh-for-netgroups.patch
Type: text/x-patch
Size: 3832 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141031/5160bdec/attachment-0001.bin>


More information about the sssd-devel mailing list