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

Pavel Březina pbrezina at redhat.com
Mon Nov 3 14:18:49 UTC 2014


On 10/31/2014 03:02 PM, Pavel Reichl wrote:
>
> 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...

The benefit is when you add a new value to the enum and forget to extend 
the switch, compiler will produce a warning. But given the use case and 
near future of this refresh, I don't really mind using default statement 
here.

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

Ack.




More information about the sssd-devel mailing list