[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