Re: [Qemu-devel] [PATCH v2] mirror: drop local_err in mirror_complete

[Paolo Bonzini]

Il 18/10/2013 19:59, Max Reitz ha scritto:
> Someone obviously intended some purpose for it, so even if it doesn't
> make a difference now (and my RFC is unneeded), I'd still use it to
> propagate errors (instead of passing the error pointer). My point being
> that there *is* a function for propagating errors and I think we should
> therefore use it. The current qemu code seems to alternate between the
> two alternatives, although using error_propagate seems more common to me
> (at least, that was the result when I looked through the code trying to
> decide whether to use it or not).
> 
> Generally, we need a proper discussion about whether error_propagate is
> obsolete or not. Afterwards, we can adapt the current codebase to the
> result of that discussion; but until then, I oppose changing existing
> code without actual need to do so but personal preference.

error_propagate is not obsolete.  It is particularly pervasive in
generated code.

You can and should skip error_propagate if you are tail-calling another
function.  In this case, the extra if/error_propagate pair is useless,
makes the code less clear and adds 3-4 useless lines of code.

If you have an alternative way to see whether an error occurred
(typically based on the return value: <0 if it is int, NULL if it is a
pointer), it is fine to use it instead of error_propagate, because
error_propagate adds some complexity to the logic.  It is also fine to
use error_propagate; whatever makes the code simpler.

However, the converse is not true.  If you have a function that returns
void and takes an Error*, it is not okay to make it return int or bool
for the sake of avoiding error_propagate.

There are also cases where you have a return value, but you cannot use
it to ascertain whether an error occurred.  For example, NULL may be a
valid return value even when no error occurs.  In such case, you have to
use error_propagate.

In the end, I agree with Kevin: "If in doubt, use local_err".  Tail
calls should be the only case where local_err is clearly unnecessary,
any other case needs to be considered specifically.

Paolo