[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