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
> "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