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

Pavel Reichl preichl at redhat.com
Wed Jun 18 12:07:27 UTC 2014


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.

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

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

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

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-NSS-sysdb_getnetgr-check-return-value-first.patch
Type: text/x-patch
Size: 2314 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140618/68381106/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-NSS-sysdb_getnetgr-refactor.patch
Type: text/x-patch
Size: 3256 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140618/68381106/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-NSS-fix-memory-leak-in-sysdb_getnetgr.patch
Type: text/x-patch
Size: 4011 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140618/68381106/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-NSS-minor-code-style-improvements.patch
Type: text/x-patch
Size: 1768 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140618/68381106/attachment-0003.bin>


More information about the sssd-devel mailing list