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

Michal Židek mzidek at redhat.com
Wed Jun 18 16:58:59 UTC 2014


On 06/18/2014 06:49 PM, Pavel Reichl wrote:
> 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!
>
>

Ack.

Michal




More information about the sssd-devel mailing list