When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right away. Coccinelle script:
@@
identifier fun, err, errp;
expression list args;
@@
- fun(args, &err);
+ fun(args, errp);
... when != err
when strict
- error_propagate(errp, err);
@@
identifier fun, err, errp;
expression list args;
expression ret;
@@
- ret = fun(args, &err);
+ ret = fun(args, errp);
... when != err
when strict
- error_propagate(errp, err);
@@
identifier fun, err, errp;
expression list args;
expression ret;
@@
- ret = fun(args, &err);
+ ret = fun(args, errp);
... when != err
when strict
if (
(
ret
|
!ret
|
ret == 0
|
ret != 0
|
ret < 0
|
ret != NULL
|
ret == NULL
)
)
{
... when != err
when strict
- error_propagate(errp, err);
...
}
@@
identifier fun, err, errp;
expression list args;
@@
if (
(
- fun(args, &err)
+ fun(args, errp)
|
- !fun(args, &err)
+ !fun(args, errp)
|
- fun(args, &err) == 0
+ fun(args, errp) == 0
|
- fun(args, &err) != 0
+ fun(args, errp) != 0
|
- fun(args, &err) < 0
+ fun(args, errp) < 0
|
- fun(args, &err) == NULL
+ fun(args, errp) == NULL
|
- fun(args, &err) != NULL
+ fun(args, errp) != NULL
)
)
{
... when != err;
when strict
- error_propagate(errp, err);
...
}
The first two rules are prone to fail with "error_propagate(...)
... reachable by inconsistent control-flow paths". Script without
them re-run where that happens.
Manually double-check @err is not used afterwards. Dereferencing it
would be use after free, but checking whether it's null would be
legitimate. One such change to qbus_realize() backed out.
Suboptimal line breaks tweaked manually.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
backends/cryptodev-vhost-user.c | 3 +--
block.c | 3 +--
block/file-posix.c | 8 ++------
block/parallels.c | 3 +--
block/qcow.c | 3 +--
block/qcow2.c | 10 +++-------
block/qed.c | 3 +--
block/replication.c | 4 +---
block/vhdx.c | 3 +--
block/vmdk.c | 7 ++-----
block/vpc.c | 3 +--
blockdev.c | 10 +++-------
dump/dump.c | 7 ++-----
hw/intc/xics_kvm.c | 3 +--
hw/s390x/s390-pci-bus.c | 4 +---
hw/usb/bus.c | 4 +---
hw/vfio/pci.c | 10 +++-------
iothread.c | 3 +--
qdev-monitor.c | 3 +--
qga/commands-win32.c | 3 +--
util/main-loop.c | 4 +---
util/qemu-option.c | 3 +--
22 files changed, 31 insertions(+), 73 deletions(-)
diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
index 8b8cbc4223..dbe5a8aae6 100644
--- a/backends/cryptodev-vhost-user.c
+++ b/backends/cryptodev-vhost-user.c
@@ -209,8 +209,7 @@ static void cryptodev_vhost_user_init(
backend->conf.peers.ccs[i] = cc;
if (i == 0) {
- if (!qemu_chr_fe_init(&s->chr, chr, &local_err)) {
- error_propagate(errp, local_err);
+ if (!qemu_chr_fe_init(&s->chr, chr, errp)) {
return;
}
}
diff --git a/block.c b/block.c
index 6dbcb7e083..30a72bc4c2 100644
--- a/block.c
+++ b/block.c
@@ -5680,10 +5680,9 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
if (bs->open_flags & BDRV_O_INACTIVE) {
bs->open_flags &= ~BDRV_O_INACTIVE;
bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
- ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &local_err);
+ ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
if (ret < 0) {
bs->open_flags |= BDRV_O_INACTIVE;
- error_propagate(errp, local_err);
return;
}
bdrv_set_perm(bs, perm, shared_perm);
diff --git a/block/file-posix.c b/block/file-posix.c
index 3ab8f5a0fa..2294bf5d00 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3336,7 +3336,6 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVRawState *s = bs->opaque;
- Error *local_err = NULL;
int ret;
#if defined(__APPLE__) && defined(__MACH__)
@@ -3401,9 +3400,8 @@ hdev_open_Mac_error:
s->type = FTYPE_FILE;
- ret = raw_open_common(bs, options, flags, 0, true, &local_err);
+ ret = raw_open_common(bs, options, flags, 0, true, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
#if defined(__APPLE__) && defined(__MACH__)
if (*bsd_path) {
filename = bsd_path;
@@ -3679,14 +3677,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;
- ret = raw_open_common(bs, options, flags, 0, true, &local_err);
+ ret = raw_open_common(bs, options, flags, 0, true, errp);
if (ret) {
- error_propagate(errp, local_err);
return ret;
}
diff --git a/block/parallels.c b/block/parallels.c
index 63a1cde8af..a84ec6a518 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -646,9 +646,8 @@ static int coroutine_fn parallels_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, &local_err);
+ ret = bdrv_create_file(filename, opts, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
goto done;
}
diff --git a/block/qcow.c b/block/qcow.c
index ee5d35fe20..dca2a1fe7d 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -973,9 +973,8 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, &local_err);
+ ret = bdrv_create_file(filename, opts, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 0cd2e6757e..e81c284319 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1583,8 +1583,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
/* read qcow2 extensions */
if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
- flags, &update_header, &local_err)) {
- error_propagate(errp, local_err);
+ flags, &update_header, errp)) {
ret = -EINVAL;
goto fail;
}
@@ -3356,7 +3355,6 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
int version;
int refcount_order;
uint64_t* refcount_table;
- Error *local_err = NULL;
int ret;
uint8_t compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
@@ -3582,9 +3580,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
}
blk = blk_new_open(NULL, NULL, options,
BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_NO_FLUSH,
- &local_err);
+ errp);
if (blk == NULL) {
- error_propagate(errp, local_err);
ret = -EIO;
goto out;
}
@@ -3664,9 +3661,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
}
blk = blk_new_open(NULL, NULL, options,
BDRV_O_RDWR | BDRV_O_NO_BACKING | BDRV_O_NO_IO,
- &local_err);
+ errp);
if (blk == NULL) {
- error_propagate(errp, local_err);
ret = -EIO;
goto out;
}
diff --git a/block/qed.c b/block/qed.c
index c0c65015c7..e369fd360a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -749,9 +749,8 @@ static int coroutine_fn bdrv_qed_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, &local_err);
+ ret = bdrv_create_file(filename, opts, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/replication.c b/block/replication.c
index ccf7b78160..aa7164dbe3 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -369,7 +369,6 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
{
BDRVReplicationState *s = bs->opaque;
BlockReopenQueue *reopen_queue = NULL;
- Error *local_err = NULL;
if (writable) {
s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs);
@@ -394,8 +393,7 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
}
if (reopen_queue) {
- bdrv_reopen_multiple(reopen_queue, &local_err);
- error_propagate(errp, local_err);
+ bdrv_reopen_multiple(reopen_queue, errp);
}
bdrv_subtree_drained_end(s->hidden_disk->bs);
diff --git a/block/vhdx.c b/block/vhdx.c
index fa9e544a5e..ac5a9094c4 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2086,9 +2086,8 @@ static int coroutine_fn vhdx_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, &local_err);
+ ret = bdrv_create_file(filename, opts, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/vmdk.c b/block/vmdk.c
index 62da465126..9a09193f3b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2250,19 +2250,16 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
{
int ret;
BlockBackend *blk = NULL;
- Error *local_err = NULL;
- ret = bdrv_create_file(filename, opts, &local_err);
+ ret = bdrv_create_file(filename, opts, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
goto exit;
}
blk = blk_new_open(filename, NULL, NULL,
BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- &local_err);
+ errp);
if (blk == NULL) {
- error_propagate(errp, local_err);
ret = -EIO;
goto exit;
}
diff --git a/block/vpc.c b/block/vpc.c
index c055591641..36412f764d 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1115,9 +1115,8 @@ static int coroutine_fn vpc_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, &local_err);
+ ret = bdrv_create_file(filename, opts, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/blockdev.c b/blockdev.c
index 31d5eaf6bf..b66863c42a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3147,9 +3147,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
arg->has_copy_mode, arg->copy_mode,
arg->has_auto_finalize, arg->auto_finalize,
arg->has_auto_dismiss, arg->auto_dismiss,
- &local_err);
+ errp);
bdrv_unref(target_bs);
- error_propagate(errp, local_err);
out:
aio_context_release(aio_context);
}
@@ -3177,7 +3176,6 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
AioContext *aio_context;
AioContext *old_context;
BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN;
- Error *local_err = NULL;
bool zero_target;
int ret;
@@ -3219,8 +3217,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
has_copy_mode, copy_mode,
has_auto_finalize, auto_finalize,
has_auto_dismiss, auto_dismiss,
- &local_err);
- error_propagate(errp, local_err);
+ errp);
out:
aio_context_release(aio_context);
}
@@ -3439,8 +3436,7 @@ void qmp_change_backing_file(const char *device,
}
if (ro) {
- bdrv_reopen_set_read_only(image_bs, true, &local_err);
- error_propagate(errp, local_err);
+ bdrv_reopen_set_read_only(image_bs, true, errp);
}
out:
diff --git a/dump/dump.c b/dump/dump.c
index 248ea06370..383bc7876b 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1031,14 +1031,11 @@ out:
static void write_dump_header(DumpState *s, Error **errp)
{
- Error *local_err = NULL;
-
if (s->dump_info.d_class == ELFCLASS32) {
- create_header32(s, &local_err);
+ create_header32(s, errp);
} else {
- create_header64(s, &local_err);
+ create_header64(s, errp);
}
- error_propagate(errp, local_err);
}
static size_t dump_bitmap_get_bufsize(DumpState *s)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 8d6156578f..6705220380 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -316,9 +316,8 @@ int ics_set_kvm_state(ICSState *ics, Error **errp)
continue;
}
- ret = ics_set_kvm_state_one(ics, i, &local_err);
+ ret = ics_set_kvm_state_one(ics, i, errp);
if (ret < 0) {
- error_propagate(errp, local_err);
return ret;
}
}
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 0517901024..be8535304e 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -741,7 +741,6 @@ static void s390_pcihost_realize(DeviceState *dev, Error **errp)
BusState *bus;
PCIHostState *phb = PCI_HOST_BRIDGE(dev);
S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
- Error *local_err = NULL;
DPRINTF("host_init\n");
@@ -765,8 +764,7 @@ static void s390_pcihost_realize(DeviceState *dev, Error **errp)
QTAILQ_INIT(&s->zpci_devs);
css_register_io_adapters(CSS_IO_ADAPTER_PCI, true, false,
- S390_ADAPTER_SUPPRESSIBLE, &local_err);
- error_propagate(errp, local_err);
+ S390_ADAPTER_SUPPRESSIBLE, errp);
}
static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index ba27afe9f2..b17bda3b29 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -723,15 +723,13 @@ static bool usb_get_attached(Object *obj, Error **errp)
static void usb_set_attached(Object *obj, bool value, Error **errp)
{
USBDevice *dev = USB_DEVICE(obj);
- Error *err = NULL;
if (dev->attached == value) {
return;
}
if (value) {
- usb_device_attach(dev, &err);
- error_propagate(errp, err);
+ usb_device_attach(dev, errp);
} else {
usb_device_detach(dev);
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 6838bcc4b3..fb51fc9f6e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -116,7 +116,6 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
{
#ifdef CONFIG_KVM
int irq_fd = event_notifier_get_fd(&vdev->intx.interrupt);
- Error *err = NULL;
if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
vdev->intx.route.mode != PCI_INTX_ENABLED ||
@@ -147,8 +146,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
VFIO_IRQ_SET_ACTION_UNMASK,
event_notifier_get_fd(&vdev->intx.unmask),
- &err)) {
- error_propagate(errp, err);
+ errp)) {
goto fail_vfio;
}
@@ -294,8 +292,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev);
if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
- error_propagate(errp, err);
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
qemu_set_fd_handler(fd, NULL, NULL, vdev);
event_notifier_cleanup(&vdev->intx.interrupt);
return -errno;
@@ -2741,9 +2738,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (!pdev->failover_pair_id) {
error_setg(&vdev->migration_blocker,
"VFIO device doesn't support migration");
- ret = migrate_add_blocker(vdev->migration_blocker, &err);
+ ret = migrate_add_blocker(vdev->migration_blocker, errp);
if (ret) {
- error_propagate(errp, err);
error_free(vdev->migration_blocker);
vdev->migration_blocker = NULL;
return;
diff --git a/iothread.c b/iothread.c
index cb082b9b26..6f7ac976de 100644
--- a/iothread.c
+++ b/iothread.c
@@ -169,9 +169,8 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
iothread->stopping = false;
iothread->running = true;
- iothread->ctx = aio_context_new(&local_error);
+ iothread->ctx = aio_context_new(errp);
if (!iothread->ctx) {
- error_propagate(errp, local_error);
return;
}
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 13a13a811a..e38030429b 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -808,9 +808,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
qemu_opts_del(opts);
return;
}
- dev = qdev_device_add(opts, &local_err);
+ dev = qdev_device_add(opts, errp);
if (!dev) {
- error_propagate(errp, local_err);
qemu_opts_del(opts);
return;
}
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 5ba56327dd..49dd792d2c 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2202,9 +2202,8 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
}
server = os_version.wProductType != VER_NT_WORKSTATION;
- product_name = ga_get_win_product_name(&local_err);
+ product_name = ga_get_win_product_name(errp);
if (product_name == NULL) {
- error_propagate(errp, local_err);
return NULL;
}
diff --git a/util/main-loop.c b/util/main-loop.c
index eda63fe4e0..f69f055013 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -148,7 +148,6 @@ int qemu_init_main_loop(Error **errp)
{
int ret;
GSource *src;
- Error *local_error = NULL;
init_clocks(qemu_timer_notify_cb);
@@ -157,9 +156,8 @@ int qemu_init_main_loop(Error **errp)
return ret;
}
- qemu_aio_context = aio_context_new(&local_error);
+ qemu_aio_context = aio_context_new(errp);
if (!qemu_aio_context) {
- error_propagate(errp, local_error);
return -EMFILE;
}
qemu_notify_bh = qemu_bh_new(notify_event_cb, NULL);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 0ebfd97a98..fd667ea523 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -899,10 +899,9 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
* (if unlikely) future misuse:
*/
assert(!defaults || list->merge_lists);
- opts = qemu_opts_create(list, id, !defaults, &local_err);
+ opts = qemu_opts_create(list, id, !defaults, errp);
g_free(id);
if (opts == NULL) {
- error_propagate(errp, local_err);
return NULL;
}
--
2.26.2
On 6/24/20 11:43 AM, Markus Armbruster wrote: > When all we do with an Error we receive into a local variable is > propagating to somewhere else, we can just as well receive it there > right away. Coccinelle script: This seems to be a recurring cleanup (witness commit 06592d7e, c0e90679, 6b62d961). In fact, shouldn't you just update that script with your enhancements here, and then run it directly, instead of embedding your tweaks in the commit message? > > @@ > identifier fun, err, errp; > expression list args; > @@ > - fun(args, &err); > + fun(args, errp); > ... when != err > when strict > - error_propagate(errp, err); What does the 'when strict' accomplish? The existing coccinelle script uses 'when != errp', which may be enough to address... > > The first two rules are prone to fail with "error_propagate(...) > ... reachable by inconsistent control-flow paths". Script without > them re-run where that happens. ...the control-flow failures you hit? > > Manually double-check @err is not used afterwards. Dereferencing it > would be use after free, but checking whether it's null would be > legitimate. One such change to qbus_realize() backed out. > > Suboptimal line breaks tweaked manually. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > 22 files changed, 31 insertions(+), 73 deletions(-) At any rate, it's small enough to ensure all the changes remaining are still valid. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On 6/24/20 11:43 AM, Markus Armbruster wrote:
>> When all we do with an Error we receive into a local variable is
>> propagating to somewhere else, we can just as well receive it there
>> right away. Coccinelle script:
>
> This seems to be a recurring cleanup (witness commit 06592d7e,
> c0e90679, 6b62d961). In fact, shouldn't you just update that script
> with your enhancements here, and then run it directly, instead of
> embedding your tweaks in the commit message?
I didn't remember scripts/coccinelle/remove_local_err.cocci.
remove_local_err.cocci transforms
fun(..., &err);
error_propagate(errp, err);
to
fun(..., errp);
when the context permits that, using a conservative approximation of
"context permits". In other words, the script is sound.
My script matches more, but is unsound: it *can* mess up things. For
instance
Error err = NULL;
foo(1, 2, 3, &err);
error_propagate(errp, err);
return !err;
would become
Error err = NULL;
foo(1, 2, 3, errp);
return !err;
Oops. See "manually double-check" below. For a one-shot cleanup,
manual checking is much, much easier for me than making the Coccinelle
script sound without sacrificing (too much) matching power.
>
>>
>> @@
>> identifier fun, err, errp;
>> expression list args;
>> @@
>> - fun(args, &err);
>> + fun(args, errp);
>> ... when != err
>> when strict
>> - error_propagate(errp, err);
>
> What does the 'when strict' accomplish? The existing coccinelle
> script uses 'when != errp', which may be enough to address...
Magic! Without it, I get
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8310c77ec0..2355999171 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1511,16 +1511,14 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
ret = msix_init(&vdev->pdev, vdev->msix->entries,
vdev->bars[vdev->msix->table_bar].mr,
vdev->msix->table_bar, vdev->msix->table_offset,
- vdev->bars[vdev->msix->pba_bar].mr,
- vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
- &err);
+ vdev->bars[vdev->msix->pba_bar].mr, vdev->msix->pba_bar,
+ vdev->msix->pba_offset, pos, errp);
if (ret < 0) {
if (ret == -ENOTSUP) {
WTF!?! ---> warn_report_err(err);
return 0;
}
- error_propagate(errp, err);
return ret;
}
Since Coccinelle's documentation is of no help whatsoever (again), I
went looking for possibly applicable ideas in existing scripts. I found
"when strict" in the kernel's scripts/coccinelle/ directory. I couldn't
find it in Coccinelle docs. So I gave it a try, and here we are.
Did I mention Coccinelle is terrible?
>> The first two rules are prone to fail with "error_propagate(...)
>> ... reachable by inconsistent control-flow paths". Script without
>> them re-run where that happens.
>
> ...the control-flow failures you hit?
This one I can actually explain, I think. Consider this code snippet
from net.c:
1 visit_type_Netdev(v, NULL, &object, &err);
2 if (!err) {
3 ret = net_client_init1(object, is_netdev, &err);
4 }
5 qapi_free_Netdev(object);
6 out:
7 error_propagate(errp, err);
There are two overlapping matches of rule 1: either line 1 and 7, or
line 3 and 7. Which one is right? Coccinelle can't decide, and
implodes:
rule starting on line 10: node 96: error_propagate(...)[1,2,41,42] in net_client_init reachable by inconsistent control-flow paths
>>
>> Manually double-check @err is not used afterwards. Dereferencing it
>> would be use after free, but checking whether it's null would be
>> legitimate. One such change to qbus_realize() backed out.
>>
>> Suboptimal line breaks tweaked manually.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> 22 files changed, 31 insertions(+), 73 deletions(-)
>
> At any rate, it's small enough to ensure all the changes remaining are
> still valid.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
24.06.2020 19:43, Markus Armbruster wrote:
> When all we do with an Error we receive into a local variable is
> propagating to somewhere else, we can just as well receive it there
> right away. Coccinelle script:
>
> @@
> identifier fun, err, errp;
> expression list args;
> @@
> - fun(args, &err);
> + fun(args, errp);
> ... when != err
> when strict
> - error_propagate(errp, err);
>
> @@
> identifier fun, err, errp;
> expression list args;
> expression ret;
> @@
> - ret = fun(args, &err);
> + ret = fun(args, errp);
> ... when != err
> when strict
> - error_propagate(errp, err);
>
> @@
> identifier fun, err, errp;
> expression list args;
> expression ret;
> @@
> - ret = fun(args, &err);
> + ret = fun(args, errp);
> ... when != err
> when strict
> if (
> (
> ret
> |
> !ret
> |
> ret == 0
> |
> ret != 0
> |
> ret < 0
> |
> ret != NULL
> |
> ret == NULL
> )
> )
> {
> ... when != err
> when strict
> - error_propagate(errp, err);
> ...
> }
>
> @@
> identifier fun, err, errp;
> expression list args;
> @@
> if (
> (
> - fun(args, &err)
> + fun(args, errp)
> |
> - !fun(args, &err)
> + !fun(args, errp)
> |
> - fun(args, &err) == 0
> + fun(args, errp) == 0
> |
> - fun(args, &err) != 0
> + fun(args, errp) != 0
> |
> - fun(args, &err) < 0
> + fun(args, errp) < 0
> |
> - fun(args, &err) == NULL
> + fun(args, errp) == NULL
> |
> - fun(args, &err) != NULL
> + fun(args, errp) != NULL
> )
> )
> {
> ... when != err;
> when strict
> - error_propagate(errp, err);
> ...
> }
>
> The first two rules are prone to fail with "error_propagate(...)
> ... reachable by inconsistent control-flow paths". Script without
> them re-run where that happens.
>
> Manually double-check @err is not used afterwards. Dereferencing it
> would be use after free, but checking whether it's null would be
> legitimate. One such change to qbus_realize() backed out.
>
> Suboptimal line breaks tweaked manually.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
[..]
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 8d6156578f..6705220380 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -316,9 +316,8 @@ int ics_set_kvm_state(ICSState *ics, Error **errp)
> continue;
> }
local_err becomes unused in this function, we should drop it
with this fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> - ret = ics_set_kvm_state_one(ics, i, &local_err);
> + ret = ics_set_kvm_state_one(ics, i, errp);
> if (ret < 0) {
> - error_propagate(errp, local_err);
> return ret;
> }
> }
--
Best regards,
Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 24.06.2020 19:43, Markus Armbruster wrote:
>> When all we do with an Error we receive into a local variable is
>> propagating to somewhere else, we can just as well receive it there
>> right away. Coccinelle script:
>>
>> @@
>> identifier fun, err, errp;
>> expression list args;
>> @@
>> - fun(args, &err);
>> + fun(args, errp);
>> ... when != err
>> when strict
>> - error_propagate(errp, err);
>>
>> @@
>> identifier fun, err, errp;
>> expression list args;
>> expression ret;
>> @@
>> - ret = fun(args, &err);
>> + ret = fun(args, errp);
>> ... when != err
>> when strict
>> - error_propagate(errp, err);
>>
>> @@
>> identifier fun, err, errp;
>> expression list args;
>> expression ret;
>> @@
>> - ret = fun(args, &err);
>> + ret = fun(args, errp);
>> ... when != err
>> when strict
>> if (
>> (
>> ret
>> |
>> !ret
>> |
>> ret == 0
>> |
>> ret != 0
>> |
>> ret < 0
>> |
>> ret != NULL
>> |
>> ret == NULL
>> )
>> )
>> {
>> ... when != err
>> when strict
>> - error_propagate(errp, err);
>> ...
>> }
>>
>> @@
>> identifier fun, err, errp;
>> expression list args;
>> @@
>> if (
>> (
>> - fun(args, &err)
>> + fun(args, errp)
>> |
>> - !fun(args, &err)
>> + !fun(args, errp)
>> |
>> - fun(args, &err) == 0
>> + fun(args, errp) == 0
>> |
>> - fun(args, &err) != 0
>> + fun(args, errp) != 0
>> |
>> - fun(args, &err) < 0
>> + fun(args, errp) < 0
>> |
>> - fun(args, &err) == NULL
>> + fun(args, errp) == NULL
>> |
>> - fun(args, &err) != NULL
>> + fun(args, errp) != NULL
>> )
>> )
>> {
>> ... when != err;
>> when strict
>> - error_propagate(errp, err);
>> ...
>> }
>>
>> The first two rules are prone to fail with "error_propagate(...)
>> ... reachable by inconsistent control-flow paths". Script without
>> them re-run where that happens.
>>
>> Manually double-check @err is not used afterwards. Dereferencing it
>> would be use after free, but checking whether it's null would be
>> legitimate. One such change to qbus_realize() backed out.
>>
>> Suboptimal line breaks tweaked manually.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> [..]
>
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> index 8d6156578f..6705220380 100644
>> --- a/hw/intc/xics_kvm.c
>> +++ b/hw/intc/xics_kvm.c
>> @@ -316,9 +316,8 @@ int ics_set_kvm_state(ICSState *ics, Error **errp)
>> continue;
>> }
>
> local_err becomes unused in this function, we should drop it
Will fix.
> with this fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Thanks!
>
>> - ret = ics_set_kvm_state_one(ics, i, &local_err);
>> + ret = ics_set_kvm_state_one(ics, i, errp);
>> if (ret < 0) {
>> - error_propagate(errp, local_err);
>> return ret;
>> }
>> }
© 2016 - 2025 Red Hat, Inc.