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

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

[Fam Zheng]

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>

--
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(-)

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) {
-- 
1.8.3.1

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Fam Zheng]

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

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

[Max Reitz]

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

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

[Stefan Hajnoczi]

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

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

[Kevin Wolf]

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

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

[Fam Zheng]

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

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

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

[Kevin Wolf]

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

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

[Max Reitz]

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

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

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

[Stefan Hajnoczi]

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

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

[Stefan Hajnoczi]

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