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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel