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

[Max Reitz <mreitz@redhat.com>]

On 2013-10-18 10:51, Fam Zheng wrote:
> On Thu, 10/17 15:00, Kevin Wolf wrote:
>> Am 17.10.2013 um 14:49 hat Stefan Hajnoczi geschrieben:
>>> On Wed, Oct 16, 2013 at 08:56:49PM +0200, Max Reitz wrote:
>>>> On 2013-10-15 04:23, Fam Zheng wrote:
>>>> The reason I object it here is that error_propagate *currently* is a
>>>> no-op. But this may change in the future: I have already sent an RFC
>>>> which extends error_propagate so it can generate an error backtrace
>>>> if enabled through configure. If this (or something similar which
>>>> extends error_propagate to do more than basically just *errp =
>>>> local_err) is merged to/implemented in qemu later on, I believe we
>>>> want to call error_propagate in every function that touches an error
>>>> variable in order to generate a backtrace with maximum verbosity.
>>> Did you check if glib's backtrace(3) APIs can be used in error_set()
>>> instead of rolling our own backtrace?
>>>
>>> Also, what is the purpose of the backtrace?  If the problem is that
>>> error messages don't identify unique errors, then we should fix that
>>> instead of relying on backtraces.  For example, a function that opens
>>> two separate files shouldn't just fail with "File not found" since we
>>> don't know which of the two files wasn't found.
>> Mostly debugging, I guess. Even if you have unique error messages that
>> can only come from a single place, finding that single place by looking
>> at all called functions from a given QMP command can take quite a bit of
>> time. I can see it useful.
>>
>> And we don't even have the unique error messages yet, so we shouldn't
>> fall in the trap of rejecting an improvement because it's not perfect.
> Stacktrace already provides useful information for debugging, so I'm wondering
> if it makes sense to add support (a framework) to catch or dump the stacktrace,
> which can be used in error_set(), abort() and tracing framework.

Well, it seems like a hack to me, but if it works… Okay, that's why I 
sent that series as an RFC with the comment “Since I do not know whether 
I am the only one considering it useful in the first place, this is just 
an RFC for now.” Now I know. ;-)

> Manually calling error_propagate as above sounds a unnecessary reinvention of
> this.

Then there's still the problem that I'm not the one who introduced 
error_propagate. 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.

Max


PS: I wrote my error_propagate RFC in part because I was disappointed 
about how much of a no-op error_propagate actually is and was trying to 
change that. ;-)