[SSSD] [PATCH] NSS: disable midpoint refresh for netgroups

Jakub Hrozek jhrozek at redhat.com
Tue Nov 4 16:42:32 UTC 2014


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



More information about the sssd-devel mailing list