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