Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables
On Mon, Jun 13, 2016 at 01:42:15PM +0200, Markus Armbruster wrote: > Eduardo Habkostwrites: > > > 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 > [...] > > diff --git a/scripts/coccinelle/remove_local_err.cocci > > b/scripts/coccinelle/remove_local_err.cocci > > new file mode 100644 > > index 000..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, _ERR); > > +-error_propagate(ERRP, LOCAL_ERR); > > ++F2(ARGS, ERRP); > > +| > > +-V = F2(ARGS, _ERR); > > +-error_propagate(ERRP, LOCAL_ERR); > > ++V = F2(ARGS, ERRP); > > +) > > + ... when != LOCAL_ERR > > + } > > There is an (ugly) difference between > > error_setg(_err, ...); > error_propagate(errp, _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, _err); acquire_privilege(SE_SHUTDOWN_NAME, _err); execute_async(do_suspend, mode, _err); if (local_err) { error_propagate(errp, local_err); g_free(mode); } } -- Eduardo
Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables
Eduardo Habkostwrites: > 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 [...] > diff --git a/scripts/coccinelle/remove_local_err.cocci > b/scripts/coccinelle/remove_local_err.cocci > new file mode 100644 > index 000..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, _ERR); > +-error_propagate(ERRP, LOCAL_ERR); > ++F2(ARGS, ERRP); > +| > +-V = F2(ARGS, _ERR); > +-error_propagate(ERRP, LOCAL_ERR); > ++V = F2(ARGS, ERRP); > +) > + ... when != LOCAL_ERR > + } There is an (ugly) difference between error_setg(_err, ...); error_propagate(errp, _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? [...]
Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables
On Fri, 10 Jun 2016 17:12:17 -0300 Eduardo Habkostwrote: > 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 > --- > 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 > 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
Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables
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> > --- > > 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, _err); > > -error_propagate(>err, local_err); > > +ret = cco->drv->bdrv_create(cco->filename, cco->opts, >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. -- Eduardo
Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables
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> --- > 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, _err); > -error_propagate(>err, local_err); > +ret = cco->drv->bdrv_create(cco->filename, cco->opts, >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, > - _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, _ERR); > +-error_propagate(ERRP, LOCAL_ERR); > ++F2(ARGS, ERRP); > +| > +-V = F2(ARGS, _ERR); > +-error_propagate(ERRP, LOCAL_ERR); > ++V = F2(ARGS, ERRP); > +) > + ... when != LOCAL_ERR > + } Looks good. Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables
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--- 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 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, _err); -error_propagate(>err, local_err); +ret = cco->drv->bdrv_create(cco->filename, cco->opts, >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, _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, _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, _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, _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, - _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