| Message ID | 1465589538-24998-3-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/10/2016 02:12 PM, Eduardo Habkost wrote: > This patch simplifies code that uses a local_err variable just to > immediately use it for an error_propagate() call. > > Coccinelle patch used to perform the changes added to > scripts/coccinelle/remove_local_err.cocci. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > block.c | 8 ++------ > block/raw-posix.c | 8 ++------ > block/raw_bsd.c | 4 +--- > blockdev.c | 16 +++++----------- > hw/s390x/s390-virtio-ccw.c | 5 +---- > hw/s390x/virtio-ccw.c | 28 +++++++--------------------- > scripts/coccinelle/remove_local_err.cocci | 27 +++++++++++++++++++++++++++ > target-i386/cpu.c | 4 +--- > 8 files changed, 46 insertions(+), 54 deletions(-) > create mode 100644 scripts/coccinelle/remove_local_err.cocci > > +++ b/block.c > @@ -294,14 +294,12 @@ typedef struct CreateCo { > > static void coroutine_fn bdrv_create_co_entry(void *opaque) > { > - Error *local_err = NULL; > int ret; > > CreateCo *cco = opaque; > assert(cco->drv); > > - ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err); > - error_propagate(&cco->err, local_err); > + ret = cco->drv->bdrv_create(cco->filename, cco->opts, &cco->err); > cco->ret = ret; This hunk doesn't get simplified by 3/3; you may want to consider a manual followup to drop 'int ret' and just assign cco->drv->bdrv_create() directly to cco->ret. But doesn't change this patch. > +++ b/blockdev.c > @@ -3654,7 +3654,6 @@ void qmp_blockdev_mirror(const char *device, const char *target, > BlockBackend *blk; > BlockDriverState *target_bs; > AioContext *aio_context; > - Error *local_err = NULL; > > blk = blk_by_name(device); > if (!blk) { > @@ -3678,16 +3677,11 @@ void qmp_blockdev_mirror(const char *device, const char *target, > > bdrv_set_aio_context(target_bs, aio_context); > > - blockdev_mirror_common(bs, target_bs, > - has_replaces, replaces, sync, > - has_speed, speed, > - has_granularity, granularity, > - has_buf_size, buf_size, > - has_on_source_error, on_source_error, > - has_on_target_error, on_target_error, > - true, true, > - &local_err); > - error_propagate(errp, local_err); > + blockdev_mirror_common(bs, target_bs, has_replaces, replaces, sync, > + has_speed, speed, has_granularity, granularity, > + has_buf_size, buf_size, has_on_source_error, > + on_source_error, has_on_target_error, > + on_target_error, true, true, errp); Coccinelle messes a bit with the formatting (the old way explicitly tried to pair related has_foo with foo). But I'm going to mess with it again with my qapi patches for passing a boxed parameter rather than lots of arguments, so don't worry about it. > +++ b/scripts/coccinelle/remove_local_err.cocci > @@ -0,0 +1,27 @@ > +// Replace unnecessary usage of local_err variable with > +// direct usage of errp argument > + > +@@ > +expression list ARGS; > +expression F2; > +identifier LOCAL_ERR; > +expression ERRP; > +idexpression V; > +typedef Error; > +expression I; > +@@ > + { > + ... > +- Error *LOCAL_ERR; > + ... when != LOCAL_ERR > +( > +- F2(ARGS, &LOCAL_ERR); > +- error_propagate(ERRP, LOCAL_ERR); > ++ F2(ARGS, ERRP); > +| > +- V = F2(ARGS, &LOCAL_ERR); > +- error_propagate(ERRP, LOCAL_ERR); > ++ V = F2(ARGS, ERRP); > +) > + ... when != LOCAL_ERR > + } Looks good. Reviewed-by: Eric Blake <eblake@redhat.com>
On Fri, Jun 10, 2016 at 02:59:55PM -0600, Eric Blake wrote: > On 06/10/2016 02:12 PM, Eduardo Habkost wrote: > > This patch simplifies code that uses a local_err variable just to > > immediately use it for an error_propagate() call. > > > > Coccinelle patch used to perform the changes added to > > scripts/coccinelle/remove_local_err.cocci. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > block.c | 8 ++------ > > block/raw-posix.c | 8 ++------ > > block/raw_bsd.c | 4 +--- > > blockdev.c | 16 +++++----------- > > hw/s390x/s390-virtio-ccw.c | 5 +---- > > hw/s390x/virtio-ccw.c | 28 +++++++--------------------- > > scripts/coccinelle/remove_local_err.cocci | 27 +++++++++++++++++++++++++++ > > target-i386/cpu.c | 4 +--- > > 8 files changed, 46 insertions(+), 54 deletions(-) > > create mode 100644 scripts/coccinelle/remove_local_err.cocci > > > > > +++ b/block.c > > @@ -294,14 +294,12 @@ typedef struct CreateCo { > > > > static void coroutine_fn bdrv_create_co_entry(void *opaque) > > { > > - Error *local_err = NULL; > > int ret; > > > > CreateCo *cco = opaque; > > assert(cco->drv); > > > > - ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err); > > - error_propagate(&cco->err, local_err); > > + ret = cco->drv->bdrv_create(cco->filename, cco->opts, &cco->err); > > cco->ret = ret; > > This hunk doesn't get simplified by 3/3; you may want to consider a > manual followup to drop 'int ret' and just assign > cco->drv->bdrv_create() directly to cco->ret. But doesn't change this > patch. This could become yet another Coccinelle script, but we need to be careful about type conversions, and tell it to do it only if the types of 'ret', 'cc->drv->bdrv_create()' and 'cco->ret' are the same.
On Fri, 10 Jun 2016 17:12:17 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > This patch simplifies code that uses a local_err variable just to > immediately use it for an error_propagate() call. > > Coccinelle patch used to perform the changes added to > scripts/coccinelle/remove_local_err.cocci. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > block.c | 8 ++------ > block/raw-posix.c | 8 ++------ > block/raw_bsd.c | 4 +--- > blockdev.c | 16 +++++----------- > hw/s390x/s390-virtio-ccw.c | 5 +---- > hw/s390x/virtio-ccw.c | 28 +++++++--------------------- For the two virtio-ccw files: Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com> > scripts/coccinelle/remove_local_err.cocci | 27 +++++++++++++++++++++++++++ > target-i386/cpu.c | 4 +--- > 8 files changed, 46 insertions(+), 54 deletions(-) > create mode 100644 scripts/coccinelle/remove_local_err.cocci
Eduardo Habkost <ehabkost@redhat.com> writes: > This patch simplifies code that uses a local_err variable just to > immediately use it for an error_propagate() call. > > Coccinelle patch used to perform the changes added to > scripts/coccinelle/remove_local_err.cocci. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> [...] > diff --git a/scripts/coccinelle/remove_local_err.cocci b/scripts/coccinelle/remove_local_err.cocci > new file mode 100644 > index 0000000..5f0b6c0 > --- /dev/null > +++ b/scripts/coccinelle/remove_local_err.cocci > @@ -0,0 +1,27 @@ > +// Replace unnecessary usage of local_err variable with > +// direct usage of errp argument > + > +@@ > +expression list ARGS; > +expression F2; > +identifier LOCAL_ERR; > +expression ERRP; > +idexpression V; > +typedef Error; > +expression I; I isn't used. > +@@ > + { > + ... > +- Error *LOCAL_ERR; > + ... when != LOCAL_ERR > +( > +- F2(ARGS, &LOCAL_ERR); > +- error_propagate(ERRP, LOCAL_ERR); > ++ F2(ARGS, ERRP); > +| > +- V = F2(ARGS, &LOCAL_ERR); > +- error_propagate(ERRP, LOCAL_ERR); > ++ V = F2(ARGS, ERRP); > +) > + ... when != LOCAL_ERR > + } There is an (ugly) difference between error_setg(&local_err, ...); error_propagate(errp, &local_err); and error_setg(errp, ...); The latter aborts when @errp already contains an error, the former does nothing. Your transformation has the error_setg() or similar hidden in F2(). It can add aborts. I think it can be salvaged: we know that @errp must not contain an error on function entry. If @errp doesn't occur elsewhere in this function, it cannot pick up an error on the way to the transformed spot. Can you add that to your when constraints? [...]
On Mon, Jun 13, 2016 at 01:42:15PM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > This patch simplifies code that uses a local_err variable just to > > immediately use it for an error_propagate() call. > > > > Coccinelle patch used to perform the changes added to > > scripts/coccinelle/remove_local_err.cocci. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > [...] > > diff --git a/scripts/coccinelle/remove_local_err.cocci b/scripts/coccinelle/remove_local_err.cocci > > new file mode 100644 > > index 0000000..5f0b6c0 > > --- /dev/null > > +++ b/scripts/coccinelle/remove_local_err.cocci > > @@ -0,0 +1,27 @@ > > +// Replace unnecessary usage of local_err variable with > > +// direct usage of errp argument > > + > > +@@ > > +expression list ARGS; > > +expression F2; > > +identifier LOCAL_ERR; > > +expression ERRP; > > +idexpression V; > > +typedef Error; > > +expression I; > > I isn't used. > > > +@@ > > + { > > + ... > > +- Error *LOCAL_ERR; > > + ... when != LOCAL_ERR > > +( > > +- F2(ARGS, &LOCAL_ERR); > > +- error_propagate(ERRP, LOCAL_ERR); > > ++ F2(ARGS, ERRP); > > +| > > +- V = F2(ARGS, &LOCAL_ERR); > > +- error_propagate(ERRP, LOCAL_ERR); > > ++ V = F2(ARGS, ERRP); > > +) > > + ... when != LOCAL_ERR > > + } > > There is an (ugly) difference between > > error_setg(&local_err, ...); > error_propagate(errp, &local_err); > > and > > error_setg(errp, ...); > > The latter aborts when @errp already contains an error, the former does > nothing. Why the difference? Couldn't we change that so that both are equivalent? > > Your transformation has the error_setg() or similar hidden in F2(). It > can add aborts. > > I think it can be salvaged: we know that @errp must not contain an error > on function entry. If @errp doesn't occur elsewhere in this function, > it cannot pick up an error on the way to the transformed spot. Can you > add that to your when constraints? Do we really know that *errp is NULL on entry? Aren't we allowed to call functions with a non-NULL *errp? See, e.g.: void qmp_guest_suspend_disk(Error **errp) { Error *local_err = NULL; GuestSuspendMode *mode = g_new(GuestSuspendMode, 1); *mode = GUEST_SUSPEND_MODE_DISK; check_suspend_mode(*mode, &local_err); acquire_privilege(SE_SHUTDOWN_NAME, &local_err); execute_async(do_suspend, mode, &local_err); if (local_err) { error_propagate(errp, local_err); g_free(mode); } }
On 06/13/2016 09:52 AM, Eduardo Habkost wrote: >> >> There is an (ugly) difference between >> >> error_setg(&local_err, ...); >> error_propagate(errp, &local_err); >> >> and >> >> error_setg(errp, ...); >> >> The latter aborts when @errp already contains an error, the former does >> nothing. > > Why the difference? Couldn't we change that so that both are equivalent? Maybe, but I think it weakens our position. An abort() on an attempt to incorrectly set an error twice helps catch errors where we are throwing away a more useful first error message. The documentation for error_propagate() already mentioned that it was an exception to the rule. > >> >> Your transformation has the error_setg() or similar hidden in F2(). It >> can add aborts. >> >> I think it can be salvaged: we know that @errp must not contain an error >> on function entry. If @errp doesn't occur elsewhere in this function, >> it cannot pick up an error on the way to the transformed spot. Can you >> add that to your when constraints? > > Do we really know that *errp is NULL on entry? Aren't we allowed to call > functions with a non-NULL *errp? Except for error_propagate(), no. > > See, e.g.: > > void qmp_guest_suspend_disk(Error **errp) > { > Error *local_err = NULL; > GuestSuspendMode *mode = g_new(GuestSuspendMode, 1); > > *mode = GUEST_SUSPEND_MODE_DISK; > check_suspend_mode(*mode, &local_err); > acquire_privilege(SE_SHUTDOWN_NAME, &local_err); > execute_async(do_suspend, mode, &local_err); That usage is a bug. A Coccinelle script to root out such buggy instances would be nice.
Eric Blake <eblake@redhat.com> writes: > On 06/13/2016 09:52 AM, Eduardo Habkost wrote: > >>> >>> There is an (ugly) difference between >>> >>> error_setg(&local_err, ...); >>> error_propagate(errp, &local_err); >>> >>> and >>> >>> error_setg(errp, ...); >>> >>> The latter aborts when @errp already contains an error, the former does >>> nothing. >> >> Why the difference? Couldn't we change that so that both are equivalent? > > Maybe, but I think it weakens our position. An abort() on an attempt to > incorrectly set an error twice helps catch errors where we are throwing > away a more useful first error message. The documentation for > error_propagate() already mentioned that it was an exception to the rule. For what it's worth, both g_set_error(err, ...) and g_propagate_error(err, ...) require !err || !*err. To accumulate multiple errors so that the first one wins, you have to do something like if (local_err) { g_clear_error(errp); g_propagate_error(errp, local_err); } error.h happend before we got GLib. Its designers chose to deviate from GLib and made error_propagate() accumulate errors. This trades the ability to detect misuse for less boilerplate: error_propagate(errp, local_err); Tightening error_propagate() now would lead to a rather tedious hunt for callers that rely on its accumulating behavior. We could do it incrementally by renaming error_propagate() to error_accumulate(), and have a new error_propagate() that's consistent with error_setg(). Not sure it's worth it. Loosening error_setg() & friends is also possible, but we'd detect fewer misuse, and we'd be left with some superfluous code to clean up. Not sure that's smart. >>> Your transformation has the error_setg() or similar hidden in F2(). It >>> can add aborts. >>> >>> I think it can be salvaged: we know that @errp must not contain an error >>> on function entry. If @errp doesn't occur elsewhere in this function, >>> it cannot pick up an error on the way to the transformed spot. Can you >>> add that to your when constraints? >> >> Do we really know that *errp is NULL on entry? Aren't we allowed to call >> functions with a non-NULL *errp? > > Except for error_propagate(), no. By convention, all functions taking an Error **errp argument expect one that can be safely passed to error_setg(). That means !errp || errp == &error_abort || errp == &error_fatal || !*errp. >> >> See, e.g.: >> >> void qmp_guest_suspend_disk(Error **errp) >> { >> Error *local_err = NULL; >> GuestSuspendMode *mode = g_new(GuestSuspendMode, 1); >> >> *mode = GUEST_SUSPEND_MODE_DISK; >> check_suspend_mode(*mode, &local_err); >> acquire_privilege(SE_SHUTDOWN_NAME, &local_err); >> execute_async(do_suspend, mode, &local_err); > > That usage is a bug. A Coccinelle script to root out such buggy > instances would be nice. We've discussed this before. See for instance commit 297a364: qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent.
On Mon, Jun 13, 2016 at 10:01:16AM -0600, Eric Blake wrote: > On 06/13/2016 09:52 AM, Eduardo Habkost wrote: [...] > > > > See, e.g.: > > > > void qmp_guest_suspend_disk(Error **errp) > > { > > Error *local_err = NULL; > > GuestSuspendMode *mode = g_new(GuestSuspendMode, 1); > > > > *mode = GUEST_SUSPEND_MODE_DISK; > > check_suspend_mode(*mode, &local_err); > > acquire_privilege(SE_SHUTDOWN_NAME, &local_err); > > execute_async(do_suspend, mode, &local_err); > > That usage is a bug. A Coccinelle script to root out such buggy > instances would be nice. I've tried to write one, but got lots of false positives due to error-checking using the function return value, not local_err. For reference, this is the script I have used: @@ typedef Error; identifier F1 !~ "hmp_handle_error|error_free_or_abort"; identifier F2 !~ "hmp_handle_error|error_free_or_abort"; idexpression Error *ERR; @@ -F1(..., &ERR) +FIXME_incorrect_error_usage1() ... when != ERR -F2(..., &ERR) +FIXME_incorrect_error_usage2() @@ identifier F1 !~ "hmp_handle_error|error_free_or_abort"; identifier F2 !~ "hmp_handle_error|error_free_or_abort"; idexpression Error **ERRP; @@ -F1(..., ERRP) +FIXME_incorrect_error_usage1() ... when != ERRP -F2(..., ERRP) +FIXME_incorrect_error_usage2()
On Mon, Jun 13, 2016 at 08:49:37PM +0200, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: [...] > >> > >> See, e.g.: > >> > >> void qmp_guest_suspend_disk(Error **errp) > >> { > >> Error *local_err = NULL; > >> GuestSuspendMode *mode = g_new(GuestSuspendMode, 1); > >> > >> *mode = GUEST_SUSPEND_MODE_DISK; > >> check_suspend_mode(*mode, &local_err); > >> acquire_privilege(SE_SHUTDOWN_NAME, &local_err); > >> execute_async(do_suspend, mode, &local_err); > > > > That usage is a bug. A Coccinelle script to root out such buggy > > instances would be nice. > > We've discussed this before. See for instance commit 297a364: > > qapi: Replace uncommon use of the error API by the common one That explains why I was confused: I remember seeing that QAPI visitors could be called with *errp set. I will try to change the script as suggested, to only apply the changes if errp is never touched before the error_setg() call.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Mon, Jun 13, 2016 at 08:49:37PM +0200, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: > [...] >> >> >> >> See, e.g.: >> >> >> >> void qmp_guest_suspend_disk(Error **errp) >> >> { >> >> Error *local_err = NULL; >> >> GuestSuspendMode *mode = g_new(GuestSuspendMode, 1); >> >> >> >> *mode = GUEST_SUSPEND_MODE_DISK; >> >> check_suspend_mode(*mode, &local_err); >> >> acquire_privilege(SE_SHUTDOWN_NAME, &local_err); >> >> execute_async(do_suspend, mode, &local_err); >> > >> > That usage is a bug. A Coccinelle script to root out such buggy >> > instances would be nice. >> >> We've discussed this before. See for instance commit 297a364: >> >> qapi: Replace uncommon use of the error API by the common one > > That explains why I was confused: I remember seeing that QAPI > visitors could be called with *errp set. Nothing confuses as effectively as bad examples. > I will try to change the script as suggested, to only apply the > changes if errp is never touched before the error_setg() call. Thanks!
diff --git a/block.c b/block.c index ecca55a..d516ab6 100644 --- a/block.c +++ b/block.c @@ -294,14 +294,12 @@ typedef struct CreateCo { static void coroutine_fn bdrv_create_co_entry(void *opaque) { - Error *local_err = NULL; int ret; CreateCo *cco = opaque; assert(cco->drv); - ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err); - error_propagate(&cco->err, local_err); + ret = cco->drv->bdrv_create(cco->filename, cco->opts, &cco->err); cco->ret = ret; } @@ -353,7 +351,6 @@ out: int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp) { BlockDriver *drv; - Error *local_err = NULL; int ret; drv = bdrv_find_protocol(filename, true, errp); @@ -361,8 +358,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp) return -ENOENT; } - ret = bdrv_create(drv, filename, opts, &local_err); - error_propagate(errp, local_err); + ret = bdrv_create(drv, filename, opts, errp); return ret; } diff --git a/block/raw-posix.c b/block/raw-posix.c index cb663d8..d7397bf 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -582,12 +582,10 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVRawState *s = bs->opaque; - Error *local_err = NULL; int ret; s->type = FTYPE_FILE; - ret = raw_open_common(bs, options, flags, 0, &local_err); - error_propagate(errp, local_err); + ret = raw_open_common(bs, options, flags, 0, errp); return ret; } @@ -2442,14 +2440,12 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVRawState *s = bs->opaque; - Error *local_err = NULL; int ret; s->type = FTYPE_CD; /* open will not fail even if no CD is inserted, so add O_NONBLOCK */ - ret = raw_open_common(bs, options, flags, O_NONBLOCK, &local_err); - error_propagate(errp, local_err); + ret = raw_open_common(bs, options, flags, O_NONBLOCK, errp); return ret; } diff --git a/block/raw_bsd.c b/block/raw_bsd.c index 5af11b6..b51ac98 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -190,11 +190,9 @@ static int raw_has_zero_init(BlockDriverState *bs) static int raw_create(const char *filename, QemuOpts *opts, Error **errp) { - Error *local_err = NULL; int ret; - ret = bdrv_create_file(filename, opts, &local_err); - error_propagate(errp, local_err); + ret = bdrv_create_file(filename, opts, errp); return ret; } diff --git a/blockdev.c b/blockdev.c index 028dba3..3b6d242 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3654,7 +3654,6 @@ void qmp_blockdev_mirror(const char *device, const char *target, BlockBackend *blk; BlockDriverState *target_bs; AioContext *aio_context; - Error *local_err = NULL; blk = blk_by_name(device); if (!blk) { @@ -3678,16 +3677,11 @@ void qmp_blockdev_mirror(const char *device, const char *target, bdrv_set_aio_context(target_bs, aio_context); - blockdev_mirror_common(bs, target_bs, - has_replaces, replaces, sync, - has_speed, speed, - has_granularity, granularity, - has_buf_size, buf_size, - has_on_source_error, on_source_error, - has_on_target_error, on_target_error, - true, true, - &local_err); - error_propagate(errp, local_err); + blockdev_mirror_common(bs, target_bs, has_replaces, replaces, sync, + has_speed, speed, has_granularity, granularity, + has_buf_size, buf_size, has_on_source_error, + on_source_error, has_on_target_error, + on_target_error, true, true, errp); aio_context_release(aio_context); } diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 95ff5e3..b7112d0 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -180,10 +180,7 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine, static void s390_hot_add_cpu(const int64_t id, Error **errp) { MachineState *machine = MACHINE(qdev_get_machine()); - Error *err = NULL; - - s390x_new_cpu(machine->cpu_model, id, &err); - error_propagate(errp, err); + s390x_new_cpu(machine->cpu_model, id, errp); } static void ccw_machine_class_init(ObjectClass *oc, void *data) diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 464b091..50b0935 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -899,13 +899,11 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp) DeviceState *qdev = DEVICE(ccw_dev); VirtIONetCcw *dev = VIRTIO_NET_CCW(ccw_dev); DeviceState *vdev = DEVICE(&dev->vdev); - Error *err = NULL; virtio_net_set_netclient_name(&dev->vdev, qdev->id, object_get_typename(OBJECT(qdev))); qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); - object_property_set_bool(OBJECT(vdev), true, "realized", &err); - error_propagate(errp, err); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); } static void virtio_ccw_net_instance_init(Object *obj) @@ -922,11 +920,9 @@ static void virtio_ccw_blk_realize(VirtioCcwDevice *ccw_dev, Error **errp) { VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev); DeviceState *vdev = DEVICE(&dev->vdev); - Error *err = NULL; qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); - object_property_set_bool(OBJECT(vdev), true, "realized", &err); - error_propagate(errp, err); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); } static void virtio_ccw_blk_instance_init(Object *obj) @@ -946,7 +942,6 @@ static void virtio_ccw_serial_realize(VirtioCcwDevice *ccw_dev, Error **errp) VirtioSerialCcw *dev = VIRTIO_SERIAL_CCW(ccw_dev); DeviceState *vdev = DEVICE(&dev->vdev); DeviceState *proxy = DEVICE(ccw_dev); - Error *err = NULL; char *bus_name; /* @@ -960,8 +955,7 @@ static void virtio_ccw_serial_realize(VirtioCcwDevice *ccw_dev, Error **errp) } qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); - object_property_set_bool(OBJECT(vdev), true, "realized", &err); - error_propagate(errp, err); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); } @@ -977,11 +971,9 @@ static void virtio_ccw_balloon_realize(VirtioCcwDevice *ccw_dev, Error **errp) { VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(ccw_dev); DeviceState *vdev = DEVICE(&dev->vdev); - Error *err = NULL; qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); - object_property_set_bool(OBJECT(vdev), true, "realized", &err); - error_propagate(errp, err); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); } static void virtio_ccw_balloon_instance_init(Object *obj) @@ -1002,7 +994,6 @@ static void virtio_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp) VirtIOSCSICcw *dev = VIRTIO_SCSI_CCW(ccw_dev); DeviceState *vdev = DEVICE(&dev->vdev); DeviceState *qdev = DEVICE(ccw_dev); - Error *err = NULL; char *bus_name; /* @@ -1016,8 +1007,7 @@ static void virtio_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp) } qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); - object_property_set_bool(OBJECT(vdev), true, "realized", &err); - error_propagate(errp, err); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); } static void virtio_ccw_scsi_instance_init(Object *obj) @@ -1035,11 +1025,9 @@ static void vhost_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp) { VHostSCSICcw *dev = VHOST_SCSI_CCW(ccw_dev); DeviceState *vdev = DEVICE(&dev->vdev); - Error *err = NULL; qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); - object_property_set_bool(OBJECT(vdev), true, "realized", &err); - error_propagate(errp, err); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); } static void vhost_ccw_scsi_instance_init(Object *obj) @@ -1856,11 +1844,9 @@ static void virtio_ccw_9p_realize(VirtioCcwDevice *ccw_dev, Error **errp) { V9fsCCWState *dev = VIRTIO_9P_CCW(ccw_dev); DeviceState *vdev = DEVICE(&dev->vdev); - Error *err = NULL; qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); - object_property_set_bool(OBJECT(vdev), true, "realized", &err); - error_propagate(errp, err); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); } static void virtio_ccw_9p_class_init(ObjectClass *klass, void *data) diff --git a/scripts/coccinelle/remove_local_err.cocci b/scripts/coccinelle/remove_local_err.cocci new file mode 100644 index 0000000..5f0b6c0 --- /dev/null +++ b/scripts/coccinelle/remove_local_err.cocci @@ -0,0 +1,27 @@ +// Replace unnecessary usage of local_err variable with +// direct usage of errp argument + +@@ +expression list ARGS; +expression F2; +identifier LOCAL_ERR; +expression ERRP; +idexpression V; +typedef Error; +expression I; +@@ + { + ... +- Error *LOCAL_ERR; + ... when != LOCAL_ERR +( +- F2(ARGS, &LOCAL_ERR); +- error_propagate(ERRP, LOCAL_ERR); ++ F2(ARGS, ERRP); +| +- V = F2(ARGS, &LOCAL_ERR); +- error_propagate(ERRP, LOCAL_ERR); ++ V = F2(ARGS, ERRP); +) + ... when != LOCAL_ERR + } diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 895a386..2cea40a 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1820,7 +1820,6 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, { uint32_t *array = (uint32_t *)opaque; FeatureWord w; - Error *err = NULL; X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; X86CPUFeatureWordInfoList *list = NULL; @@ -1840,8 +1839,7 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, list = &list_entries[w]; } - visit_type_X86CPUFeatureWordInfoList(v, "feature-words", &list, &err); - error_propagate(errp, err); + visit_type_X86CPUFeatureWordInfoList(v, "feature-words", &list, errp); } static void x86_get_hv_spinlocks(Object *obj, Visitor *v, const char *name,
This patch simplifies code that uses a local_err variable just to immediately use it for an error_propagate() call. Coccinelle patch used to perform the changes added to scripts/coccinelle/remove_local_err.cocci. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- block.c | 8 ++------ block/raw-posix.c | 8 ++------ block/raw_bsd.c | 4 +--- blockdev.c | 16 +++++----------- hw/s390x/s390-virtio-ccw.c | 5 +---- hw/s390x/virtio-ccw.c | 28 +++++++--------------------- scripts/coccinelle/remove_local_err.cocci | 27 +++++++++++++++++++++++++++ target-i386/cpu.c | 4 +--- 8 files changed, 46 insertions(+), 54 deletions(-) create mode 100644 scripts/coccinelle/remove_local_err.cocci