| Message ID | 20180118025245.13042-1-famz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } > >
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
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.
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
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.
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
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.
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 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
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.
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; }
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(-)