[SSSD] NSS: disable midpoint refresh for netgroups if ptask refresh is enabled

Pavel Březina pbrezina at redhat.com
Thu Feb 27 10:55:18 UTC 2014


On 02/26/2014 06:35 PM, Pavel Reichl wrote:
> Hello,
>
> please see attached patches.
>
> patch #1 - disable midpoint refresh for netgroups if ptask refresh is
> enabled

Nack.
It is still possible having a netgroup expired if 
refresh_expired_interval is misconfigured (> entry_cache_timeout). Thus 
you still need to check if the netgroup is valid, maybe throw a visible 
warning if it is expired and periodic refresh is enabled.

> The rest of patches solves some minor problems that occurred while I
> was working on 1st patch:
>
> patch #2 - fixes sysdb_getnetgr to return ENOENT as is as is expected in
> code

This is not necessary. ENOENT is returned from ldb_search(). Or did you 
manage to get EOK and count==0 somehow?

> patch #3 - first check return value then access output parameter

Tentatively ack. The change looks correct and it will not change any 
behaviour, but I couldn't understand code flow in this function in a 
short time - hence tentatively.

Also there are many ways how to leak memory of 'name'. Can you fix it as 
well?

> patch #4 - some minor code style improvements, some lines over 80
> columns, IMO strange indentation of string constants - feel free to
> NACK.

There will be lots of places in the code like this one:
          DEBUG(SSSDBG_TRACE_LIBS, "netgroup [%s] was already removed\n",
-                                  netgr->name);
+              netgr->name);

It is due to the recent change where we removed parentheses around 
message and its arguments. Aligning format parameters with message was 
more natural when it was closed in parentheses, but IMHO it is still 
better to keep it this way.

-            DEBUG(SSSDBG_OP_FAILURE, "No results for netgroup %s 
(domain %s)\n",
-                      name, dom->name);
+            DEBUG(SSSDBG_OP_FAILURE,
+                  "No results for netgroup %s (domain %s)\n",
+                  name, dom->name);

I personally prefer to keep debug statements on as fewer lines as 
possible, like this:

+            DEBUG(SSSDBG_OP_FAILURE, "No results for netgroup "
+                  "%s (domain %s)\n", name, dom->name);

Many of us uses different approach how to wrap debug statements. I think 
we should agree on one style and make it part of our coding style.

I'm leaving this patch open for now -- it is not nack nor ack.

> Bye,
>
> Pavel Reichl




More information about the sssd-devel mailing list