Stephen Gallagher <sgallagh(a)redhat.com> wrote:
On 09/03/2010 08:11 AM, Jan Zelený wrote:
> 0001-Dead-assignments-cleanup-in-providers-code.patch:
> Here the biggest change is in prototype of sdap_access_decide_offline,
> which IMO doesn't need to return any value, since it returns alway EOK
> now and there is no code which could be failing.
Nack
The change in child_common is wrong. The assignment of ret = errno is
intentional, because ret was supposed to be used in the DEBUG statement
below, rather than errno. It's supposed to be a protection in case the
DEBUG macro ever changes to something that might reset errno internally.
So the real bug here is that we are printing errno instead of ret.
Thanks for the info.
if (be_is_offline(state->be_ctx)) {
/* Ok, we're offline. Return from the cache */
- ret = sdap_access_decide_offline(req);
+ sdap_access_decide_offline(req);
goto finished;
}
Please add ret = EOK before 'goto finished'. While it just happens to be
true right now, I'd rather we kept this explicit here, in case we ever
add something above this that changes ret.
That would create dead assignment and this patch should eliminate them rather
than create new ones. I would agree with you if the ret value was used after
"finished" label, but this isn't the case. I just want to be sure about
this.
--
Jan