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