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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel