[Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Fam Zheng]

Coverity doesn't like the ignored return value introduced in
9d3b155186c278 (hw/block: Fix the return type), and other callers are
converted already in ceff3e1f01.

This one was added lately in d9bcd6f7f23a and missed the train. Do it
now.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-generic.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index ba70c0dc19..7414fe2d67 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -482,7 +482,6 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
     int rc;
     int sg_version;
     struct sg_scsi_id scsiid;
-    Error *local_err = NULL;
 
     if (!s->conf.blk) {
         error_setg(errp, "drive property not set");
@@ -516,11 +515,9 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
         error_setg(errp, "SG_GET_SCSI_ID ioctl failed");
         return;
     }
-    blkconf_apply_backend_options(&s->conf,
-                                  blk_is_read_only(s->conf.blk),
-                                  true, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!blkconf_apply_backend_options(&s->conf,
+                                       blk_is_read_only(s->conf.blk),
+                                       true, errp)) {
         return;
     }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Philippe Mathieu-Daudé]

On 01/17/2018 11:52 PM, Fam Zheng wrote:
> Coverity doesn't like the ignored return value introduced in
> 9d3b155186c278 (hw/block: Fix the return type), and other callers are
> converted already in ceff3e1f01.
> 
> This one was added lately in d9bcd6f7f23a and missed the train. Do it
> now.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/scsi/scsi-generic.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index ba70c0dc19..7414fe2d67 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -482,7 +482,6 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
>      int rc;
>      int sg_version;
>      struct sg_scsi_id scsiid;
> -    Error *local_err = NULL;
>  
>      if (!s->conf.blk) {
>          error_setg(errp, "drive property not set");
> @@ -516,11 +515,9 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
>          error_setg(errp, "SG_GET_SCSI_ID ioctl failed");
>          return;
>      }
> -    blkconf_apply_backend_options(&s->conf,
> -                                  blk_is_read_only(s->conf.blk),
> -                                  true, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!blkconf_apply_backend_options(&s->conf,
> +                                       blk_is_read_only(s->conf.blk),
> +                                       true, errp)) {
>          return;
>      }
>  
> 

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Paolo Bonzini]

On 18/01/2018 03:52, Fam Zheng wrote:
> Coverity doesn't like the ignored return value introduced in
> 9d3b155186c278 (hw/block: Fix the return type), and other callers are
> converted already in ceff3e1f01.
> 
> This one was added lately in d9bcd6f7f23a and missed the train. Do it
> now.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/scsi-generic.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index ba70c0dc19..7414fe2d67 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -482,7 +482,6 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
>      int rc;
>      int sg_version;
>      struct sg_scsi_id scsiid;
> -    Error *local_err = NULL;
>  
>      if (!s->conf.blk) {
>          error_setg(errp, "drive property not set");
> @@ -516,11 +515,9 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
>          error_setg(errp, "SG_GET_SCSI_ID ioctl failed");
>          return;
>      }
> -    blkconf_apply_backend_options(&s->conf,
> -                                  blk_is_read_only(s->conf.blk),
> -                                  true, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!blkconf_apply_backend_options(&s->conf,
> +                                       blk_is_read_only(s->conf.blk),
> +                                       true, errp)) {
>          return;
>      }
>  
> 

I'm not a fan of bool return types, in general (because "!" is often
success while "< 0" is failure) and especially when there is an Error**;
I disagree with commit 9d3b155186.  But the function is not in an area I
maintain so I'm queuing this, thanks.

Paolo

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Philippe Mathieu-Daudé]

On 01/18/2018 05:20 AM, Paolo Bonzini wrote:
> On 18/01/2018 03:52, Fam Zheng wrote:
>> Coverity doesn't like the ignored return value introduced in
>> 9d3b155186c278 (hw/block: Fix the return type), and other callers are
>> converted already in ceff3e1f01.
>>
>> This one was added lately in d9bcd6f7f23a and missed the train. Do it
>> now.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  hw/scsi/scsi-generic.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
>> index ba70c0dc19..7414fe2d67 100644
>> --- a/hw/scsi/scsi-generic.c
>> +++ b/hw/scsi/scsi-generic.c
>> @@ -482,7 +482,6 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
>>      int rc;
>>      int sg_version;
>>      struct sg_scsi_id scsiid;
>> -    Error *local_err = NULL;
>>  
>>      if (!s->conf.blk) {
>>          error_setg(errp, "drive property not set");
>> @@ -516,11 +515,9 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
>>          error_setg(errp, "SG_GET_SCSI_ID ioctl failed");
>>          return;
>>      }
>> -    blkconf_apply_backend_options(&s->conf,
>> -                                  blk_is_read_only(s->conf.blk),
>> -                                  true, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> +    if (!blkconf_apply_backend_options(&s->conf,
>> +                                       blk_is_read_only(s->conf.blk),
>> +                                       true, errp)) {
>>          return;
>>      }
>>  
>>
> 
> I'm not a fan of bool return types, in general (because "!" is often
> success while "< 0" is failure) and especially when there is an Error**;
> I disagree with commit 9d3b155186.  But the function is not in an area I
> maintain so I'm queuing this, thanks.

Do you prefer "if (local_err)" and "if (errp && *errp)" ?

I wondered once if a macro might improve this pattern but thought the
code would get more obscure.

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Paolo Bonzini]

On 18/01/2018 12:21, Philippe Mathieu-Daudé wrote:
>> I'm not a fan of bool return types, in general (because "!" is often
>> success while "< 0" is failure) and especially when there is an Error**;
>> I disagree with commit 9d3b155186.  But the function is not in an area I
>> maintain so I'm queuing this, thanks.
> Do you prefer "if (local_err)" and "if (errp && *errp)" ?

The latter is wrong.  I do prefer

    if (local_err) {
        error_propagate(errp, local_err);
        return;
    }

or maybe (but only if there is a meaning to a zero vs. positive return
value, or if errno is an important part of the returned Error *)

    ret = f(..., errp);
    if (ret < 0) {
        return;
    }

> I wondered once if a macro might improve this pattern but thought the
> code would get more obscure.

Eduardo had a series to avoid error_propagate, where NULL was replaced
by a (non-NULL) IGNORED_ERRORS macro.  Then you could do:

    f(..., errp);
    if (error_is_set(errp)) {
        return;
    }

See here:
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03139.html

Paolo

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Philippe Mathieu-Daudé]

On Thu, Jan 18, 2018 at 9:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 18/01/2018 12:21, Philippe Mathieu-Daudé wrote:
>>> I'm not a fan of bool return types, in general (because "!" is often
>>> success while "< 0" is failure) and especially when there is an Error**;
>>> I disagree with commit 9d3b155186.  But the function is not in an area I
>>> maintain so I'm queuing this, thanks.
>> Do you prefer "if (local_err)" and "if (errp && *errp)" ?
>
> The latter is wrong.  I do prefer

Ok so my 253674981e24 missed that train too.

>
>     if (local_err) {
>         error_propagate(errp, local_err);
>         return;
>     }
>
> or maybe (but only if there is a meaning to a zero vs. positive return
> value, or if errno is an important part of the returned Error *)
>
>     ret = f(..., errp);
>     if (ret < 0) {
>         return;
>     }
>
>> I wondered once if a macro might improve this pattern but thought the
>> code would get more obscure.
>
> Eduardo had a series to avoid error_propagate, where NULL was replaced
> by a (non-NULL) IGNORED_ERRORS macro.  Then you could do:
>
>     f(..., errp);
>     if (error_is_set(errp)) {
>         return;
>     }
>
> See here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03139.html

This series never hit /master!

Reading the thread I'm not sure what was the expected outcome.

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Paolo Bonzini]

