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

[Max Reitz]

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. ;-)