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

Pavel Reichl preichl at redhat.com
Wed Jun 18 16:49:59 UTC 2014


On Wed, 2014-06-18 at 18:47 +0200, Michal Židek wrote:
> On 06/18/2014 06:26 PM, Pavel Reichl wrote:
> > On Wed, 2014-06-18 at 17:50 +0200, Michal Židek wrote:
> >> On 06/18/2014 04:38 PM, Pavel Reichl wrote:
> >>> Hello Michal,
> >>>
> >>> thank you for quick review. Please see attached patches (only 2nd
> >>> changed).
> >>>
> >>> On Wed, 2014-06-18 at 15:58 +0200, Michal Židek wrote:
> >>> [snip]
> >>>> 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.
> >>>
> >>> I think you are right, in previous patch I was just being careful not to
> >>> change anything - to be 1:1 with original code, but I agree that these
> >>> changes are for better.
> >>>
> >>>>
> >>>> 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?
> >>>
> >>> Renamed, hope you like the new name better.
> >>>
> >>> Thanks,
> >>>
> >>> PR
> >>>
> >>
> >> In this part
> >>
> >>    454         ret = set_netgroup_entry(step_ctx->nctx, netgr);
> >>    455         if (ret != EOK) {
> >>    456             DEBUG(SSSDBG_CRIT_FAILURE, "...");
> >>    457             return ret;
> >>    458         }
> >>
> >> you miss a talloc_free(netgr) call before return. It does
> >> not really make sense to keep the memory allocated when
> >> you do not call the set_netgr_lifetime and return. I think
> >> it was a bug in the original code, that we actually called
> >> the set_netgr_lifetime even after set_netgroup_entry failed.
> >> Sorry, I should have said this in the previous iteration.
> >>
> >> Michal
> > OK, updated path attached.
> >
> > Regards,
> > PR
> >
> 
> Sorry, but this is wrong. You should not free the netgr
> pointer if the function succeeds. I originally just wanted
> you to add the free call before the return statement
> in the failed branch. I agree that having multiple
> free calls is not very nice, so the "done" label you
> added is a better way, but in that case, you have
> to check the ret value and only free the netgr
> pointer if it is not EOK.
> 
> Michal

Silly me, thanks for noticing!



> _______________________________________________
> 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/0aff99c8/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-NSS-sysdb_getnetgr-refactor.patch
Type: text/x-patch
Size: 3381 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140618/0aff99c8/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: 4023 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140618/0aff99c8/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/0aff99c8/attachment-0003.bin>


More information about the sssd-devel mailing list