When the function no success value to transmit, it usually make the
function return void. It has turned out not to be a success, because
it means that the extra local_err variable and error_propagate() will
be needed. It leads to cumbersome code, therefore, transmit success/
failure in the return value is worth.
So fix the return type of blkconf_apply_backend_options(),
blkconf_geometry() and virtio_blk_data_plane_create() to avoid it.
Cc: John Snow <jsnow@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/block/block.c | 21 ++++++++++++---------
hw/block/dataplane/virtio-blk.c | 16 +++++++++-------
hw/block/dataplane/virtio-blk.h | 6 +++---
include/hw/block/block.h | 10 +++++-----
4 files changed, 29 insertions(+), 24 deletions(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index 27878d0..717bd0e 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -51,8 +51,8 @@ void blkconf_blocksizes(BlockConf *conf)
}
}
-void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
- bool resizable, Error **errp)
+int blkconf_apply_backend_options(BlockConf *conf, bool readonly,
+ bool resizable, Error **errp)
{
BlockBackend *blk = conf->blk;
BlockdevOnError rerror, werror;
@@ -76,7 +76,7 @@ void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
ret = blk_set_perm(blk, perm, shared_perm, errp);
if (ret < 0) {
- return;
+ return -1;
}
switch (conf->wce) {
@@ -99,11 +99,13 @@ void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
blk_set_enable_write_cache(blk, wce);
blk_set_on_error(blk, rerror, werror);
+
+ return 0;
}
-void blkconf_geometry(BlockConf *conf, int *ptrans,
- unsigned cyls_max, unsigned heads_max, unsigned secs_max,
- Error **errp)
+int blkconf_geometry(BlockConf *conf, int *ptrans,
+ unsigned cyls_max, unsigned heads_max, unsigned secs_max,
+ Error **errp)
{
DriveInfo *dinfo;
@@ -129,15 +131,16 @@ void blkconf_geometry(BlockConf *conf, int *ptrans,
if (conf->cyls || conf->heads || conf->secs) {
if (conf->cyls < 1 || conf->cyls > cyls_max) {
error_setg(errp, "cyls must be between 1 and %u", cyls_max);
- return;
+ return -1;
}
if (conf->heads < 1 || conf->heads > heads_max) {
error_setg(errp, "heads must be between 1 and %u", heads_max);
- return;
+ return -1;
}
if (conf->secs < 1 || conf->secs > secs_max) {
error_setg(errp, "secs must be between 1 and %u", secs_max);
- return;
+ return -1;
}
}
+ return 0;
}
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5556f0e..619bc5e 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -76,9 +76,9 @@ static void notify_guest_bh(void *opaque)
}
/* Context: QEMU global mutex held */
-void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
- VirtIOBlockDataPlane **dataplane,
- Error **errp)
+int virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
+ VirtIOBlockDataPlane **dataplane,
+ Error **errp)
{
VirtIOBlockDataPlane *s;
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
@@ -91,11 +91,11 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
error_setg(errp,
"device is incompatible with iothread "
"(transport does not support notifiers)");
- return;
+ return -1;
}
if (!virtio_device_ioeventfd_enabled(vdev)) {
error_setg(errp, "ioeventfd is required for iothread");
- return;
+ return -1;
}
/* If dataplane is (re-)enabled while the guest is running there could
@@ -103,12 +103,12 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
*/
if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
error_prepend(errp, "cannot start virtio-blk dataplane: ");
- return;
+ return -1;
}
}
/* Don't try if transport does not support notifiers. */
if (!virtio_device_ioeventfd_enabled(vdev)) {
- return;
+ return -1;
}
s = g_new0(VirtIOBlockDataPlane, 1);
@@ -126,6 +126,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
+
+ return 0;
}
/* Context: QEMU global mutex held */
diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h
index db3f47b..d25773d 100644
--- a/hw/block/dataplane/virtio-blk.h
+++ b/hw/block/dataplane/virtio-blk.h
@@ -19,9 +19,9 @@
typedef struct VirtIOBlockDataPlane VirtIOBlockDataPlane;
-void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
- VirtIOBlockDataPlane **dataplane,
- Error **errp);
+int virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
+ VirtIOBlockDataPlane **dataplane,
+ Error **errp);
void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s);
void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq);
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index f3f6e8e..66117c6 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -72,12 +72,12 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
/* Configuration helpers */
void blkconf_serial(BlockConf *conf, char **serial);
-void blkconf_geometry(BlockConf *conf, int *trans,
- unsigned cyls_max, unsigned heads_max, unsigned secs_max,
- Error **errp);
+int blkconf_geometry(BlockConf *conf, int *trans,
+ unsigned cyls_max, unsigned heads_max, unsigned secs_max,
+ Error **errp);
void blkconf_blocksizes(BlockConf *conf);
-void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
- bool resizable, Error **errp);
+int blkconf_apply_backend_options(BlockConf *conf, bool readonly,
+ bool resizable, Error **errp);
/* Hard disk geometry */
--
2.9.4
On Wed, Jul 26, 2017 at 08:02:53PM +0800, Mao Zhongyi wrote: > When the function no success value to transmit, it usually make the > function return void. It has turned out not to be a success, because > it means that the extra local_err variable and error_propagate() will > be needed. It leads to cumbersome code, therefore, transmit success/ > failure in the return value is worth. > > So fix the return type of blkconf_apply_backend_options(), > blkconf_geometry() and virtio_blk_data_plane_create() to avoid it. > > Cc: John Snow <jsnow@redhat.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > > Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com> > --- > hw/block/block.c | 21 ++++++++++++--------- > hw/block/dataplane/virtio-blk.c | 16 +++++++++------- > hw/block/dataplane/virtio-blk.h | 6 +++--- > include/hw/block/block.h | 10 +++++----- > 4 files changed, 29 insertions(+), 24 deletions(-) > > diff --git a/hw/block/block.c b/hw/block/block.c > index 27878d0..717bd0e 100644 > --- a/hw/block/block.c > +++ b/hw/block/block.c > @@ -51,8 +51,8 @@ void blkconf_blocksizes(BlockConf *conf) > } > } > > -void blkconf_apply_backend_options(BlockConf *conf, bool readonly, > - bool resizable, Error **errp) > +int blkconf_apply_backend_options(BlockConf *conf, bool readonly, > + bool resizable, Error **errp) I'm not a fan of these changes because it makes inconsistencies between the return value and the errp argument possible (e.g. returning success but setting errp, or returning failure without setting errp). If you really want to do this please use bool as the return type instead of int. int can be abused to return error information that should really be in the Error object. Stefan
Stefan Hajnoczi <stefanha@gmail.com> writes:
> On Wed, Jul 26, 2017 at 08:02:53PM +0800, Mao Zhongyi wrote:
>> When the function no success value to transmit, it usually make the
>> function return void. It has turned out not to be a success, because
>> it means that the extra local_err variable and error_propagate() will
>> be needed. It leads to cumbersome code, therefore, transmit success/
>> failure in the return value is worth.
>>
>> So fix the return type of blkconf_apply_backend_options(),
>> blkconf_geometry() and virtio_blk_data_plane_create() to avoid it.
>>
>> Cc: John Snow <jsnow@redhat.com>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> hw/block/block.c | 21 ++++++++++++---------
>> hw/block/dataplane/virtio-blk.c | 16 +++++++++-------
>> hw/block/dataplane/virtio-blk.h | 6 +++---
>> include/hw/block/block.h | 10 +++++-----
>> 4 files changed, 29 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/block/block.c b/hw/block/block.c
>> index 27878d0..717bd0e 100644
>> --- a/hw/block/block.c
>> +++ b/hw/block/block.c
>> @@ -51,8 +51,8 @@ void blkconf_blocksizes(BlockConf *conf)
>> }
>> }
>>
>> -void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
>> - bool resizable, Error **errp)
>> +int blkconf_apply_backend_options(BlockConf *conf, bool readonly,
>> + bool resizable, Error **errp)
>
> I'm not a fan of these changes because it makes inconsistencies between
> the return value and the errp argument possible (e.g. returning success
> but setting errp, or returning failure without setting errp).
Opinions and practice vary on this one, and we've discussed the
tradeoffs a few times.
Having both an Error parameter and an error return value poses the
question whether the two agree. When there's no success value to
transmit, you avoid the problem by making the function return void. The
problem remains for all the function that return a value on success, and
therefore must return some error value on failure. A related problem
even remains for functions returning void: consistency between side
effects and the Error parameter.
Returning void leads to awkward boilerplate:
Error err = NULL;
foo(..., err);
if (err) {
error_propagate(errp, err);
... bail out ...
}
Compare:
if (!foo(..., errp)) {
... bail out ...
}
Much easier on the eyes.
For what it's worth, GLib wants you to transmit success / failure in the
return value, too:
https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html#gerror-rules
Plenty of older code returns void, some newer code doesn't, because
returning void is just too awkward. We willfully deviated from GLib's
convention, and we're now paying the price.
See also
Subject: Re: [RFC 00/15] Error API: Flag errors in *errp even if errors are being ignored
Message-ID: <87o9t8qy7d.fsf@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg06191.html
> If you really want to do this please use bool as the return type instead
> of int. int can be abused to return error information that should
> really be in the Error object.
For what it's worth, GLib wants bool[*]. Let's stick to that unless we
have a compelling reason to differ.
[*] Except being GLib, it wants its very own homemade version of bool.
Which is inferior to stdbool.h's in pretty much every conceivable way.
Hi
On 08/01/2017 10:29 PM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Wed, Jul 26, 2017 at 08:02:53PM +0800, Mao Zhongyi wrote:
>>> When the function no success value to transmit, it usually make the
>>> function return void. It has turned out not to be a success, because
>>> it means that the extra local_err variable and error_propagate() will
>>> be needed. It leads to cumbersome code, therefore, transmit success/
>>> failure in the return value is worth.
>>>
>>> So fix the return type of blkconf_apply_backend_options(),
>>> blkconf_geometry() and virtio_blk_data_plane_create() to avoid it.
>>>
>>> Cc: John Snow <jsnow@redhat.com>
>>> Cc: Kevin Wolf <kwolf@redhat.com>
>>> Cc: Max Reitz <mreitz@redhat.com>
>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>>
>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>> ---
>>> hw/block/block.c | 21 ++++++++++++---------
>>> hw/block/dataplane/virtio-blk.c | 16 +++++++++-------
>>> hw/block/dataplane/virtio-blk.h | 6 +++---
>>> include/hw/block/block.h | 10 +++++-----
>>> 4 files changed, 29 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/hw/block/block.c b/hw/block/block.c
>>> index 27878d0..717bd0e 100644
>>> --- a/hw/block/block.c
>>> +++ b/hw/block/block.c
>>> @@ -51,8 +51,8 @@ void blkconf_blocksizes(BlockConf *conf)
>>> }
>>> }
>>>
>>> -void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
>>> - bool resizable, Error **errp)
>>> +int blkconf_apply_backend_options(BlockConf *conf, bool readonly,
>>> + bool resizable, Error **errp)
>>
>> I'm not a fan of these changes because it makes inconsistencies between
>> the return value and the errp argument possible (e.g. returning success
>> but setting errp, or returning failure without setting errp).
>
> Opinions and practice vary on this one, and we've discussed the
> tradeoffs a few times.
>
> Having both an Error parameter and an error return value poses the
> question whether the two agree. When there's no success value to
> transmit, you avoid the problem by making the function return void. The
> problem remains for all the function that return a value on success, and
> therefore must return some error value on failure. A related problem
> even remains for functions returning void: consistency between side
> effects and the Error parameter.
>
> Returning void leads to awkward boilerplate:
>
> Error err = NULL;
>
> foo(..., err);
> if (err) {
> error_propagate(errp, err);
> ... bail out ...
> }
>
> Compare:
>
> if (!foo(..., errp)) {
> ... bail out ...
> }
>
> Much easier on the eyes.
>
> For what it's worth, GLib wants you to transmit success / failure in the
> return value, too:
>
> https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html#gerror-rules
>
> Plenty of older code returns void, some newer code doesn't, because
> returning void is just too awkward. We willfully deviated from GLib's
> convention, and we're now paying the price.
>
> See also
> Subject: Re: [RFC 00/15] Error API: Flag errors in *errp even if errors are being ignored
> Message-ID: <87o9t8qy7d.fsf@dusky.pond.sub.org>
> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg06191.html
>
>> If you really want to do this please use bool as the return type instead
>> of int. int can be abused to return error information that should
>> really be in the Error object.
>
> For what it's worth, GLib wants bool[*]. Let's stick to that unless we
> have a compelling reason to differ.
>
>
> [*] Except being GLib, it wants its very own homemade version of bool.
> Which is inferior to stdbool.h's in pretty much every conceivable way.
>
Thanks for pointing that out！
I will use bool as the return type instead of int.
Thanks,
Mao
© 2016 - 2026 Red Hat, Inc.