| Message ID | 1381803804-4551-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/14/2013 08:23 PM, Fam Zheng wrote: > There is errp passed in, so no need for local_err and error_propagate. > Also drop the backing_filename which is set but unused since 34b5d2c. > > Signed-off-by: Fam Zheng <famz@redhat.com> > > -- Three dashes instead of two before 'git am' will notice that the rest of the comment is mail-only. > v2: fix typo in subject line. > drop backing_filename. > (Thanks Eric) > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/mirror.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On Mon, 10/14 21:05, Eric Blake wrote: > On 10/14/2013 08:23 PM, Fam Zheng wrote: > > There is errp passed in, so no need for local_err and error_propagate. > > Also drop the backing_filename which is set but unused since 34b5d2c. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > -- > > Three dashes instead of two before 'git am' will notice that the rest of > the comment is mail-only. Sorry, stingy me. > > > v2: fix typo in subject line. > > drop backing_filename. > > (Thanks Eric) > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block/mirror.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> > Thanks, Fam
On 2013-10-15 04:23, Fam Zheng wrote: > There is errp passed in, so no need for local_err and error_propagate. > Also drop the backing_filename which is set but unused since 34b5d2c. I approve of dropping the backing_filename code, but I don't know if I like removing the error_propagate. I personally like explicit error_propagate calls in every function receiving an error code from an underlying function and propagating that as its own. There are two reasons for this: The first reason is pragmatic: There are lots of places in qemu where this is done exactly that way. In fact, I'm responsible for some myself. "Fixing" all of them is basically impossible and also unreasonable, since the worst error_propagate can do is blow up the code size. However, this is not the reason I'd object a patch doing something different (here: dropping the unused backing_filename code) and dropping "redundant" error_propagate calls along the way. 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. Max
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. Stefan
On Wed, Oct 16, 2013 at 08:56:49PM +0200, Max Reitz wrote: > However, this is not the reason I'd object a patch doing > something different (here: dropping the unused backing_filename > code) BTW I agree with this. This should be a separate patch. Stefan
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. Kevin
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. Manually calling error_propagate as above sounds a unnecessary reinvention of this. Fam
On Thu, Oct 17, 2013 at 03:00:23PM +0200, 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. Relying on a backtrace is a workaround that only helps developers, not users. Users cannot interpret the backtrace to troubleshoot their own problem. If developers become content with backtraces then we won't fix the actual error messages. Error messages are useful to both users and developers so we should focus on them. They will reduce the amount of troubleshooting and help that developers need to field from users who are left stuck by poor error messages. Stefan
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. ;-)
Am 18.10.2013 um 19:59 hat Max Reitz geschrieben: > 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. ;-) It's not a no-op. The point is that the caller can pass NULL as errp if it isn't interested in the error message. If you do anything else with errp than just passing it to other functions - most commonly a error_is_set() check - you need to make sure that you use a local Error variable and propagate it. Otherwise, if the caller passed NULL, your error path would never get executed. So in this specific case, not having a local_err and error_propagate() does work, but it's not generally safe. If in doubt, use local_err. Regarding this patch, I'm not sure it's useful. Kevin
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
On 2013-10-19 10:05, Kevin Wolf wrote: > Am 18.10.2013 um 19:59 hat Max Reitz geschrieben: >> 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. ;-) > It's not a no-op. That's why I wrote “how much of a” instead of “that error_propagate is a no-op”. ;-) > The point is that the caller can pass NULL as errp if > it isn't interested in the error message. If you do anything else with > errp than just passing it to other functions - most commonly a > error_is_set() check - you need to make sure that you use a local Error > variable and propagate it. Otherwise, if the caller passed NULL, your > error path would never get executed. Okay, together with Paolo's comment I believe I now can see the meaning of error_propagate and its intended use; thank you both for explaining. Max
diff --git a/block/mirror.c b/block/mirror.c index 7b95acf..7a73022 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -505,15 +505,10 @@ static void mirror_iostatus_reset(BlockJob *job) static void mirror_complete(BlockJob *job, Error **errp) { MirrorBlockJob *s = container_of(job, MirrorBlockJob, common); - Error *local_err = NULL; int ret; - ret = bdrv_open_backing_file(s->target, NULL, &local_err); + ret = bdrv_open_backing_file(s->target, NULL, errp); if (ret < 0) { - char backing_filename[PATH_MAX]; - bdrv_get_full_backing_filename(s->target, backing_filename, - sizeof(backing_filename)); - error_propagate(errp, local_err); return; } if (!s->synced) {