|
From: | Max Reitz |
Subject: | Re: [Qemu-devel] [PATCH v2] mirror: drop local_err in mirror_complete |
Date: | Fri, 18 Oct 2013 19:59:30 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0 |
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.
MaxPS: 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. ;-)
[Prev in Thread] | Current Thread | [Next in Thread] |