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