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.