From da002526ac30f679b0c797b0d71102dcdf200fe6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 31 May 2016 10:31:38 +0200 Subject: vl: Error messages need to go to stderr, fix some We print a few fatal error messages to stdout instead of stderr. Reproducer: $ qemu-system-x86_64 -g 1024x768 Option g not supported for this target $ qemu-system-x86_64 -g 1024x768 >/dev/null Fix by printing them with error_report(). This also improves the messages. The above one becomes qemu-system-x86_64: -g 1024x768: Option not supported for this target Reported-by: Tobi {github.com/tobimensch} Signed-off-by: Markus Armbruster Message-Id: <1464683498-28779-1-git-send-email-armbru@redhat.com> Reviewed-by: Marcel Apfelbaum --- vl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vl.c b/vl.c index 2f63eb448f..18af70afb7 100644 --- a/vl.c +++ b/vl.c @@ -3068,7 +3068,7 @@ int main(int argc, char **argv, char **envp) popt = lookup_opt(argc, argv, &optarg, &optind); if (!(popt->arch_mask & arch_type)) { - printf("Option %s not supported for this target\n", popt->name); + error_report("Option not supported for this target"); exit(1); } switch(popt->index) { @@ -3846,21 +3846,21 @@ int main(int argc, char **argv, char **envp) break; case QEMU_OPTION_xen_domid: if (!(xen_available())) { - printf("Option %s not supported for this target\n", popt->name); + error_report("Option not supported for this target"); exit(1); } xen_domid = atoi(optarg); break; case QEMU_OPTION_xen_create: if (!(xen_available())) { - printf("Option %s not supported for this target\n", popt->name); + error_report("Option not supported for this target"); exit(1); } xen_mode = XEN_CREATE; break; case QEMU_OPTION_xen_attach: if (!(xen_available())) { - printf("Option %s not supported for this target\n", popt->name); + error_report("Option not supported for this target"); exit(1); } xen_mode = XEN_ATTACH; -- cgit v1.2.1 From 621ff94d5074d88253a5818c6b9c4db718fbfc65 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Mon, 13 Jun 2016 18:57:56 -0300 Subject: error: Remove NULL checks on error_propagate() calls error_propagate() already ignores local_err==NULL, so there's no need to check it before calling. Coccinelle patch used to perform the changes added to scripts/coccinelle/error_propagate_null.cocci. Reviewed-by: Eric Blake Acked-by: Cornelia Huck Reviewed-by: Markus Armbruster Signed-off-by: Eduardo Habkost Message-Id: <1465855078-19435-2-git-send-email-ehabkost@redhat.com> Signed-off-by: Markus Armbruster --- block.c | 20 +++++-------------- block/qcow2.c | 4 +--- block/quorum.c | 4 +--- block/raw-posix.c | 16 ++++----------- block/raw_bsd.c | 4 +--- block/snapshot.c | 4 +--- blockdev.c | 12 +++--------- bootdevice.c | 4 +--- dump.c | 4 +--- hw/ide/qdev.c | 4 +--- hw/net/ne2000-isa.c | 4 +--- hw/s390x/virtio-ccw.c | 28 +++++++-------------------- hw/usb/dev-storage.c | 4 +--- qga/commands-win32.c | 8 ++------ qom/object.c | 4 +--- scripts/coccinelle/error_propagate_null.cocci | 10 ++++++++++ 16 files changed, 41 insertions(+), 93 deletions(-) create mode 100644 scripts/coccinelle/error_propagate_null.cocci diff --git a/block.c b/block.c index b331eb9d38..524aa542fa 100644 --- a/block.c +++ b/block.c @@ -301,9 +301,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque) assert(cco->drv); ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err); - if (local_err) { - error_propagate(&cco->err, local_err); - } + error_propagate(&cco->err, local_err); cco->ret = ret; } @@ -364,9 +362,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp) } ret = bdrv_create(drv, filename, opts, &local_err); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return ret; } @@ -1763,18 +1759,14 @@ fail: QDECREF(options); bs->options = NULL; bdrv_unref(bs); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return NULL; close_and_fail: bdrv_unref(bs); QDECREF(snapshot_options); QDECREF(options); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return NULL; } @@ -3599,9 +3591,7 @@ void bdrv_img_create(const char *filename, const char *fmt, out: qemu_opts_del(opts); qemu_opts_free(create_opts); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); } AioContext *bdrv_get_aio_context(BlockDriverState *bs) diff --git a/block/qcow2.c b/block/qcow2.c index 4718f8250e..23f666d4ae 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2403,9 +2403,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags, cluster_size, prealloc, opts, version, refcount_order, &local_err); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); finish: g_free(backing_file); diff --git a/block/quorum.c b/block/quorum.c index ec6f3b9059..331b726f97 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -971,9 +971,7 @@ close_exit: exit: qemu_opts_del(opts); /* propagate error */ - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return ret; } diff --git a/block/raw-posix.c b/block/raw-posix.c index aacf13203f..a825a0a9f5 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -587,9 +587,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, s->type = FTYPE_FILE; ret = raw_open_common(bs, options, flags, 0, &local_err); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return ret; } @@ -2236,9 +2234,7 @@ hdev_open_Mac_error: ret = raw_open_common(bs, options, flags, 0, &local_err); if (ret < 0) { - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); #if defined(__APPLE__) && defined(__MACH__) if (*bsd_path) { filename = bsd_path; @@ -2450,9 +2446,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags, /* 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); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return ret; } @@ -2571,9 +2565,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags, ret = raw_open_common(bs, options, flags, 0, &local_err); if (ret) { - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return ret; } diff --git a/block/raw_bsd.c b/block/raw_bsd.c index b1d5237135..5af11b6621 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -194,9 +194,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp) int ret; ret = bdrv_create_file(filename, opts, &local_err); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return ret; } diff --git a/block/snapshot.c b/block/snapshot.c index da89d2b085..bf5c2ca5e1 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -358,9 +358,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs, ret = bdrv_snapshot_load_tmp(bs, NULL, id_or_name, &local_err); } - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); return ret; } diff --git a/blockdev.c b/blockdev.c index c9a0068cd6..8f94f760a2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3644,9 +3644,7 @@ void qmp_drive_mirror(const char *device, const char *target, has_unmap, unmap, &local_err); bdrv_unref(target_bs); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); out: aio_context_release(aio_context); } @@ -3701,9 +3699,7 @@ void qmp_blockdev_mirror(const char *device, const char *target, has_on_target_error, on_target_error, true, true, &local_err); - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); aio_context_release(aio_context); } @@ -3913,9 +3909,7 @@ void qmp_change_backing_file(const char *device, if (ro) { bdrv_reopen(image_bs, open_flags, &local_err); - if (local_err) { - error_propagate(errp, local_err); /* will preserve prior errp */ - } + error_propagate(errp, local_err); } out: diff --git a/bootdevice.c b/bootdevice.c index bb9c08e535..33e3029e40 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -302,9 +302,7 @@ static void device_set_bootindex(Object *obj, Visitor *v, const char *name, add_boot_device_path(*prop->bootindex, prop->dev, prop->suffix); out: - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); } static void property_release_bootindex(Object *obj, const char *name, diff --git a/dump.c b/dump.c index 9726f1f477..f7b80d856b 100644 --- a/dump.c +++ b/dump.c @@ -918,9 +918,7 @@ static void write_dump_header(DumpState *s, Error **errp) } else { create_header64(s, &local_err); } - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); } static size_t dump_bitmap_get_bufsize(DumpState *s) diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 4bc74a32d2..6842a5596a 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -233,9 +233,7 @@ static void ide_dev_set_bootindex(Object *obj, Visitor *v, const char *name, d->unit ? "/disk@1" : "/disk@0"); } out: - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); } static void ide_dev_instance_init(Object *obj) diff --git a/hw/net/ne2000-isa.c b/hw/net/ne2000-isa.c index a7f5a9464d..8fab7ae2ec 100644 --- a/hw/net/ne2000-isa.c +++ b/hw/net/ne2000-isa.c @@ -127,9 +127,7 @@ static void isa_ne2000_set_bootindex(Object *obj, Visitor *v, s->c.bootindex = boot_index; out: - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); } static void isa_ne2000_instance_init(Object *obj) diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 2192be8774..348ae4f668 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -890,9 +890,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp) object_get_typename(OBJECT(qdev))); qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); object_property_set_bool(OBJECT(vdev), true, "realized", &err); - if (err) { - error_propagate(errp, err); - } + error_propagate(errp, err); } static void virtio_ccw_net_instance_init(Object *obj) @@ -913,9 +911,7 @@ static void virtio_ccw_blk_realize(VirtioCcwDevice *ccw_dev, Error **errp) qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); object_property_set_bool(OBJECT(vdev), true, "realized", &err); - if (err) { - error_propagate(errp, err); - } + error_propagate(errp, err); } static void virtio_ccw_blk_instance_init(Object *obj) @@ -950,9 +946,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); - if (err) { - error_propagate(errp, err); - } + error_propagate(errp, err); } @@ -972,9 +966,7 @@ static void virtio_ccw_balloon_realize(VirtioCcwDevice *ccw_dev, Error **errp) qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); object_property_set_bool(OBJECT(vdev), true, "realized", &err); - if (err) { - error_propagate(errp, err); - } + error_propagate(errp, err); } static void virtio_ccw_balloon_instance_init(Object *obj) @@ -1010,9 +1002,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); - if (err) { - error_propagate(errp, err); - } + error_propagate(errp, err); } static void virtio_ccw_scsi_instance_init(Object *obj) @@ -1034,9 +1024,7 @@ static void vhost_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); - if (err) { - error_propagate(errp, err); - } + error_propagate(errp, err); } static void vhost_ccw_scsi_instance_init(Object *obj) @@ -1858,9 +1846,7 @@ static void virtio_ccw_9p_realize(VirtioCcwDevice *ccw_dev, Error **errp) qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); object_property_set_bool(OBJECT(vdev), true, "realized", &err); - if (err) { - error_propagate(errp, err); - } + error_propagate(errp, err); } static void virtio_ccw_9p_class_init(ObjectClass *klass, void *data) diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index 248a580457..9fd00dfffc 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -818,9 +818,7 @@ static void usb_msd_set_bootindex(Object *obj, Visitor *v, const char *name, } out: - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); } static const TypeInfo usb_storage_dev_type_info = { diff --git a/qga/commands-win32.c b/qga/commands-win32.c index c1a8588206..ea23797c0f 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -247,9 +247,7 @@ out: if (token) { CloseHandle(token); } - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); } static void execute_async(DWORD WINAPI (*func)(LPVOID), LPVOID opaque, @@ -882,9 +880,7 @@ static void check_suspend_mode(GuestSuspendMode mode, Error **errp) } out: - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); } static DWORD WINAPI do_suspend(LPVOID opaque) diff --git a/qom/object.c b/qom/object.c index 0311414c0a..9743ea4708 100644 --- a/qom/object.c +++ b/qom/object.c @@ -549,9 +549,7 @@ Object *object_new_with_propv(const char *typename, return obj; error: - if (local_err) { - error_propagate(errp, local_err); - } + error_propagate(errp, local_err); object_unref(obj); return NULL; } diff --git a/scripts/coccinelle/error_propagate_null.cocci b/scripts/coccinelle/error_propagate_null.cocci new file mode 100644 index 0000000000..c23638007a --- /dev/null +++ b/scripts/coccinelle/error_propagate_null.cocci @@ -0,0 +1,10 @@ +// error_propagate() already ignores local_err==NULL, so there's +// no need to check it before calling. + +@@ +identifier L; +expression E; +@@ +-if (L) { + error_propagate(E, L); +-} -- cgit v1.2.1 From 6b62d961373e0327f2af8fb77d6d5d6308864180 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Mon, 13 Jun 2016 18:57:57 -0300 Subject: 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. Reviewed-by: Eric Blake Acked-by: Cornelia Huck Signed-off-by: Eduardo Habkost Message-Id: <1465855078-19435-3-git-send-email-ehabkost@redhat.com> Reviewed-by: Markus Armbruster [Blank line in s390-virtio-ccw.c restored] Signed-off-by: Markus Armbruster --- block/raw-posix.c | 8 ++------ block/raw_bsd.c | 4 +--- hw/s390x/s390-virtio-ccw.c | 4 +--- hw/s390x/virtio-ccw.c | 28 +++++++--------------------- scripts/coccinelle/remove_local_err.cocci | 29 +++++++++++++++++++++++++++++ target-i386/cpu.c | 4 +--- 6 files changed, 41 insertions(+), 36 deletions(-) create mode 100644 scripts/coccinelle/remove_local_err.cocci diff --git a/block/raw-posix.c b/block/raw-posix.c index a825a0a9f5..dbdabc9cf1 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; } @@ -2439,14 +2437,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 5af11b6621..b51ac98061 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/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index e257ca5ab0..52f079a884 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -180,10 +180,8 @@ 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 348ae4f668..1625e6b38b 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -884,13 +884,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) @@ -907,11 +905,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) @@ -931,7 +927,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; /* @@ -945,8 +940,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); } @@ -962,11 +956,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) @@ -987,7 +979,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; /* @@ -1001,8 +992,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) @@ -1020,11 +1010,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) @@ -1842,11 +1830,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 0000000000..9261c99687 --- /dev/null +++ b/scripts/coccinelle/remove_local_err.cocci @@ -0,0 +1,29 @@ +// Replace unnecessary usage of local_err variable with +// direct usage of errp argument + +@@ +identifier F; +expression list ARGS; +expression F2; +identifier LOCAL_ERR; +identifier ERRP; +idexpression V; +typedef Error; +@@ + F(..., Error **ERRP) + { + ... +- Error *LOCAL_ERR; + ... when != LOCAL_ERR + when != ERRP +( +- 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 3665fecb5f..3bd3cfc3ad 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1875,7 +1875,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; @@ -1895,8 +1894,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, -- cgit v1.2.1 From 9be385980d37e8f4fd33f605f5fb1c3d144170a8 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Mon, 13 Jun 2016 18:57:58 -0300 Subject: coccinelle: Remove unnecessary variables for function return value Use Coccinelle script to replace 'ret = E; return ret' with 'return E'. The script will do the substitution only when the function return type and variable type are the same. Manual fixups: * audio/audio.c: coding style of "read (...)" and "write (...)" * block/qcow2-cluster.c: wrap line to make it shorter * block/qcow2-refcount.c: change indentation of wrapped line * target-tricore/op_helper.c: fix coding style of "remainder|quotient" * target-mips/dsp_helper.c: reverted changes because I don't want to argue about checkpatch.pl * ui/qemu-pixman.c: fix line indentation * block/rbd.c: restore blank line between declarations and statements Reviewed-by: Eric Blake Signed-off-by: Eduardo Habkost Message-Id: <1465855078-19435-4-git-send-email-ehabkost@redhat.com> Reviewed-by: Markus Armbruster [Unused Coccinelle rule name dropped along with a redundant comment; whitespace touched up in block/qcow2-cluster.c; stale commit message paragraph deleted] Signed-off-by: Markus Armbruster --- audio/audio.c | 10 ++-------- block/archipelago.c | 4 +--- block/qcow2-cluster.c | 6 ++---- block/qcow2-refcount.c | 7 ++----- block/raw-posix.c | 8 ++------ block/raw_bsd.c | 5 +---- block/rbd.c | 4 +--- block/vmdk.c | 6 ++---- block/vvfat.c | 5 +---- hw/acpi/aml-build.c | 13 +++---------- hw/audio/intel-hda.c | 5 +---- hw/display/vga.c | 4 +--- hw/intc/s390_flic_kvm.c | 5 ++--- hw/pci-host/uninorth.c | 5 +---- hw/ppc/spapr_vio.c | 5 +---- hw/scsi/megasas.c | 5 +---- hw/scsi/scsi-generic.c | 5 +---- hw/timer/mc146818rtc.c | 4 +--- hw/virtio/virtio-pci.c | 4 +--- linux-user/signal.c | 15 ++++----------- page_cache.c | 5 +---- qga/commands-posix.c | 4 +--- qga/commands-win32.c | 5 +---- qobject/qlist.c | 5 +---- scripts/coccinelle/return_directly.cocci | 19 +++++++++++++++++++ target-i386/fpu_helper.c | 10 ++-------- target-i386/kvm.c | 5 ++--- target-mips/op_helper.c | 4 +--- target-s390x/helper.c | 6 +----- target-sparc/cc_helper.c | 25 +++++-------------------- target-tricore/op_helper.c | 13 ++++--------- tests/display-vga-test.c | 6 +----- tests/endianness-test.c | 5 +---- tests/i440fx-test.c | 4 +--- tests/intel-hda-test.c | 6 +----- tests/test-filter-redirector.c | 6 +----- tests/virtio-blk-test.c | 5 +---- tests/virtio-console-test.c | 6 +----- tests/virtio-net-test.c | 6 +----- tests/virtio-scsi-test.c | 6 +----- tests/wdt_ib700-test.c | 6 +----- ui/cursor.c | 10 ++-------- ui/qemu-pixman.c | 13 +++++-------- util/module.c | 6 +----- vl.c | 5 +---- 45 files changed, 88 insertions(+), 228 deletions(-) create mode 100644 scripts/coccinelle/return_directly.cocci diff --git a/audio/audio.c b/audio/audio.c index e60c124de8..9d4dcc72dd 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -1131,8 +1131,6 @@ static void audio_timer (void *opaque) */ int AUD_write (SWVoiceOut *sw, void *buf, int size) { - int bytes; - if (!sw) { /* XXX: Consider options */ return size; @@ -1143,14 +1141,11 @@ int AUD_write (SWVoiceOut *sw, void *buf, int size) return 0; } - bytes = sw->hw->pcm_ops->write (sw, buf, size); - return bytes; + return sw->hw->pcm_ops->write(sw, buf, size); } int AUD_read (SWVoiceIn *sw, void *buf, int size) { - int bytes; - if (!sw) { /* XXX: Consider options */ return size; @@ -1161,8 +1156,7 @@ int AUD_read (SWVoiceIn *sw, void *buf, int size) return 0; } - bytes = sw->hw->pcm_ops->read (sw, buf, size); - return bytes; + return sw->hw->pcm_ops->read(sw, buf, size); } int AUD_get_buffer_size_out (SWVoiceOut *sw) diff --git a/block/archipelago.c b/block/archipelago.c index b9f5e69d4a..37b8aca78d 100644 --- a/block/archipelago.c +++ b/block/archipelago.c @@ -974,11 +974,9 @@ err_exit2: static int64_t qemu_archipelago_getlength(BlockDriverState *bs) { - int64_t ret; BDRVArchipelagoState *s = bs->opaque; - ret = archipelago_volume_info(s); - return ret; + return archipelago_volume_info(s); } static int qemu_archipelago_truncate(BlockDriverState *bs, int64_t offset) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 893ddf6798..0fb43566fb 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -154,11 +154,9 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset, uint64_t **l2_table) { BDRVQcow2State *s = bs->opaque; - int ret; - - ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset, (void**) l2_table); - return ret; + return qcow2_cache_get(bs, s->l2_table_cache, l2_offset, + (void **)l2_table); } /* diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 66f187a74b..3bef410839 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -218,13 +218,10 @@ static int load_refcount_block(BlockDriverState *bs, void **refcount_block) { BDRVQcow2State *s = bs->opaque; - int ret; BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_LOAD); - ret = qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset, - refcount_block); - - return ret; + return qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset, + refcount_block); } /* diff --git a/block/raw-posix.c b/block/raw-posix.c index dbdabc9cf1..bef7a671a6 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -582,11 +582,9 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVRawState *s = bs->opaque; - int ret; s->type = FTYPE_FILE; - ret = raw_open_common(bs, options, flags, 0, errp); - return ret; + return raw_open_common(bs, options, flags, 0, errp); } static int raw_reopen_prepare(BDRVReopenState *state, @@ -2437,13 +2435,11 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVRawState *s = bs->opaque; - 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, errp); - return ret; + return raw_open_common(bs, options, flags, O_NONBLOCK, errp); } static int cdrom_probe_device(const char *filename) diff --git a/block/raw_bsd.c b/block/raw_bsd.c index b51ac98061..7f637914b7 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -190,10 +190,7 @@ static int raw_has_zero_init(BlockDriverState *bs) static int raw_create(const char *filename, QemuOpts *opts, Error **errp) { - int ret; - - ret = bdrv_create_file(filename, opts, errp); - return ret; + return bdrv_create_file(filename, opts, errp); } static int raw_open(BlockDriverState *bs, QDict *options, int flags, diff --git a/block/rbd.c b/block/rbd.c index 5226b6fef8..0a5840d08b 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -883,10 +883,8 @@ static int qemu_rbd_snap_rollback(BlockDriverState *bs, const char *snapshot_name) { BDRVRBDState *s = bs->opaque; - int r; - r = rbd_snap_rollback(s->image, snapshot_name); - return r; + return rbd_snap_rollback(s->image, snapshot_name); } static int qemu_rbd_snap_list(BlockDriverState *bs, diff --git a/block/vmdk.c b/block/vmdk.c index ee09423b46..2901692700 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1260,15 +1260,13 @@ static VmdkExtent *find_extent(BDRVVmdkState *s, static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent, int64_t offset) { - uint64_t offset_in_cluster, extent_begin_offset, extent_relative_offset; + uint64_t extent_begin_offset, extent_relative_offset; uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE; extent_begin_offset = (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE; extent_relative_offset = offset - extent_begin_offset; - offset_in_cluster = extent_relative_offset % cluster_size; - - return offset_in_cluster; + return extent_relative_offset % cluster_size; } static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent, diff --git a/block/vvfat.c b/block/vvfat.c index 6d2e21ce11..5569450616 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -114,15 +114,12 @@ static inline int array_ensure_allocated(array_t* array, int index) static inline void* array_get_next(array_t* array) { unsigned int next = array->next; - void* result; if (array_ensure_allocated(array, next) < 0) return NULL; array->next = next + 1; - result = array_get(array, next); - - return result; + return array_get(array, next); } static inline void* array_insert(array_t* array,unsigned int index,unsigned int count) { diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index 123160a94e..874e473cac 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -324,12 +324,9 @@ static void aml_free(gpointer data, gpointer user_data) Aml *init_aml_allocator(void) { - Aml *var; - assert(!alloc_list); alloc_list = g_ptr_array_new(); - var = aml_alloc(); - return var; + return aml_alloc(); } void free_aml_allocator(void) @@ -451,12 +448,10 @@ Aml *aml_name_decl(const char *name, Aml *val) /* ACPI 1.0b: 16.2.6.1 Arg Objects Encoding */ Aml *aml_arg(int pos) { - Aml *var; uint8_t op = 0x68 /* ARG0 op */ + pos; assert(pos <= 6); - var = aml_opcode(op); - return var; + return aml_opcode(op); } /* ACPI 2.0a: 17.2.4.4 Type 2 Opcodes Encoding: DefToInteger */ @@ -1082,12 +1077,10 @@ Aml *aml_string(const char *name_format, ...) /* ACPI 1.0b: 16.2.6.2 Local Objects Encoding */ Aml *aml_local(int num) { - Aml *var; uint8_t op = 0x60 /* Local0Op */ + num; assert(num <= 7); - var = aml_opcode(op); - return var; + return aml_opcode(op); } /* ACPI 2.0a: 17.2.2 Data Objects Encoding: DefVarPackage */ diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index 93d7669fb5..098b17d020 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -219,10 +219,7 @@ static void intel_hda_reset(DeviceState *dev); static hwaddr intel_hda_addr(uint32_t lbase, uint32_t ubase) { - hwaddr addr; - - addr = ((uint64_t)ubase << 32) | lbase; - return addr; + return ((uint64_t)ubase << 32) | lbase; } static void intel_hda_update_int_sts(IntelHDAState *d) diff --git a/hw/display/vga.c b/hw/display/vga.c index 9ebc54f22b..2a88b3c1b4 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -700,9 +700,7 @@ static void vbe_update_vgaregs(VGACommonState *s) static uint32_t vbe_ioport_read_index(void *opaque, uint32_t addr) { VGACommonState *s = opaque; - uint32_t val; - val = s->vbe_index; - return val; + return s->vbe_index; } uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr) diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c index 680857f0c7..fef808011f 100644 --- a/hw/intc/s390_flic_kvm.c +++ b/hw/intc/s390_flic_kvm.c @@ -195,7 +195,7 @@ static int kvm_s390_register_io_adapter(S390FLICState *fs, uint32_t id, .swap = swap, }; KVMS390FLICState *flic = KVM_S390_FLIC(fs); - int r, ret; + int r; struct kvm_device_attr attr = { .group = KVM_DEV_FLIC_ADAPTER_REGISTER, .addr = (uint64_t)&adapter, @@ -208,8 +208,7 @@ static int kvm_s390_register_io_adapter(S390FLICState *fs, uint32_t id, r = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr); - ret = r ? -errno : 0; - return ret; + return r ? -errno : 0; } static int kvm_s390_io_adapter_map(S390FLICState *fs, uint32_t id, diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c index 15b1054232..7aac4d67a4 100644 --- a/hw/pci-host/uninorth.c +++ b/hw/pci-host/uninorth.c @@ -62,12 +62,9 @@ typedef struct UNINState { static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num) { - int retval; int devfn = pci_dev->devfn & 0x00FFFFFF; - retval = (((devfn >> 11) & 0x1F) + irq_num) & 3; - - return retval; + return (((devfn >> 11) & 0x1F) + irq_num) & 3; } static void pci_unin_set_irq(void *opaque, int irq_num, int level) diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c index 3d9b9c60f4..ae40db8fd2 100644 --- a/hw/ppc/spapr_vio.c +++ b/hw/ppc/spapr_vio.c @@ -57,12 +57,9 @@ static char *spapr_vio_get_dev_name(DeviceState *qdev) { VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev); VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev); - char *name; /* Device tree style name device@reg */ - name = g_strdup_printf("%s@%x", pc->dt_name, dev->reg); - - return name; + return g_strdup_printf("%s@%x", pc->dt_name, dev->reg); } static void spapr_vio_bus_class_init(ObjectClass *klass, void *data) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index a9ffc32682..d1772183cf 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -410,17 +410,14 @@ static void megasas_encode_lba(uint8_t *cdb, uint64_t lba, static uint64_t megasas_fw_time(void) { struct tm curtime; - uint64_t bcd_time; qemu_get_timedate(&curtime, 0); - bcd_time = ((uint64_t)curtime.tm_sec & 0xff) << 48 | + return ((uint64_t)curtime.tm_sec & 0xff) << 48 | ((uint64_t)curtime.tm_min & 0xff) << 40 | ((uint64_t)curtime.tm_hour & 0xff) << 32 | ((uint64_t)curtime.tm_mday & 0xff) << 24 | ((uint64_t)curtime.tm_mon & 0xff) << 16 | ((uint64_t)(curtime.tm_year + 1900) & 0xffff); - - return bcd_time; } /* diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index 71372a8383..6a2d89afba 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -579,10 +579,7 @@ const SCSIReqOps scsi_generic_req_ops = { static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun, uint8_t *buf, void *hba_private) { - SCSIRequest *req; - - req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private); - return req; + return scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private); } static Property scsi_generic_properties[] = { diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c index a11b8b4b21..f4e333eb8f 100644 --- a/hw/timer/mc146818rtc.c +++ b/hw/timer/mc146818rtc.c @@ -105,12 +105,10 @@ static inline bool rtc_running(RTCState *s) static uint64_t get_guest_rtc_ns(RTCState *s) { - uint64_t guest_rtc; uint64_t guest_clock = qemu_clock_get_ns(rtc_clock); - guest_rtc = s->base_rtc * NANOSECONDS_PER_SECOND + + return s->base_rtc * NANOSECONDS_PER_SECOND + guest_clock - s->last_update + s->offset; - return guest_rtc; } #ifdef TARGET_I386 diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index bfedbbf17f..1a0278304b 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -761,9 +761,7 @@ static int kvm_virtio_pci_irqfd_use(VirtIOPCIProxy *proxy, VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); VirtQueue *vq = virtio_get_queue(vdev, queue_no); EventNotifier *n = virtio_queue_get_guest_notifier(vq); - int ret; - ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, irqfd->virq); - return ret; + return kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, irqfd->virq); } static void kvm_virtio_pci_irqfd_release(VirtIOPCIProxy *proxy, diff --git a/linux-user/signal.c b/linux-user/signal.c index 61c1145446..1dadddf2dd 100644 --- a/linux-user/signal.c +++ b/linux-user/signal.c @@ -195,7 +195,6 @@ int block_signals(void) { TaskState *ts = (TaskState *)thread_cpu->opaque; sigset_t set; - int pending; /* It's OK to block everything including SIGSEGV, because we won't * run any further guest code before unblocking signals in @@ -204,9 +203,7 @@ int block_signals(void) sigfillset(&set); sigprocmask(SIG_SETMASK, &set, 0); - pending = atomic_xchg(&ts->signal_pending, 1); - - return pending; + return atomic_xchg(&ts->signal_pending, 1); } /* Wrapper for sigprocmask function @@ -3956,9 +3953,7 @@ static void setup_sigcontext(struct target_sigcontext *sc, static inline unsigned long align_sigframe(unsigned long sp) { - unsigned long i; - i = sp & ~3UL; - return i; + return sp & ~3UL; } static inline abi_ulong get_sigframe(struct target_sigaction *ka, @@ -4555,7 +4550,7 @@ static target_ulong get_sigframe(struct target_sigaction *ka, CPUPPCState *env, int frame_size) { - target_ulong oldsp, newsp; + target_ulong oldsp; oldsp = env->gpr[1]; @@ -4565,9 +4560,7 @@ static target_ulong get_sigframe(struct target_sigaction *ka, + target_sigaltstack_used.ss_size); } - newsp = (oldsp - frame_size) & ~0xFUL; - - return newsp; + return (oldsp - frame_size) & ~0xFUL; } static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame) diff --git a/page_cache.c b/page_cache.c index a2809db2f5..5f8578736e 100644 --- a/page_cache.c +++ b/page_cache.c @@ -111,11 +111,8 @@ void cache_fini(PageCache *cache) static size_t cache_get_cache_pos(const PageCache *cache, uint64_t address) { - size_t pos; - g_assert(cache->max_num_items); - pos = (address / cache->page_size) & (cache->max_num_items - 1); - return pos; + return (address / cache->page_size) & (cache->max_num_items - 1); } static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index eaef7be42e..ea37c097cf 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -127,7 +127,6 @@ int64_t qmp_guest_get_time(Error **errp) { int ret; qemu_timeval tq; - int64_t time_ns; ret = qemu_gettimeofday(&tq); if (ret < 0) { @@ -135,8 +134,7 @@ int64_t qmp_guest_get_time(Error **errp) return -1; } - time_ns = tq.tv_sec * 1000000000LL + tq.tv_usec * 1000; - return time_ns; + return tq.tv_sec * 1000000000LL + tq.tv_usec * 1000; } void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index ea23797c0f..9c9be12116 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -1150,7 +1150,6 @@ out: int64_t qmp_guest_get_time(Error **errp) { SYSTEMTIME ts = {0}; - int64_t time_ns; FILETIME tf; GetSystemTime(&ts); @@ -1164,10 +1163,8 @@ int64_t qmp_guest_get_time(Error **errp) return -1; } - time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime) + return ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime) - W32_FT_OFFSET) * 100; - - return time_ns; } void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp) diff --git a/qobject/qlist.c b/qobject/qlist.c index 1ec74de2b3..86b60cb88c 100644 --- a/qobject/qlist.c +++ b/qobject/qlist.c @@ -100,7 +100,6 @@ QObject *qlist_pop(QList *qlist) QObject *qlist_peek(QList *qlist) { QListEntry *entry; - QObject *ret; if (qlist == NULL || QTAILQ_EMPTY(&qlist->head)) { return NULL; @@ -108,9 +107,7 @@ QObject *qlist_peek(QList *qlist) entry = QTAILQ_FIRST(&qlist->head); - ret = entry->value; - - return ret; + return entry->value; } int qlist_empty(const QList *qlist) diff --git a/scripts/coccinelle/return_directly.cocci b/scripts/coccinelle/return_directly.cocci new file mode 100644 index 0000000000..48680f2c2a --- /dev/null +++ b/scripts/coccinelle/return_directly.cocci @@ -0,0 +1,19 @@ +// replace 'R = X; return R;' with 'return R;' +@@ +identifier VAR; +expression E; +type T; +identifier F; +@@ + T F(...) + { + ... +- T VAR; + ... when != VAR + +- VAR = ++ return + E; +- return VAR; + ... when != VAR + } diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c index 206e60fdf5..929489bfc8 100644 --- a/target-i386/fpu_helper.c +++ b/target-i386/fpu_helper.c @@ -298,18 +298,12 @@ int32_t helper_fistt_ST0(CPUX86State *env) int32_t helper_fisttl_ST0(CPUX86State *env) { - int32_t val; - - val = floatx80_to_int32_round_to_zero(ST0, &env->fp_status); - return val; + return floatx80_to_int32_round_to_zero(ST0, &env->fp_status); } int64_t helper_fisttll_ST0(CPUX86State *env) { - int64_t val; - - val = floatx80_to_int64_round_to_zero(ST0, &env->fp_status); - return val; + return floatx80_to_int64_round_to_zero(ST0, &env->fp_status); } void helper_fldt_ST0(CPUX86State *env, target_ulong ptr) diff --git a/target-i386/kvm.c b/target-i386/kvm.c index ff92b1d118..f3698f19b5 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -1346,7 +1346,7 @@ static int kvm_put_xsave(X86CPU *cpu) CPUX86State *env = &cpu->env; X86XSaveArea *xsave = env->kvm_xsave_buf; uint16_t cwd, swd, twd; - int i, r; + int i; if (!has_xsave) { return kvm_put_fpu(cpu); @@ -1395,8 +1395,7 @@ static int kvm_put_xsave(X86CPU *cpu) 16 * sizeof env->xmm_regs[16]); memcpy(&xsave->pkru_state, &env->pkru, sizeof env->pkru); #endif - r = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_XSAVE, xsave); - return r; + return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_XSAVE, xsave); } static int kvm_put_xcrs(X86CPU *cpu) diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c index 7cf980748e..1ae1dda0af 100644 --- a/target-mips/op_helper.c +++ b/target-mips/op_helper.c @@ -133,10 +133,8 @@ static inline uint64_t get_HILO(CPUMIPSState *env) static inline target_ulong set_HIT0_LO(CPUMIPSState *env, uint64_t HILO) { - target_ulong tmp; env->active_tc.LO[0] = (int32_t)(HILO & 0xFFFFFFFF); - tmp = env->active_tc.HI[0] = (int32_t)(HILO >> 32); - return tmp; + return env->active_tc.HI[0] = (int32_t)(HILO >> 32); } static inline target_ulong set_HI_LOT0(CPUMIPSState *env, uint64_t HILO) diff --git a/target-s390x/helper.c b/target-s390x/helper.c index 9a744df6c8..54a5177345 100644 --- a/target-s390x/helper.c +++ b/target-s390x/helper.c @@ -70,11 +70,7 @@ void s390x_cpu_timer(void *opaque) S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp) { - S390CPU *cpu; - - cpu = S390_CPU(object_new(TYPE_S390_CPU)); - - return cpu; + return S390_CPU(object_new(TYPE_S390_CPU)); } S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp) diff --git a/target-sparc/cc_helper.c b/target-sparc/cc_helper.c index 44c4409346..a410a0b98f 100644 --- a/target-sparc/cc_helper.c +++ b/target-sparc/cc_helper.c @@ -200,10 +200,7 @@ static uint32_t compute_all_addx_xcc(CPUSPARCState *env) static uint32_t compute_C_addx_xcc(CPUSPARCState *env) { - uint32_t ret; - - ret = get_C_addx_xcc(CC_DST, CC_SRC, CC_SRC2); - return ret; + return get_C_addx_xcc(CC_DST, CC_SRC, CC_SRC2); } #endif @@ -219,10 +216,7 @@ static uint32_t compute_all_addx(CPUSPARCState *env) static uint32_t compute_C_addx(CPUSPARCState *env) { - uint32_t ret; - - ret = get_C_addx_icc(CC_DST, CC_SRC, CC_SRC2); - return ret; + return get_C_addx_icc(CC_DST, CC_SRC, CC_SRC2); } static inline uint32_t get_V_tag_icc(target_ulong src1, target_ulong src2) @@ -365,10 +359,7 @@ static uint32_t compute_all_subx_xcc(CPUSPARCState *env) static uint32_t compute_C_subx_xcc(CPUSPARCState *env) { - uint32_t ret; - - ret = get_C_subx_xcc(CC_DST, CC_SRC, CC_SRC2); - return ret; + return get_C_subx_xcc(CC_DST, CC_SRC, CC_SRC2); } #endif @@ -384,10 +375,7 @@ static uint32_t compute_all_subx(CPUSPARCState *env) static uint32_t compute_C_subx(CPUSPARCState *env) { - uint32_t ret; - - ret = get_C_subx_icc(CC_DST, CC_SRC, CC_SRC2); - return ret; + return get_C_subx_icc(CC_DST, CC_SRC, CC_SRC2); } static uint32_t compute_all_tsub(CPUSPARCState *env) @@ -479,8 +467,5 @@ void helper_compute_psr(CPUSPARCState *env) uint32_t helper_compute_C_icc(CPUSPARCState *env) { - uint32_t ret; - - ret = icc_table[CC_OP].compute_c(env) >> PSR_CARRY_SHIFT; - return ret; + return icc_table[CC_OP].compute_c(env) >> PSR_CARRY_SHIFT; } diff --git a/target-tricore/op_helper.c b/target-tricore/op_helper.c index a73ed530f6..3382a126bd 100644 --- a/target-tricore/op_helper.c +++ b/target-tricore/op_helper.c @@ -2117,7 +2117,7 @@ uint64_t helper_dvadj(uint64_t r1, uint32_t r2) int32_t eq_pos = x_sign & ((r1 >> 32) == r2); int32_t eq_neg = x_sign & ((r1 >> 32) == -r2); uint32_t quotient; - uint64_t ret, remainder; + uint64_t remainder; if ((q_sign & ~eq_neg) | eq_pos) { quotient = (r1 + 1) & 0xffffffff; @@ -2130,8 +2130,7 @@ uint64_t helper_dvadj(uint64_t r1, uint32_t r2) } else { remainder = (r1 & 0xffffffff00000000ull); } - ret = remainder|quotient; - return ret; + return remainder | quotient; } uint64_t helper_dvstep(uint64_t r1, uint32_t r2) @@ -2236,7 +2235,6 @@ uint64_t helper_divide_u(CPUTriCoreState *env, uint32_t r1, uint32_t r2) uint64_t helper_mul_h(uint32_t arg00, uint32_t arg01, uint32_t arg10, uint32_t arg11, uint32_t n) { - uint64_t ret; uint32_t result0, result1; int32_t sc1 = ((arg00 & 0xffff) == 0x8000) && @@ -2253,8 +2251,7 @@ uint64_t helper_mul_h(uint32_t arg00, uint32_t arg01, } else { result0 = (((uint32_t)(arg01 * arg11)) << n); } - ret = (((uint64_t)result1 << 32)) | result0; - return ret; + return (((uint64_t)result1 << 32)) | result0; } uint64_t helper_mulm_h(uint32_t arg00, uint32_t arg01, @@ -2308,11 +2305,9 @@ uint32_t helper_mulr_h(uint32_t arg00, uint32_t arg01, uint32_t helper_crc32(uint32_t arg0, uint32_t arg1) { uint8_t buf[4]; - uint32_t ret; stl_be_p(buf, arg0); - ret = crc32(arg1, buf, 4); - return ret; + return crc32(arg1, buf, 4); } /* context save area (CSA) related helpers */ diff --git a/tests/display-vga-test.c b/tests/display-vga-test.c index 06b244ed9a..91460215cc 100644 --- a/tests/display-vga-test.c +++ b/tests/display-vga-test.c @@ -50,8 +50,6 @@ static void pci_virtio_vga(void) int main(int argc, char **argv) { - int ret; - g_test_init(&argc, &argv, NULL); qtest_add_func("/display/pci/cirrus", pci_cirrus); @@ -62,7 +60,5 @@ int main(int argc, char **argv) #ifdef CONFIG_VIRTIO_VGA qtest_add_func("/display/pci/virtio-vga", pci_virtio_vga); #endif - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/tests/endianness-test.c b/tests/endianness-test.c index 2197972e55..b7a120e0a4 100644 --- a/tests/endianness-test.c +++ b/tests/endianness-test.c @@ -282,7 +282,6 @@ static void test_endianness_combine(gconstpointer data) int main(int argc, char **argv) { const char *arch = qtest_get_arch(); - int ret; int i; g_test_init(&argc, &argv, NULL); @@ -305,7 +304,5 @@ int main(int argc, char **argv) qtest_add_data_func(path, &test_cases[i], test_endianness_combine); } - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c index c1d9b3eb9e..3542ad114e 100644 --- a/tests/i440fx-test.c +++ b/tests/i440fx-test.c @@ -394,7 +394,6 @@ static void request_pflash(FirmwareTestFixture *fixture, int main(int argc, char **argv) { TestData data; - int ret; g_test_init(&argc, &argv, NULL); @@ -405,6 +404,5 @@ int main(int argc, char **argv) add_firmware_test("i440fx/firmware/bios", request_bios); add_firmware_test("i440fx/firmware/pflash", request_pflash); - ret = g_test_run(); - return ret; + return g_test_run(); } diff --git a/tests/intel-hda-test.c b/tests/intel-hda-test.c index b0ca7e042a..b782b2e944 100644 --- a/tests/intel-hda-test.c +++ b/tests/intel-hda-test.c @@ -31,13 +31,9 @@ static void ich9_test(void) int main(int argc, char **argv) { - int ret; - g_test_init(&argc, &argv, NULL); qtest_add_func("/intel-hda/ich6", ich6_test); qtest_add_func("/intel-hda/ich9", ich9_test); - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c index 280e4b68ab..c63b68f03a 100644 --- a/tests/test-filter-redirector.c +++ b/tests/test-filter-redirector.c @@ -209,12 +209,8 @@ static void test_redirector_rx(void) int main(int argc, char **argv) { - int ret; - g_test_init(&argc, &argv, NULL); qtest_add_func("/netfilter/redirector_tx", test_redirector_tx); qtest_add_func("/netfilter/redirector_rx", test_redirector_rx); - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c index 8272ba8c42..06dd7fc8eb 100644 --- a/tests/virtio-blk-test.c +++ b/tests/virtio-blk-test.c @@ -763,7 +763,6 @@ static void mmio_basic(void) int main(int argc, char **argv) { - int ret; const char *arch = qtest_get_arch(); g_test_init(&argc, &argv, NULL); @@ -779,7 +778,5 @@ int main(int argc, char **argv) qtest_add_func("/virtio/blk/mmio/basic", mmio_basic); } - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/tests/virtio-console-test.c b/tests/virtio-console-test.c index 6d6414dc8e..1c3de072f4 100644 --- a/tests/virtio-console-test.c +++ b/tests/virtio-console-test.c @@ -27,13 +27,9 @@ static void serialport_pci_nop(void) int main(int argc, char **argv) { - int ret; - g_test_init(&argc, &argv, NULL); qtest_add_func("/virtio/console/pci/nop", console_pci_nop); qtest_add_func("/virtio/serialport/pci/nop", serialport_pci_nop); - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c index e5c144818e..f4f6a4ae7a 100644 --- a/tests/virtio-net-test.c +++ b/tests/virtio-net-test.c @@ -248,8 +248,6 @@ static void hotplug(void) int main(int argc, char **argv) { - int ret; - g_test_init(&argc, &argv, NULL); #ifndef _WIN32 qtest_add_data_func("/virtio/net/pci/basic", send_recv_test, pci_basic); @@ -258,7 +256,5 @@ int main(int argc, char **argv) #endif qtest_add_func("/virtio/net/pci/hotplug", hotplug); - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c index 5f1a8aefeb..b712652fd7 100644 --- a/tests/virtio-scsi-test.c +++ b/tests/virtio-scsi-test.c @@ -260,15 +260,11 @@ static void test_unaligned_write_same(void) int main(int argc, char **argv) { - int ret; - g_test_init(&argc, &argv, NULL); qtest_add_func("/virtio/scsi/pci/nop", pci_nop); qtest_add_func("/virtio/scsi/pci/hotplug", hotplug); qtest_add_func("/virtio/scsi/pci/scsi-disk/unaligned-write-same", test_unaligned_write_same); - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/tests/wdt_ib700-test.c b/tests/wdt_ib700-test.c index 9c1d78b1bc..49f4f0c221 100644 --- a/tests/wdt_ib700-test.c +++ b/tests/wdt_ib700-test.c @@ -117,15 +117,11 @@ static void ib700_none(void) int main(int argc, char **argv) { - int ret; - g_test_init(&argc, &argv, NULL); qtest_add_func("/wdt_ib700/pause", ib700_pause); qtest_add_func("/wdt_ib700/reset", ib700_reset); qtest_add_func("/wdt_ib700/shutdown", ib700_shutdown); qtest_add_func("/wdt_ib700/none", ib700_none); - ret = g_test_run(); - - return ret; + return g_test_run(); } diff --git a/ui/cursor.c b/ui/cursor.c index a276e01f1c..5155b392e8 100644 --- a/ui/cursor.c +++ b/ui/cursor.c @@ -81,18 +81,12 @@ void cursor_print_ascii_art(QEMUCursor *c, const char *prefix) QEMUCursor *cursor_builtin_hidden(void) { - QEMUCursor *c; - - c = cursor_parse_xpm(cursor_hidden_xpm); - return c; + return cursor_parse_xpm(cursor_hidden_xpm); } QEMUCursor *cursor_builtin_left_ptr(void) { - QEMUCursor *c; - - c = cursor_parse_xpm(cursor_left_ptr_xpm); - return c; + return cursor_parse_xpm(cursor_left_ptr_xpm); } QEMUCursor *cursor_alloc(int width, int height) diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c index c9f8dce7f4..6e8b83add6 100644 --- a/ui/qemu-pixman.c +++ b/ui/qemu-pixman.c @@ -180,14 +180,11 @@ void qemu_pixman_linebuf_copy(pixman_image_t *fb, int width, int x, int y, pixman_image_t *qemu_pixman_mirror_create(pixman_format_code_t format, pixman_image_t *image) { - pixman_image_t *mirror; - - mirror = pixman_image_create_bits(format, - pixman_image_get_width(image), - pixman_image_get_height(image), - NULL, - pixman_image_get_stride(image)); - return mirror; + return pixman_image_create_bits(format, + pixman_image_get_width(image), + pixman_image_get_height(image), + NULL, + pixman_image_get_stride(image)); } void qemu_pixman_image_unref(pixman_image_t *image) diff --git a/util/module.c b/util/module.c index ce058aef6f..86e3f7aba0 100644 --- a/util/module.c +++ b/util/module.c @@ -55,13 +55,9 @@ static void init_lists(void) static ModuleTypeList *find_type(module_init_type type) { - ModuleTypeList *l; - init_lists(); - l = &init_type_list[type]; - - return l; + return &init_type_list[type]; } void register_module_init(void (*fn)(void), module_init_type type) diff --git a/vl.c b/vl.c index 18af70afb7..afd5c0be1f 100644 --- a/vl.c +++ b/vl.c @@ -2352,10 +2352,7 @@ static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp) #ifdef CONFIG_VIRTFS static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp) { - int ret; - ret = qemu_fsdev_add(opts); - - return ret; + return qemu_fsdev_add(opts); } #endif -- cgit v1.2.1 From 2ec62faea274aabb2feaad2b8f85961161b5e1e4 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Jun 2016 19:27:14 +0200 Subject: log: Plug memory leak on multiple -dfilter -dfilter overwrites any previous filter. The overwritten filter is leaked. Leaks since the beginning (commit 3514552, v2.6.0). Free it properly. Signed-off-by: Markus Armbruster Message-Id: <1466011636-6112-2-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake --- util/log.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/util/log.c b/util/log.c index 5ad72c197f..6f45e0a26c 100644 --- a/util/log.c +++ b/util/log.c @@ -145,9 +145,16 @@ bool qemu_log_in_addr_range(uint64_t addr) void qemu_set_dfilter_ranges(const char *filter_spec) { gchar **ranges = g_strsplit(filter_spec, ",", 0); + + if (debug_regions) { + g_array_unref(debug_regions); + debug_regions = NULL; + } + if (ranges) { gchar **next = ranges; gchar *r = *next++; + debug_regions = g_array_sized_new(FALSE, FALSE, sizeof(Range), g_strv_length(ranges)); while (r) { -- cgit v1.2.1 From bd6fee9f1263dc5ba487c7ac57d33a727af63c00 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Jun 2016 19:27:15 +0200 Subject: log: Fix qemu_set_dfilter_ranges() error reporting g_error() is not an acceptable way to report errors to the user: $ qemu-system-x86_64 -dfilter 1000+0 ** (process:17187): ERROR **: Failed to parse range in: 1000+0 Trace/breakpoint trap (core dumped) g_assert() isn't, either: $ qemu-system-x86_64 -dfilter 1000x+64 ** ERROR:/work/armbru/qemu/util/log.c:180:qemu_set_dfilter_ranges: assertion failed: (e == range_op) Aborted (core dumped) Convert qemu_set_dfilter_ranges() to Error. Rework its deeply nested control flow. Touch up the error messages. Call it with &error_fatal. This also permits testing without a subprocess, so do that. Signed-off-by: Markus Armbruster Message-Id: <1466011636-6112-3-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake --- include/qemu/log.h | 2 +- tests/test-logging.c | 49 ++++++++-------------- util/log.c | 113 ++++++++++++++++++++++++++------------------------- vl.c | 2 +- 4 files changed, 75 insertions(+), 91 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index 234fa81153..df8d041854 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -107,7 +107,7 @@ extern const QEMULogItem qemu_log_items[]; void qemu_set_log(int log_flags); void qemu_log_needs_buffers(void); void qemu_set_log_filename(const char *filename); -void qemu_set_dfilter_ranges(const char *ranges); +void qemu_set_dfilter_ranges(const char *ranges, Error **errp); bool qemu_log_in_addr_range(uint64_t addr); int qemu_str_to_log_mask(const char *str); diff --git a/tests/test-logging.c b/tests/test-logging.c index 5ef5bb887d..e043adc8f8 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -27,11 +27,14 @@ #include "qemu/osdep.h" #include "qemu-common.h" -#include "include/qemu/log.h" +#include "qapi/error.h" +#include "qemu/log.h" static void test_parse_range(void) { - qemu_set_dfilter_ranges("0x1000+0x100"); + Error *err = NULL; + + qemu_set_dfilter_ranges("0x1000+0x100", &error_abort); g_assert_false(qemu_log_in_addr_range(0xfff)); g_assert(qemu_log_in_addr_range(0x1000)); @@ -39,56 +42,40 @@ static void test_parse_range(void) g_assert(qemu_log_in_addr_range(0x10ff)); g_assert_false(qemu_log_in_addr_range(0x1100)); - qemu_set_dfilter_ranges("0x1000-0x100"); + qemu_set_dfilter_ranges("0x1000-0x100", &error_abort); g_assert_false(qemu_log_in_addr_range(0x1001)); g_assert(qemu_log_in_addr_range(0x1000)); g_assert(qemu_log_in_addr_range(0x0f01)); g_assert_false(qemu_log_in_addr_range(0x0f00)); - qemu_set_dfilter_ranges("0x1000..0x1100"); + qemu_set_dfilter_ranges("0x1000..0x1100", &error_abort); g_assert_false(qemu_log_in_addr_range(0xfff)); g_assert(qemu_log_in_addr_range(0x1000)); g_assert(qemu_log_in_addr_range(0x1100)); g_assert_false(qemu_log_in_addr_range(0x1101)); - qemu_set_dfilter_ranges("0x1000..0x1000"); + qemu_set_dfilter_ranges("0x1000..0x1000", &error_abort); g_assert_false(qemu_log_in_addr_range(0xfff)); g_assert(qemu_log_in_addr_range(0x1000)); g_assert_false(qemu_log_in_addr_range(0x1001)); - qemu_set_dfilter_ranges("0x1000+0x100,0x2100-0x100,0x3000..0x3100"); + qemu_set_dfilter_ranges("0x1000+0x100,0x2100-0x100,0x3000..0x3100", + &error_abort); g_assert(qemu_log_in_addr_range(0x1050)); g_assert(qemu_log_in_addr_range(0x2050)); g_assert(qemu_log_in_addr_range(0x3050)); -} -#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS -static void test_parse_invalid_range_subprocess(void) -{ - qemu_set_dfilter_ranges("0x1000+onehundred"); -} -static void test_parse_invalid_range(void) -{ - g_test_trap_subprocess("/logging/parse_invalid_range/subprocess", 0, 0); - g_test_trap_assert_failed(); - g_test_trap_assert_stdout(""); - g_test_trap_assert_stderr("*Failed to parse range in: 0x1000+onehundred\n"); -} -static void test_parse_zero_range_subprocess(void) -{ - qemu_set_dfilter_ranges("0x1000+0"); -} -static void test_parse_zero_range(void) -{ - g_test_trap_subprocess("/logging/parse_zero_range/subprocess", 0, 0); - g_test_trap_assert_failed(); - g_test_trap_assert_stdout(""); - g_test_trap_assert_stderr("*Failed to parse range in: 0x1000+0\n"); + qemu_set_dfilter_ranges("0x1000+onehundred", &err); + error_free_or_abort(&err); + + qemu_set_dfilter_ranges("0x1000+0", &err); + error_free_or_abort(&err); } +#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS /* As the only real failure from a bad log filename path spec is * reporting to the user we have to use the g_test_trap_subprocess * mechanism and check no errors reported on stderr. @@ -126,10 +113,6 @@ int main(int argc, char **argv) g_test_add_func("/logging/parse_range", test_parse_range); #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS - g_test_add_func("/logging/parse_invalid_range/subprocess", test_parse_invalid_range_subprocess); - g_test_add_func("/logging/parse_invalid_range", test_parse_invalid_range); - g_test_add_func("/logging/parse_zero_range/subprocess", test_parse_zero_range_subprocess); - g_test_add_func("/logging/parse_zero_range", test_parse_zero_range); g_test_add_func("/logging/parse_path", test_parse_path); g_test_add_func("/logging/parse_path/subprocess", test_parse_path_subprocess); g_test_add_func("/logging/parse_invalid_path", test_parse_invalid_path); diff --git a/util/log.c b/util/log.c index 6f45e0a26c..fcf85c6253 100644 --- a/util/log.c +++ b/util/log.c @@ -22,6 +22,7 @@ #include "qemu/log.h" #include "qemu/range.h" #include "qemu/error-report.h" +#include "qapi/error.h" #include "qemu/cutils.h" #include "trace/control.h" @@ -142,75 +143,75 @@ bool qemu_log_in_addr_range(uint64_t addr) } -void qemu_set_dfilter_ranges(const char *filter_spec) +void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) { gchar **ranges = g_strsplit(filter_spec, ",", 0); + int i; if (debug_regions) { g_array_unref(debug_regions); debug_regions = NULL; } - if (ranges) { - gchar **next = ranges; - gchar *r = *next++; + debug_regions = g_array_sized_new(FALSE, FALSE, + sizeof(Range), g_strv_length(ranges)); + for (i = 0; ranges[i]; i++) { + const char *r = ranges[i]; + const char *range_op, *r2, *e; + uint64_t r1val, r2val; + struct Range range; - debug_regions = g_array_sized_new(FALSE, FALSE, - sizeof(Range), g_strv_length(ranges)); - while (r) { - char *range_op = strstr(r, "-"); - char *r2 = range_op ? range_op + 1 : NULL; - if (!range_op) { - range_op = strstr(r, "+"); - r2 = range_op ? range_op + 1 : NULL; - } - if (!range_op) { - range_op = strstr(r, ".."); - r2 = range_op ? range_op + 2 : NULL; - } - if (range_op) { - const char *e = NULL; - uint64_t r1val, r2val; - - if ((qemu_strtoull(r, &e, 0, &r1val) == 0) && - (qemu_strtoull(r2, NULL, 0, &r2val) == 0) && - r2val > 0) { - struct Range range; - - g_assert(e == range_op); + range_op = strstr(r, "-"); + r2 = range_op ? range_op + 1 : NULL; + if (!range_op) { + range_op = strstr(r, "+"); + r2 = range_op ? range_op + 1 : NULL; + } + if (!range_op) { + range_op = strstr(r, ".."); + r2 = range_op ? range_op + 2 : NULL; + } + if (!range_op) { + error_setg(errp, "Bad range specifier"); + goto out; + } - switch (*range_op) { - case '+': - { - range.begin = r1val; - range.end = r1val + (r2val - 1); - break; - } - case '-': - { - range.end = r1val; - range.begin = r1val - (r2val - 1); - break; - } - case '.': - range.begin = r1val; - range.end = r2val; - break; - default: - g_assert_not_reached(); - } - g_array_append_val(debug_regions, range); + if (qemu_strtoull(r, &e, 0, &r1val) + || e != range_op) { + error_setg(errp, "Invalid number to the left of %.*s", + (int)(r2 - range_op), range_op); + goto out; + } + if (qemu_strtoull(r2, NULL, 0, &r2val)) { + error_setg(errp, "Invalid number to the right of %.*s", + (int)(r2 - range_op), range_op); + goto out; + } + if (r2val == 0) { + error_setg(errp, "Invalid range"); + goto out; + } - } else { - g_error("Failed to parse range in: %s", r); - } - } else { - g_error("Bad range specifier in: %s", r); - } - r = *next++; + switch (*range_op) { + case '+': + range.begin = r1val; + range.end = r1val + (r2val - 1); + break; + case '-': + range.end = r1val; + range.begin = r1val - (r2val - 1); + break; + case '.': + range.begin = r1val; + range.end = r2val; + break; + default: + g_assert_not_reached(); } - g_strfreev(ranges); + g_array_append_val(debug_regions, range); } +out: + g_strfreev(ranges); } /* fflush() the log file */ diff --git a/vl.c b/vl.c index afd5c0be1f..749c421234 100644 --- a/vl.c +++ b/vl.c @@ -3339,7 +3339,7 @@ int main(int argc, char **argv, char **envp) log_file = optarg; break; case QEMU_OPTION_DFILTER: - qemu_set_dfilter_ranges(optarg); + qemu_set_dfilter_ranges(optarg, &error_fatal); break; case QEMU_OPTION_s: add_device_config(DEV_GDB, "tcp::" DEFAULT_GDBSTUB_PORT); -- cgit v1.2.1 From daa76aa416b1e18ab1fac650ff53d966d8f21f68 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Jun 2016 19:27:16 +0200 Subject: log: Fix qemu_set_log_filename() error handling When qemu_set_log_filename() detects an invalid file name, it reports an error, closes the log file (if any), and starts logging to stderr (unless daemonized or nothing is being logged). This is wrong. Asking for an invalid log file on the command line should be fatal. Asking for one in the monitor should fail without messing up an existing logfile. Fix by converting qemu_set_log_filename() to Error. Pass it &error_fatal, except for hmp_logfile report errors. This also permits testing without a subprocess, so do that. Signed-off-by: Markus Armbruster Message-Id: <1466011636-6112-4-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake --- bsd-user/main.c | 3 ++- include/qemu/log.h | 2 +- linux-user/main.c | 3 ++- monitor.c | 7 ++++++- tests/test-logging.c | 41 ++++++++--------------------------------- trace/control.c | 3 ++- util/log.c | 6 +++--- vl.c | 2 +- 8 files changed, 25 insertions(+), 42 deletions(-) diff --git a/bsd-user/main.c b/bsd-user/main.c index abe9a26f9b..4819b9ec63 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -19,6 +19,7 @@ #include "qemu/osdep.h" #include +#include "qapi/error.h" #include "qemu.h" #include "qemu/path.h" #include "qemu/help_option.h" @@ -847,7 +848,7 @@ int main(int argc, char **argv) /* init debug */ qemu_log_needs_buffers(); - qemu_set_log_filename(log_file); + qemu_set_log_filename(log_file, &error_fatal); if (log_mask) { int mask; diff --git a/include/qemu/log.h b/include/qemu/log.h index df8d041854..8bec6b4039 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -106,7 +106,7 @@ extern const QEMULogItem qemu_log_items[]; void qemu_set_log(int log_flags); void qemu_log_needs_buffers(void); -void qemu_set_log_filename(const char *filename); +void qemu_set_log_filename(const char *filename, Error **errp); void qemu_set_dfilter_ranges(const char *ranges, Error **errp); bool qemu_log_in_addr_range(uint64_t addr); int qemu_str_to_log_mask(const char *str); diff --git a/linux-user/main.c b/linux-user/main.c index b9a4e0ea45..358ed01e96 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -21,6 +21,7 @@ #include #include +#include "qapi/error.h" #include "qemu.h" #include "qemu/path.h" #include "qemu/cutils.h" @@ -3845,7 +3846,7 @@ static void handle_arg_log(const char *arg) static void handle_arg_log_filename(const char *arg) { - qemu_set_log_filename(arg); + qemu_set_log_filename(arg, &error_fatal); } static void handle_arg_set_env(const char *arg) diff --git a/monitor.c b/monitor.c index a5d054b039..6f960f13a6 100644 --- a/monitor.c +++ b/monitor.c @@ -1111,7 +1111,12 @@ void qmp_client_migrate_info(const char *protocol, const char *hostname, static void hmp_logfile(Monitor *mon, const QDict *qdict) { - qemu_set_log_filename(qdict_get_str(qdict, "filename")); + Error *err = NULL; + + qemu_set_log_filename(qdict_get_str(qdict, "filename"), &err); + if (err) { + error_report_err(err); + } } static void hmp_log(Monitor *mon, const QDict *qdict) diff --git a/tests/test-logging.c b/tests/test-logging.c index e043adc8f8..440e75f5db 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -75,49 +75,24 @@ static void test_parse_range(void) error_free_or_abort(&err); } -#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS -/* As the only real failure from a bad log filename path spec is - * reporting to the user we have to use the g_test_trap_subprocess - * mechanism and check no errors reported on stderr. - */ -static void test_parse_path_subprocess(void) -{ - /* All these should work without issue */ - qemu_set_log_filename("/tmp/qemu.log"); - qemu_set_log_filename("/tmp/qemu-%d.log"); - qemu_set_log_filename("/tmp/qemu.log.%d"); -} static void test_parse_path(void) { - g_test_trap_subprocess ("/logging/parse_path/subprocess", 0, 0); - g_test_trap_assert_passed(); - g_test_trap_assert_stdout(""); - g_test_trap_assert_stderr(""); -} -static void test_parse_invalid_path_subprocess(void) -{ - qemu_set_log_filename("/tmp/qemu-%d%d.log"); -} -static void test_parse_invalid_path(void) -{ - g_test_trap_subprocess ("/logging/parse_invalid_path/subprocess", 0, 0); - g_test_trap_assert_passed(); - g_test_trap_assert_stdout(""); - g_test_trap_assert_stderr("Bad logfile format: /tmp/qemu-%d%d.log\n"); + Error *err = NULL; + + qemu_set_log_filename("/tmp/qemu.log", &error_abort); + qemu_set_log_filename("/tmp/qemu-%d.log", &error_abort); + qemu_set_log_filename("/tmp/qemu.log.%d", &error_abort); + + qemu_set_log_filename("/tmp/qemu-%d%d.log", &err); + error_free_or_abort(&err); } -#endif /* CONFIG_HAS_GLIB_SUBPROCESS_TESTS */ int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); g_test_add_func("/logging/parse_range", test_parse_range); -#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS g_test_add_func("/logging/parse_path", test_parse_path); - g_test_add_func("/logging/parse_path/subprocess", test_parse_path_subprocess); - g_test_add_func("/logging/parse_invalid_path", test_parse_invalid_path); - g_test_add_func("/logging/parse_invalid_path/subprocess", test_parse_invalid_path_subprocess); -#endif return g_test_run(); } diff --git a/trace/control.c b/trace/control.c index d099f735d5..e1556a3570 100644 --- a/trace/control.c +++ b/trace/control.c @@ -19,6 +19,7 @@ #ifdef CONFIG_TRACE_LOG #include "qemu/log.h" #endif +#include "qapi/error.h" #include "qemu/error-report.h" #include "monitor/monitor.h" @@ -187,7 +188,7 @@ void trace_init_file(const char *file) * only applies to the simple backend; use "-D" for the log backend. */ if (file) { - qemu_set_log_filename(file); + qemu_set_log_filename(file, &error_fatal); } #else if (file) { diff --git a/util/log.c b/util/log.c index fcf85c6253..32e416051c 100644 --- a/util/log.c +++ b/util/log.c @@ -103,7 +103,7 @@ void qemu_log_needs_buffers(void) * substituted with the current PID. This is useful for debugging many * nested linux-user tasks but will result in lots of logs. */ -void qemu_set_log_filename(const char *filename) +void qemu_set_log_filename(const char *filename, Error **errp) { char *pidstr; g_free(logfilename); @@ -112,8 +112,8 @@ void qemu_set_log_filename(const char *filename) if (pidstr) { /* We only accept one %d, no other format strings */ if (pidstr[1] != 'd' || strchr(pidstr + 2, '%')) { - error_report("Bad logfile format: %s", filename); - logfilename = NULL; + error_setg(errp, "Bad logfile format: %s", filename); + return; } else { logfilename = g_strdup_printf(filename, getpid()); } diff --git a/vl.c b/vl.c index 749c421234..c85833a63c 100644 --- a/vl.c +++ b/vl.c @@ -4054,7 +4054,7 @@ int main(int argc, char **argv, char **envp) /* Open the logfile at this point and set the log mask if necessary. */ if (log_file) { - qemu_set_log_filename(log_file); + qemu_set_log_filename(log_file, &error_fatal); } if (log_mask) { -- cgit v1.2.1