On 18/01/2018 16:55, Philippe Mathieu-Daudé wrote:
> On Thu, Jan 18, 2018 at 9:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 18/01/2018 12:21, Philippe Mathieu-Daudé wrote:
>>>> I'm not a fan of bool return types, in general (because "!" is often
>>>> success while "< 0" is failure) and especially when there is an Error**;
>>>> I disagree with commit 9d3b155186.  But the function is not in an area I
>>>> maintain so I'm queuing this, thanks.
>>> Do you prefer "if (local_err)" and "if (errp && *errp)" ?
>>
>> The latter is wrong.  I do prefer
> 
> Ok so my 253674981e24 missed that train too.

Well, sdhci_common_realize cannot fail so you can just remove the
Error** argument.

Paolo

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Eric Blake]

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

On 01/18/2018 09:55 AM, Philippe Mathieu-Daudé wrote:
> On Thu, Jan 18, 2018 at 9:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 18/01/2018 12:21, Philippe Mathieu-Daudé wrote:
>>>> I'm not a fan of bool return types, in general (because "!" is often
>>>> success while "< 0" is failure) and especially when there is an Error**;
>>>> I disagree with commit 9d3b155186.  But the function is not in an area I
>>>> maintain so I'm queuing this, thanks.
>>> Do you prefer "if (local_err)" and "if (errp && *errp)" ?
>>
>> The latter is wrong.  I do prefer
> 
> Ok so my 253674981e24 missed that train too.

Markus has expressed as desire, as the error maintainer, to make errp
functions return a useful value for less boilerplate, and at one point
was even debating about Coccinelle scripts to make the conversion
easier.  Perhaps int with -1 is more reliable than bool for that useful
value, but this is definitely a topic of past discussion.

By the way, if (local_err) is definitely preferable; 'if (errp &&
*errp)' means that your behavior is different depending on whether the
caller wanted to ignore the error, and not whether you wanted to handle
the error.

> 
>>
>>     if (local_err) {
>>         error_propagate(errp, local_err);
>>         return;
>>     }

Yes, that's the right boilerplate if you don't have a return value witness.

>>
>> or maybe (but only if there is a meaning to a zero vs. positive return
>> value, or if errno is an important part of the returned Error *)
>>
>>     ret = f(..., errp);
>>     if (ret < 0) {
>>         return;
>>     }
>>
>>> I wondered once if a macro might improve this pattern but thought the
>>> code would get more obscure.
>>
>> Eduardo had a series to avoid error_propagate, where NULL was replaced
>> by a (non-NULL) IGNORED_ERRORS macro.  Then you could do:
>>
>>     f(..., errp);
>>     if (error_is_set(errp)) {
>>         return;
>>     }
>>
>> See here:
>> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03139.html
> 
> This series never hit /master!
> 
> Reading the thread I'm not sure what was the expected outcome.

And since Markus may not answer this thread for a while, I'm still not
sure if there is any expected outcome.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Eduardo Habkost]

On Thu, Jan 18, 2018 at 02:34:39PM -0600, Eric Blake wrote:
> On 01/18/2018 09:55 AM, Philippe Mathieu-Daudé wrote:
> > On Thu, Jan 18, 2018 at 9:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> On 18/01/2018 12:21, Philippe Mathieu-Daudé wrote:
> >>>> I'm not a fan of bool return types, in general (because "!" is often
> >>>> success while "< 0" is failure) and especially when there is an Error**;
> >>>> I disagree with commit 9d3b155186.  But the function is not in an area I
> >>>> maintain so I'm queuing this, thanks.
> >>> Do you prefer "if (local_err)" and "if (errp && *errp)" ?
> >>
> >> The latter is wrong.  I do prefer
> > 
> > Ok so my 253674981e24 missed that train too.

Oops.  I thought we had Coccinelle scripts to detect that
pattern.

