[PATCH] NSS: disable midpoint refresh for netgroups

[Pavel Reichl]

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

[Pavel Reichl]

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..
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Priority of the ticket this patch was written for was bumped to critical.

Please consider reviewing it soon!

Thanks.

[Jakub Hrozek]

On Thu, Oct 23, 2014 at 10:19:35AM +0200, Pavel Reichl wrote:
> Priority of the ticket this patch was written for was bumped to critical.
>
> Please consider reviewing it soon!

Pavel Brezina was recently looking into this area of code, I think he's
in best position to review changes to check_cache() now (also, chances
are, he'll have to rebase atop this change)

[Pavel Březina]

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.

[Pavel Reichl]

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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

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;
}

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.

[Pavel Reichl]

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...
> 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.
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

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.

[Jakub Hrozek]

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