[SSSD] [sssd][patches] NSS: netgroups minor fixes

Michal Židek mzidek at redhat.com
Wed Jun 18 13:58:21 UTC 2014


On 06/18/2014 02:07 PM, Pavel Reichl wrote:
> Hello,
>
> could we push the first already ACKed patch to master, please?
>
> I have updated the other 3 patches as asked for, but these are not so
> important and can wait (or be forgotten).
>
> Thanks,
>
> PR
>
>
> On Fri, 2014-04-18 at 10:32 +0200, Jakub Hrozek wrote:
>> 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 at 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!
> Thanks.

Additional ack.

>
>>
>>>  From da8ce47f42c6e551432a23ff813c938af6c5721a Mon Sep 17 00:00:00 2001
>>> From: Pavel Reichl <preichl at 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.
>
> Agreed, fixed.
>

Patch 02:

You  should return all errors from the create_dummy_netgr
properly. Here:

-    netgr = talloc_zero(step_ctx->nctx, struct getent_ctx);
-    if (netgr == NULL) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed, ignored.\n");

and here
-        ret = set_netgroup_entry(step_ctx->nctx, netgr);
-        if (ret != EOK) {
-            DEBUG(SSSDBG_CRIT_FAILURE, "...");
-        }

The caller can than ignore the errors (as it already
does). This is not a big deal, but I think it will be
nicer that way. Currently you only report
talloc_strdup failure to the caller and ignore the other
possible errors, which is a little inconsistent.

Also , I am not sure if create_dummy_netgr is good name
for the helper function. It creates negative cache entry,
so maybe something like create_negative_netgr?

>>
>>>  From 85a1744b803e9d25a82f6ce83a9eaf3d9fdffe16 Mon Sep 17 00:00:00 2001
>>> From: Pavel Reichl <preichl at 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.
>
> Agree, I was asked to fix this by Pavel Brezina, but I don't think he
> cares much about this.
>

Patch 03: Ack.

>>
>> 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 at 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();
>> }
>
> Fixed.

Patch 04: Ack.

Michal





More information about the sssd-devel mailing list