> 
> Markus has expressed as desire, as the error maintainer, to make errp
> functions return a useful value for less boilerplate, and at one point
> was even debating about Coccinelle scripts to make the conversion
> easier.  Perhaps int with -1 is more reliable than bool for that useful
> value, but this is definitely a topic of past discussion.
> 
> By the way, if (local_err) is definitely preferable; 'if (errp &&
> *errp)' means that your behavior is different depending on whether the
> caller wanted to ignore the error, and not whether you wanted to handle
> the error.
> 
> > 
> >>
> >>     if (local_err) {
> >>         error_propagate(errp, local_err);
> >>         return;
> >>     }
> 
> Yes, that's the right boilerplate if you don't have a return value witness.
> 
> >>
> >> or maybe (but only if there is a meaning to a zero vs. positive return
> >> value, or if errno is an important part of the returned Error *)
> >>
> >>     ret = f(..., errp);
> >>     if (ret < 0) {
> >>         return;
> >>     }
> >>
> >>> I wondered once if a macro might improve this pattern but thought the
> >>> code would get more obscure.
> >>
> >> Eduardo had a series to avoid error_propagate, where NULL was replaced
> >> by a (non-NULL) IGNORED_ERRORS macro.  Then you could do:
> >>
> >>     f(..., errp);
> >>     if (error_is_set(errp)) {
> >>         return;
> >>     }
> >>
> >> See here:
> >> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03139.html
> > 
> > This series never hit /master!
> > 
> > Reading the thread I'm not sure what was the expected outcome.
> 
> And since Markus may not answer this thread for a while, I'm still not
> sure if there is any expected outcome.

Quoting Markus on that discussion:

"If we switch to returning success/failure (which also gets rid of the
boilerplate), then the macros may still let us get rid of boilerplate
more quickly, for some additional churn.  Worthwhile?  Depends on how
long the return value change takes us."

I guess the lack of activity switching the code to returning
success/failure is enough evidence that the switch will take us
forever?

We can do some effort to document the preferred convention to
return success/failure, but I don't think we will be able to
convert the existing void/ret/bool functions to a single style
(whatever it is) in a reasonable time.

That said, IMO returning 0/-1 or true/false is always preferred
to returning void, so there's no need to add more local_err
boilerplate code.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Paolo Bonzini]

----- Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> We can do some effort to document the preferred convention to
> return success/failure, but I don't think we will be able to
> convert the existing void/ret/bool functions to a single style
> (whatever it is) in a reasonable time.
> 
> That said, IMO returning 0/-1 or true/false is always preferred
> to returning void, so there's no need to add more local_err
> boilerplate code.

I strongly prefer having one way to say things, and having return value and Error*
(with no clear winner for return value) is a disadvantage. Your solution is
slightly more verbose in that it makes it harder to use && and ||, but I am not
even sure it is a disadvantage.  And the clear advantage that a full conversion
is mandatory and can be automated...

Paolo

> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH] scsi-generic: Simplify error handling code

[Eduardo Habkost]

On Thu, Jan 18, 2018 at 05:19:56PM -0500, Paolo Bonzini wrote:
> 
> ----- Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> > We can do some effort to document the preferred convention to
> > return success/failure, but I don't think we will be able to
> > convert the existing void/ret/bool functions to a single style
> > (whatever it is) in a reasonable time.
> > 
> > That said, IMO returning 0/-1 or true/false is always preferred
> > to returning void, so there's no need to add more local_err
> > boilerplate code.
> 
> I strongly prefer having one way to say things, and having return value and Error*
> (with no clear winner for return value) is a disadvantage. [...]


I sympathize with this argument.

...wait, now we're repeating the discussion from the previous
thread:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg461702.html


>                                                      [...] Your solution is
> slightly more verbose in that it makes it harder to use && and ||, but I am not
> even sure it is a disadvantage.  And the clear advantage that a full conversion
> is mandatory and can be automated...

Well, even if we don't decide about void vs non-void right now,
we would still need something better to live with until a
conversion to non-void is finished.  I think I should rebase and
resubmit my ERR_IS_SET series.

-- 
Eduardo