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