On Mon, Nov 03, 2014 at 03:18:49PM +0100, Pavel Březina wrote:
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.
* master:
ad132722d6f3393ae1e6d720a222a0f880f2ea54
f933190722886ff23eab8148b473915908bc8c23