From 4b5766488fd3549dc47a75331cf4db62f477536c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:25 +0200 Subject: error: Fix use of error_prepend() with &error_fatal, &error_abort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From include/qapi/error.h: * Pass an existing error to the caller with the message modified: * error_propagate(errp, err); * error_prepend(errp, "Could not frobnicate '%s': ", name); Fei Li pointed out that doing error_propagate() first doesn't work well when @errp is &error_fatal or &error_abort: the error_prepend() is never reached. Since I doubt fixing the documentation will stop people from getting it wrong, introduce error_propagate_prepend(), in the hope that it lures people away from using its constituents in the wrong order. Update the instructions in error.h accordingly. Convert existing error_prepend() next to error_propagate to error_propagate_prepend(). If any of these get reached with &error_fatal or &error_abort, the error messages improve. I didn't check whether that's the case anywhere. Cc: Fei Li Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Eric Blake Message-Id: <20181017082702.5581-2-armbru@redhat.com> --- hw/9pfs/9p-local.c | 4 ++-- hw/intc/xics.c | 15 +++++++++------ hw/ppc/pnv_core.c | 4 ++-- hw/ppc/spapr_pci.c | 7 +++---- hw/timer/aspeed_timer.c | 3 +-- hw/usb/bus.c | 5 +++-- hw/vfio/pci.c | 3 +-- 7 files changed, 21 insertions(+), 20 deletions(-) (limited to 'hw') diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index c30f4f26bd..08e673a79c 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1509,8 +1509,8 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp) fsdev_throttle_parse_opts(opts, &fse->fst, &local_err); if (local_err) { - error_propagate(errp, local_err); - error_prepend(errp, "invalid throttle configuration: "); + error_propagate_prepend(errp, local_err, + "invalid throttle configuration: "); return -1; } diff --git a/hw/intc/xics.c b/hw/intc/xics.c index c90c893228..406efee064 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -320,8 +320,9 @@ static void icp_realize(DeviceState *dev, Error **errp) obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err); if (!obj) { - error_propagate(errp, err); - error_prepend(errp, "required link '" ICP_PROP_XICS "' not found: "); + error_propagate_prepend(errp, err, + "required link '" ICP_PROP_XICS + "' not found: "); return; } @@ -329,8 +330,9 @@ static void icp_realize(DeviceState *dev, Error **errp) obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err); if (!obj) { - error_propagate(errp, err); - error_prepend(errp, "required link '" ICP_PROP_CPU "' not found: "); + error_propagate_prepend(errp, err, + "required link '" ICP_PROP_CPU + "' not found: "); return; } @@ -624,8 +626,9 @@ static void ics_base_realize(DeviceState *dev, Error **errp) obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err); if (!obj) { - error_propagate(errp, err); - error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: "); + error_propagate_prepend(errp, err, + "required link '" ICS_PROP_XICS + "' not found: "); return; } ics->xics = XICS_FABRIC(obj); diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c index 9750464bf4..ad1bcc7990 100644 --- a/hw/ppc/pnv_core.c +++ b/hw/ppc/pnv_core.c @@ -148,8 +148,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) chip = object_property_get_link(OBJECT(dev), "chip", &local_err); if (!chip) { - error_propagate(errp, local_err); - error_prepend(errp, "required link 'chip' not found: "); + error_propagate_prepend(errp, local_err, + "required link 'chip' not found: "); return; } diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index c2271e6ed4..58afa46204 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1724,16 +1724,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) if (smc->legacy_irq_allocation) { irq = spapr_irq_findone(spapr, &local_err); if (local_err) { - error_propagate(errp, local_err); - error_prepend(errp, "can't allocate LSIs: "); + error_propagate_prepend(errp, local_err, + "can't allocate LSIs: "); return; } } spapr_irq_claim(spapr, irq, true, &local_err); if (local_err) { - error_propagate(errp, local_err); - error_prepend(errp, "can't allocate LSIs: "); + error_propagate_prepend(errp, local_err, "can't allocate LSIs: "); return; } diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c index 54b400b94a..5c786e5128 100644 --- a/hw/timer/aspeed_timer.c +++ b/hw/timer/aspeed_timer.c @@ -454,8 +454,7 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp) obj = object_property_get_link(OBJECT(dev), "scu", &err); if (!obj) { - error_propagate(errp, err); - error_prepend(errp, "required link 'scu' not found: "); + error_propagate_prepend(errp, err, "required link 'scu' not found: "); return; } s->scu = ASPEED_SCU(obj); diff --git a/hw/usb/bus.c b/hw/usb/bus.c index 11f7720d71..bf796d67e6 100644 --- a/hw/usb/bus.c +++ b/hw/usb/bus.c @@ -340,8 +340,9 @@ static USBDevice *usb_try_create_simple(USBBus *bus, const char *name, } object_property_set_bool(OBJECT(dev), true, "realized", &err); if (err) { - error_propagate(errp, err); - error_prepend(errp, "Failed to initialize USB device '%s': ", name); + error_propagate_prepend(errp, err, + "Failed to initialize USB device '%s': ", + name); return NULL; } return dev; diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 8b73582d51..4404c28360 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1283,8 +1283,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp) if (ret == -ENOTSUP) { return 0; } - error_prepend(&err, "msi_init failed: "); - error_propagate(errp, err); + error_propagate_prepend(errp, err, "msi_init failed: "); return ret; } vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0); -- cgit v1.2.3 From 50beeb680949117017343e593dd7323632a6d872 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:26 +0200 Subject: Use error_fatal to simplify obvious fatal errors (again) Add a slight improvement of the Coccinelle semantic patch from commit 007b06578ab, and use it to clean up. It leaves dead Error * variables behind, cleaned up manually. Cc: David Gibson Cc: Alexander Graf Cc: Eric Blake Cc: Paolo Bonzini Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Acked-by: David Gibson Message-Id: <20181017082702.5581-3-armbru@redhat.com> --- hw/intc/xics_kvm.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'hw') diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c index 30c3769a20..e8fa9a53ae 100644 --- a/hw/intc/xics_kvm.c +++ b/hw/intc/xics_kvm.c @@ -198,17 +198,12 @@ static void ics_get_kvm_state(ICSState *ics) { uint64_t state; int i; - Error *local_err = NULL; for (i = 0; i < ics->nr_irqs; i++) { ICSIRQState *irq = &ics->irqs[i]; kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES, - i + ics->offset, &state, false, &local_err); - if (local_err) { - error_report_err(local_err); - exit(1); - } + i + ics->offset, &state, false, &error_fatal); irq->server = state & KVM_XICS_DESTINATION_MASK; irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT) -- cgit v1.2.3 From 0765691e9744ed966d40a104f9808924578f2e3b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:28 +0200 Subject: cpus hw target: Use warn_report() & friends to report warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling error_report() in a function that takes an Error ** argument is suspicious. Convert a few that are actually warnings to warn_report(). While there, split a warning consisting of multiple sentences to conform to conventions spelled out in warn_report()'s contract. Cc: Alex Bennée Cc: Mark Cave-Ayland Cc: Alex Williamson Cc: Fam Zheng Cc: Wei Huang Cc: David Gibson Signed-off-by: Markus Armbruster Acked-by: David Gibson Reviewed-by: Alex Bennée Message-Id: <20181017082702.5581-5-armbru@redhat.com> --- hw/display/cg3.c | 2 +- hw/display/tcx.c | 2 +- hw/misc/ivshmem.c | 4 ++-- hw/net/virtio-net.c | 8 ++++---- hw/virtio/virtio-pci.c | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) (limited to 'hw') diff --git a/hw/display/cg3.c b/hw/display/cg3.c index 1c199ab369..e50d97e48c 100644 --- a/hw/display/cg3.c +++ b/hw/display/cg3.c @@ -307,7 +307,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp) ret = load_image_mr(fcode_filename, &s->rom); g_free(fcode_filename); if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) { - error_report("cg3: could not load prom '%s'", CG3_ROM_FILE); + warn_report("cg3: could not load prom '%s'", CG3_ROM_FILE); } } diff --git a/hw/display/tcx.c b/hw/display/tcx.c index b2786ee8d0..66f2459226 100644 --- a/hw/display/tcx.c +++ b/hw/display/tcx.c @@ -823,7 +823,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp) ret = load_image_mr(fcode_filename, &s->rom); g_free(fcode_filename); if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) { - error_report("tcx: could not load prom '%s'", TCX_ROM_FILE); + warn_report("tcx: could not load prom '%s'", TCX_ROM_FILE); } } diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 8cb17b9dd4..f88910e55c 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -1288,8 +1288,8 @@ static void ivshmem_realize(PCIDevice *dev, Error **errp) IVShmemState *s = IVSHMEM_COMMON(dev); if (!qtest_enabled()) { - error_report("ivshmem is deprecated, please use ivshmem-plain" - " or ivshmem-doorbell instead"); + warn_report("ivshmem is deprecated, please use ivshmem-plain" + " or ivshmem-doorbell instead"); } if (qemu_chr_fe_backend_connected(&s->server_chr) + !!s->shmobj != 1) { diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 4bdd5b8532..385b1a03e9 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -2020,10 +2020,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp) if (n->net_conf.tx && strcmp(n->net_conf.tx, "timer") && strcmp(n->net_conf.tx, "bh")) { - error_report("virtio-net: " - "Unknown option tx=%s, valid options: \"timer\" \"bh\"", - n->net_conf.tx); - error_report("Defaulting to \"bh\""); + warn_report("virtio-net: " + "Unknown option tx=%s, valid options: \"timer\" \"bh\"", + n->net_conf.tx); + error_printf("Defaulting to \"bh\""); } n->net_conf.tx_queue_size = MIN(virtio_net_max_tx_queue_size(n), diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 3a01fe90f0..a954799267 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1683,8 +1683,8 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) if (err) { /* Notice when a system that supports MSIx can't initialize it */ if (err != -ENOTSUP) { - error_report("unable to init msix vectors to %" PRIu32, - proxy->nvectors); + warn_report("unable to init msix vectors to %" PRIu32, + proxy->nvectors); } proxy->nvectors = 0; } -- cgit v1.2.3 From e1eb292acec5e9806b1afc33f39629e6d0859732 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:29 +0200 Subject: vfio: Use warn_report() & friends to report warnings The vfio code reports warnings like error_report(WARN_PREFIX "Could not frobnicate", DEV-NAME); where WARN_PREFIX is defined so the message comes out as vfio warning: DEV-NAME: Could not frobnicate This usage predates the introduction of warn_report() & friends in commit 97f40301f1d. It's time to convert to that interface. Since these functions already prefix the message with "warning: ", replace WARN_PREFIX by VFIO_MSG_PREFIX, so the messages come out like warning: vfio DEV-NAME: Could not frobnicate The next commit will replace ERR_PREFIX. Cc: Alex Williamson Signed-off-by: Markus Armbruster Acked-by: Alex Williamson Reviewed-by: Eric Blake Message-Id: <20181017082702.5581-6-armbru@redhat.com> --- hw/vfio/pci.c | 14 +++++++------- hw/vfio/platform.c | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) (limited to 'hw') diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 4404c28360..0c3d245932 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -252,7 +252,7 @@ static void vfio_intx_update(PCIDevice *pdev) vfio_intx_enable_kvm(vdev, &err); if (err) { - error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name); + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } /* Re-enable the interrupt in cased we missed an EOI */ @@ -317,7 +317,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp) vfio_intx_enable_kvm(vdev, &err); if (err) { - error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name); + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } vdev->interrupt = VFIO_INT_INTx; @@ -1557,7 +1557,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) &err); if (ret < 0) { if (ret == -ENOTSUP) { - error_report_err(err); + warn_report_err(err); return 0; } @@ -2590,9 +2590,9 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) } else if (irq_info.count == 1) { vdev->pci_aer = true; } else { - error_report(WARN_PREFIX - "Could not enable error recovery for the device", - vbasedev->name); + warn_report(VFIO_MSG_PREFIX + "Could not enable error recovery for the device", + vbasedev->name); } } @@ -2717,7 +2717,7 @@ static void vfio_req_notifier_handler(void *opaque) qdev_unplug(&vdev->pdev.qdev, &err); if (err) { - error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name); + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } } diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index 64c1af653d..baf236ae79 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -679,8 +679,8 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp) for (i = 0; i < vbasedev->num_regions; i++) { if (vfio_region_mmap(vdev->regions[i])) { - error_report("%s mmap unsupported. Performance may be slow", - memory_region_name(vdev->regions[i]->mem)); + warn_report("%s mmap unsupported, performance may be slow", + memory_region_name(vdev->regions[i]->mem)); } sysbus_init_mmio(sbdev, vdev->regions[i]->mem); } -- cgit v1.2.3 From c3b8e3e0ed40112367369683da1e3745cf203ac0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:30 +0200 Subject: vfio: Clean up error reporting after previous commit The previous commit changed vfio's warning messages from vfio warning: DEV-NAME: Could not frobnicate to warning: vfio DEV-NAME: Could not frobnicate To match this change, change error messages from vfio error: DEV-NAME: On fire to vfio DEV-NAME: On fire Note the loss of "error". If we think marking error messages that way is a good idea, we should mark *all* error messages, i.e. make error_report() print it. Cc: Alex Williamson Signed-off-by: Markus Armbruster Acked-by: Alex Williamson Message-Id: <20181017082702.5581-7-armbru@redhat.com> --- hw/vfio/pci-quirks.c | 4 ++-- hw/vfio/pci.c | 8 ++++---- hw/vfio/platform.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) (limited to 'hw') diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c index 481fd08df7..eae31c74d6 100644 --- a/hw/vfio/pci-quirks.c +++ b/hw/vfio/pci-quirks.c @@ -1670,7 +1670,7 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) * but also no point in us enabling VGA if disabled in hardware. */ if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev, &err)) { - error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name); + error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); error_report("IGD device %s failed to enable VGA access, " "legacy mode disabled", vdev->vbasedev.name); goto out; @@ -1696,7 +1696,7 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) ret = vfio_pci_igd_opregion_init(vdev, opregion, &err); if (ret) { error_append_hint(&err, "IGD legacy mode disabled\n"); - error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name); + error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); goto out; } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 0c3d245932..5c7bd96984 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -745,7 +745,7 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev) vfio_intx_enable(vdev, &err); if (err) { - error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name); + error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } } @@ -2196,7 +2196,7 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev) vfio_intx_enable(vdev, &err); if (err) { - error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name); + error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); } for (nr = 0; nr < PCI_NUM_REGIONS - 1; ++nr) { @@ -2830,7 +2830,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { error_setg_errno(errp, errno, "no such host device"); - error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev); + error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.sysfsdev); return; } @@ -3085,7 +3085,7 @@ out_teardown: vfio_teardown_msi(vdev); vfio_bars_exit(vdev); error: - error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name); + error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name); } static void vfio_instance_finalize(Object *obj) diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index baf236ae79..398db38f14 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -690,7 +690,7 @@ out: } if (vdev->vbasedev.name) { - error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name); + error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name); } else { error_prepend(errp, "vfio error: "); } -- cgit v1.2.3 From 1245402e3c9be20200dcc469805f4156d93381e2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:32 +0200 Subject: 9pfs: Fix CLI parsing crash on error Calling error_report() in a function that takes an Error ** argument is suspicious. 9p-handle.c's handle_parse_opts() does that, and then fails without setting an error. Wrong. Its caller crashes when it tries to report the error: $ qemu-system-x86_64 -nodefaults -fsdev id=foo,fsdriver=handle qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: warning: handle backend is deprecated qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: fsdev: No path specified Segmentation fault (core dumped) Screwed up when commit 91cda4e8f37 (v2.12.0) converted the function to Error. Fix by calling error_setg() instead of error_report(). Fixes: 91cda4e8f372602795e3a2f4bd2e3adaf9f82255 Cc: Greg Kurz Signed-off-by: Markus Armbruster Acked-by: Greg Kurz Reviewed-by: Eric Blake Message-Id: <20181017082702.5581-9-armbru@redhat.com> --- hw/9pfs/9p-handle.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c index f3641dbe4a..3465b1ef30 100644 --- a/hw/9pfs/9p-handle.c +++ b/hw/9pfs/9p-handle.c @@ -19,6 +19,7 @@ #include #include #include +#include "qapi/error.h" #include "qemu/xattr.h" #include "qemu/cutils.h" #include "qemu/error-report.h" @@ -655,12 +656,13 @@ static int handle_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp) warn_report("handle backend is deprecated"); if (sec_model) { - error_report("Invalid argument security_model specified with handle fsdriver"); + error_setg(errp, + "Invalid argument security_model specified with handle fsdriver"); return -1; } if (!path) { - error_report("fsdev: No path specified"); + error_setg(errp, "fsdev: No path specified"); return -1; } fse->path = g_strdup(path); -- cgit v1.2.3 From d319e05d6f24350272e7f600a43d0c0091924512 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:33 +0200 Subject: pc: Fix machine property nvdimm-persistence error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling error_report() in a function that takes an Error ** argument is suspicious. pc.c's pc_machine_set_nvdimm_persistence() does that, and then exit()s. Wrong. Attempting to set machine property nvdimm-persistence to a bad value instantly kills the VM: $ qemu-system-x86_64 -nodefaults -S -display none -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 3}, "package": "v3.0.0-837-gc5e4e49258"}, "capabilities": []}} {"execute": "qmp_capabilities"} {"return": {}} {"execute": "qom-set", "arguments": {"path": "/machine", "property": "nvdimm-persistence", "value": "instadeath"}} -machine nvdimm-persistence=instadeath: unsupported option $ echo $? 1 Broken when commit 11c39b5cd96 (v3.0.0) replaced error_propagate(); return by error_report(); exit() instead of error_setg(); return. Fix that. Fixes: 11c39b5cd966ddc067a1ca0c5392ec9b666c45b7 Cc: "Michael S. Tsirkin" Signed-off-by: Markus Armbruster Reviewed-by: Marc-André Lureau Message-Id: <20181017082702.5581-10-armbru@redhat.com> --- hw/i386/pc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/i386/pc.c b/hw/i386/pc.c index cd5029c149..eab8572f2a 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -2209,8 +2209,9 @@ static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value, else if (strcmp(value, "mem-ctrl") == 0) nvdimm_state->persistence = 2; else { - error_report("-machine nvdimm-persistence=%s: unsupported option", value); - exit(EXIT_FAILURE); + error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option", + value); + return; } g_free(nvdimm_state->persistence_string); -- cgit v1.2.3 From 11ab69d6e7a81c35f109a399a39d56e777f41d8a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:34 +0200 Subject: ioapic: Fix error handling in realize() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling error_report() in a function that takes an Error ** argument is suspicious. ioapic_realize() does that, and then exit()s. Currently mostly harmless, as the device cannot be hot-plugged. Fixes: 20fd4b7b6d9282fe0cb83601f1821f31bd257458 Cc: Peter Xu Signed-off-by: Markus Armbruster Reviewed-by: Peter Xu Reviewed-by: Marc-André Lureau Message-Id: <20181017082702.5581-11-armbru@redhat.com> --- hw/intc/ioapic.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'hw') diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c index b6896ac4ce..4e529729b4 100644 --- a/hw/intc/ioapic.c +++ b/hw/intc/ioapic.c @@ -21,7 +21,7 @@ */ #include "qemu/osdep.h" -#include "qemu/error-report.h" +#include "qapi/error.h" #include "monitor/monitor.h" #include "hw/hw.h" #include "hw/i386/pc.h" @@ -393,9 +393,9 @@ static void ioapic_realize(DeviceState *dev, Error **errp) IOAPICCommonState *s = IOAPIC_COMMON(dev); if (s->version != 0x11 && s->version != 0x20) { - error_report("IOAPIC only supports version 0x11 or 0x20 " - "(default: 0x%x).", IOAPIC_VER_DEF); - exit(1); + error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 " + "(default: 0x%x).", IOAPIC_VER_DEF); + return; } memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s, -- cgit v1.2.3 From 1028283c60e6e35911bb80b959a533b4af1b8b65 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:35 +0200 Subject: smbios: Clean up error handling in smbios_add() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling error_report() in a function that takes an Error ** argument is suspicious. smbios_entry_add() does that, and then exit()s. It also passes &error_fatal to qemu_opts_validate(). Both wrong, but currently harmless, as its only caller passes &error_fatal. Messed up in commit 1007a37e208. Clean it up. Cc: Paolo Bonzini Signed-off-by: Markus Armbruster Reviewed-by: Marc-André Lureau Acked-by: Paolo Bonzini Message-Id: <20181017082702.5581-12-armbru@redhat.com> --- hw/smbios/smbios.c | 90 +++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 62 insertions(+), 28 deletions(-) (limited to 'hw') diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c index a27e54b2fa..920939454e 100644 --- a/hw/smbios/smbios.c +++ b/hw/smbios/smbios.c @@ -950,6 +950,7 @@ static void save_opt_list(size_t *ndest, const char ***dest, void smbios_entry_add(QemuOpts *opts, Error **errp) { + Error *err = NULL; const char *val; assert(!smbios_immutable); @@ -960,12 +961,16 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) int size; struct smbios_table *table; /* legacy mode only */ - qemu_opts_validate(opts, qemu_smbios_file_opts, &error_fatal); + qemu_opts_validate(opts, qemu_smbios_file_opts, &err); + if (err) { + error_propagate(errp, err); + return; + } size = get_image_size(val); if (size == -1 || size < sizeof(struct smbios_structure_header)) { - error_report("Cannot read SMBIOS file %s", val); - exit(1); + error_setg(errp, "Cannot read SMBIOS file %s", val); + return; } /* @@ -978,14 +983,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) smbios_tables_len); if (load_image(val, (uint8_t *)header) != size) { - error_report("Failed to load SMBIOS file %s", val); - exit(1); + error_setg(errp, "Failed to load SMBIOS file %s", val); + return; } if (test_bit(header->type, have_fields_bitmap)) { - error_report("can't load type %d struct, fields already specified!", - header->type); - exit(1); + error_setg(errp, + "can't load type %d struct, fields already specified!", + header->type); + return; } set_bit(header->type, have_binfile_bitmap); @@ -1030,19 +1036,23 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) unsigned long type = strtoul(val, NULL, 0); if (type > SMBIOS_MAX_TYPE) { - error_report("out of range!"); - exit(1); + error_setg(errp, "out of range!"); + return; } if (test_bit(type, have_binfile_bitmap)) { - error_report("can't add fields, binary file already loaded!"); - exit(1); + error_setg(errp, "can't add fields, binary file already loaded!"); + return; } set_bit(type, have_fields_bitmap); switch (type) { case 0: - qemu_opts_validate(opts, qemu_smbios_type0_opts, &error_fatal); + qemu_opts_validate(opts, qemu_smbios_type0_opts, &err); + if (err) { + error_propagate(errp, err); + return; + } save_opt(&type0.vendor, opts, "vendor"); save_opt(&type0.version, opts, "version"); save_opt(&type0.date, opts, "date"); @@ -1051,14 +1061,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) val = qemu_opt_get(opts, "release"); if (val) { if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) { - error_report("Invalid release"); - exit(1); + error_setg(errp, "Invalid release"); + return; } type0.have_major_minor = true; } return; case 1: - qemu_opts_validate(opts, qemu_smbios_type1_opts, &error_fatal); + qemu_opts_validate(opts, qemu_smbios_type1_opts, &err); + if (err) { + error_propagate(errp, err); + return; + } save_opt(&type1.manufacturer, opts, "manufacturer"); save_opt(&type1.product, opts, "product"); save_opt(&type1.version, opts, "version"); @@ -1069,14 +1083,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) val = qemu_opt_get(opts, "uuid"); if (val) { if (qemu_uuid_parse(val, &qemu_uuid) != 0) { - error_report("Invalid UUID"); - exit(1); + error_setg(errp, "Invalid UUID"); + return; } qemu_uuid_set = true; } return; case 2: - qemu_opts_validate(opts, qemu_smbios_type2_opts, &error_fatal); + qemu_opts_validate(opts, qemu_smbios_type2_opts, &err); + if (err) { + error_propagate(errp, err); + return; + } save_opt(&type2.manufacturer, opts, "manufacturer"); save_opt(&type2.product, opts, "product"); save_opt(&type2.version, opts, "version"); @@ -1085,7 +1103,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) save_opt(&type2.location, opts, "location"); return; case 3: - qemu_opts_validate(opts, qemu_smbios_type3_opts, &error_fatal); + qemu_opts_validate(opts, qemu_smbios_type3_opts, &err); + if (err) { + error_propagate(errp, err); + return; + } save_opt(&type3.manufacturer, opts, "manufacturer"); save_opt(&type3.version, opts, "version"); save_opt(&type3.serial, opts, "serial"); @@ -1093,7 +1115,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) save_opt(&type3.sku, opts, "sku"); return; case 4: - qemu_opts_validate(opts, qemu_smbios_type4_opts, &error_fatal); + qemu_opts_validate(opts, qemu_smbios_type4_opts, &err); + if (err) { + error_propagate(errp, err); + return; + } save_opt(&type4.sock_pfx, opts, "sock_pfx"); save_opt(&type4.manufacturer, opts, "manufacturer"); save_opt(&type4.version, opts, "version"); @@ -1102,11 +1128,19 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) save_opt(&type4.part, opts, "part"); return; case 11: - qemu_opts_validate(opts, qemu_smbios_type11_opts, &error_fatal); + qemu_opts_validate(opts, qemu_smbios_type11_opts, &err); + if (err) { + error_propagate(errp, err); + return; + } save_opt_list(&type11.nvalues, &type11.values, opts, "value"); return; case 17: - qemu_opts_validate(opts, qemu_smbios_type17_opts, &error_fatal); + qemu_opts_validate(opts, qemu_smbios_type17_opts, &err); + if (err) { + error_propagate(errp, err); + return; + } save_opt(&type17.loc_pfx, opts, "loc_pfx"); save_opt(&type17.bank, opts, "bank"); save_opt(&type17.manufacturer, opts, "manufacturer"); @@ -1116,12 +1150,12 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) type17.speed = qemu_opt_get_number(opts, "speed", 0); return; default: - error_report("Don't know how to build fields for SMBIOS type %ld", - type); - exit(1); + error_setg(errp, + "Don't know how to build fields for SMBIOS type %ld", + type); + return; } } - error_report("Must specify type= or file="); - exit(1); + error_setg(errp, "Must specify type= or file="); } -- cgit v1.2.3 From fff4c9c32595e71bca3d1a89beb0781a5be2201b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:40 +0200 Subject: xen/pt: Fix incomplete conversion to realize() The conversion of "xen-pci-passthrough" to realize() (commit 5a11d0f7549, v2.6.0) neglected to convert the xen_pt_config_init() error path. If xen_pt_config_init() fails, xen_pt_realize() reports the error, then returns success without completing its job. I don't know the exact impact, but it can't be good. Belatedly convert the error path. Fixes: 5a11d0f7549e24a10e178a9dc8ff5e698031d9a6 Cc: Stefano Stabellini Cc: Anthony Perard Signed-off-by: Markus Armbruster Acked-by: Anthony PERARD Message-Id: <20181017082702.5581-17-armbru@redhat.com> --- hw/xen/xen_pt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index e5a6eff44f..f1f3a3727c 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -830,7 +830,7 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) xen_pt_config_init(s, &err); if (err) { error_append_hint(&err, "PCI Config space initialisation failed"); - error_report_err(err); + error_propagate(errp, err); rc = -1; goto err_out; } -- cgit v1.2.3 From b836723dfe642a67904bcd047feaaa0cda662d38 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 17 Oct 2018 10:26:55 +0200 Subject: fsdev: Clean up error reporting in qemu_fsdev_add() Calling error_report() from within a function that takes an Error ** argument is suspicious. qemu_fsdev_add() does that, and its caller fsdev_init_func() then fails without setting an error. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway. Cc: Greg Kurz Signed-off-by: Markus Armbruster Acked-by: Greg Kurz Message-Id: <20181017082702.5581-32-armbru@redhat.com> --- hw/9pfs/xen-9p-backend.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c index 6026780f95..3f54a21c76 100644 --- a/hw/9pfs/xen-9p-backend.c +++ b/hw/9pfs/xen-9p-backend.c @@ -14,6 +14,7 @@ #include "hw/9pfs/9p.h" #include "hw/xen/xen_backend.h" #include "hw/9pfs/xen-9pfs.h" +#include "qapi/error.h" #include "qemu/config-file.h" #include "qemu/option.h" #include "fsdev/qemu-fsdev.h" @@ -355,6 +356,7 @@ static int xen_9pfs_free(struct XenDevice *xendev) static int xen_9pfs_connect(struct XenDevice *xendev) { + Error *err = NULL; int i; Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev); V9fsState *s = &xen_9pdev->state; @@ -452,7 +454,10 @@ static int xen_9pfs_connect(struct XenDevice *xendev) qemu_opt_set(fsdev, "path", xen_9pdev->path, NULL); qemu_opt_set(fsdev, "security_model", xen_9pdev->security_model, NULL); qemu_opts_set_id(fsdev, s->fsconf.fsdev_id); - qemu_fsdev_add(fsdev); + qemu_fsdev_add(fsdev, &err); + if (err) { + error_report_err(err); + } v9fs_device_realize_common(s, &xen_9p_transport, NULL); return 0; -- cgit v1.2.3