On Thu, Apr 17, 2014 at 07:01:35PM +0200, Pavel Reichl wrote:
Hello,
while working on
https://fedorahosted.org/sssd/ticket/2102 I noticed a
few minor problems regarding netgroups.
1st patch:
First check returned value then it is safe to access results.
2nd patch:
It is my opinion that function lookup_netgr_step is too long and can be
safely (with minimum of changes) divided into 2 separate functions.
3rd patch:
Pavel Brezina asked me to solve memory leak in lookup_netgr_step.
4th patch:
Fixes two code style issues.
Thank for review!
Pavel Reichl
From d8f7815a69b676c65da189d01a999f2acfc7deec Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Wed, 26 Feb 2014 16:58:24 +0000
Subject: [PATCH 2/5] NSS: sysdb_getnetgr check return value first
Output parameter was accessed before return value was checked.
ACK, nice catch!
From da8ce47f42c6e551432a23ff813c938af6c5721a Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 17 Apr 2014 16:14:11 +0000
Subject: [PATCH 3/5] NSS: sysdb_getnetgr refactor
+ ret = create_dummy_netgr(step_ctx);
+ if (ret != EOK) {
+ return ret;
I think there is a change in behaviour. Previously, if creating the
negative entry failed, we would have just ignored the failure, the new
code errors out. I think the failure can be ignored, because at worst,
there would be a slowdown at the next lookup.
From 85a1744b803e9d25a82f6ce83a9eaf3d9fdffe16 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 17 Apr 2014 16:52:43 +0000
Subject: [PATCH 4/5] NSS: fix memory leak in sysdb_getnetgr
I'm not opposed to pushing this patch, but in general I think 'leaks'
like this are not a big issue. The string was allocated on top of a
per-request context, so when the request finishes, the string would be
freed together with step_ctx.
Such growths are much more dangerous in long-running requests like
nested group resolution, the frontend is usually quite fast and
efficient.
From eab0ebf66cdc977656ef8964c7546f54212a08dd Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Thu, 17 Apr 2014 17:02:31 +0000
Subject: [PATCH 5/5] NSS: minor code style improvements
@@ -644,10 +644,8 @@ static void lookup_netgr_dp_callback(uint16_t err_maj, uint32_t
err_min,
/* ok the backend returned, search to see if we have updated results */
ret = lookup_netgr_step(step_ctx);
- if (ret != EOK) {
- if (ret == EAGAIN) {
- return;
- }
+ if (ret == EAGAIN) {
+ return;
}
/* We have results to return */
This patch is OK, but I wonder if you could also refactor the result
handling. Currently nss_setent_notify_error() is used also for EOK which
might be a bit confusing in comparison with tevent where tevent_req_done !=
tevent_req_error(EOK). I think this would be more readable:
if (ret == EOK) {
setent_notify_done();
} else {
nss_setent_notify_error();
}