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.
+/* 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) ...
"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.
But those are just minor nitpicks, otherwise it looks good. I like the
simplicity.