From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46826) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YV2FJ-00013I-KR for qemu-devel@nongnu.org; Mon, 09 Mar 2015 14:17:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YV2FB-0004xN-B9 for qemu-devel@nongnu.org; Mon, 09 Mar 2015 14:17:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44826) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YV2FB-0004xH-43 for qemu-devel@nongnu.org; Mon, 09 Mar 2015 14:17:33 -0400 From: Markus Armbruster Date: Mon, 9 Mar 2015 19:17:25 +0100 Message-Id: <1425925048-15482-2-git-send-email-armbru@redhat.com> In-Reply-To: <1425925048-15482-1-git-send-email-armbru@redhat.com> References: <1425925048-15482-1-git-send-email-armbru@redhat.com> Subject: [Qemu-devel] [PATCH v2 1/4] scsi: Clean up duplicated error in legacy if=scsi code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: pbonzini@redhat.com, peter.crosthwaite@xilinx.com, hare@suse.de Commit a818a4b changed scsi_bus_legacy_handle_cmdline() to report errors from scsi_bus_legacy_add_drive() with error_report() in addition to returning them. That's inappropriate. Two kinds of callers: 1. realize methods (devices "esp", "virtio-scsi-device" and "spapr-vscsi") The error object gets passed up the call chain until it gets reported again and freed. Example: $ qemu-system-arm -M virt -S -display none \ > -drive if=scsi,id=foo,bus=1,file=tmp.qcow2 \ > -device nec-usb-xhci -device usb-storage,drive=foo \ > -device virtio-scsi-pci qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Setting drive property failed qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed qemu-system-arm: -device virtio-scsi-pci: Device initialization failed qemu-system-arm: -device virtio-scsi-pci: Device 'virtio-scsi-pci' could not be initialized The second message in this error cascade comes from scsi_bus_legacy_handle_cmdline(). The error object then gets passed up to the qdev_init() called from virtio_scsi_pci_init_pci(), which reports it again. 2. init methods (devices "am53c974", "dc390", "lsi53c895a", "lsi53c810", "megasas", "megasas-gen2") init methods need to report their errors with qerror_report(). These don't. The inappropriate error_report() papers over the bug. error_report() isn't the same as qerror_report() in QMP context, but this can't actually happen: QMP can still only hot-plug, and callers call scsi_bus_legacy_handle_cmdline() only on cold-plug. Except for sysbus_esp_realize(), but that can't be hot-plugged at all, as far as I can tell. Fix the init methods and drop the inappropriate error_report() in scsi_bus_legacy_handle_cmdline(). Signed-off-by: Markus Armbruster Reviewed-by: Peter Crosthwaite --- hw/scsi/esp-pci.c | 2 ++ hw/scsi/lsi53c895a.c | 3 ++- hw/scsi/megasas.c | 2 ++ hw/scsi/scsi-bus.c | 1 - 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c index 00b7297..a75fcfa 100644 --- a/hw/scsi/esp-pci.c +++ b/hw/scsi/esp-pci.c @@ -28,6 +28,7 @@ #include "hw/scsi/esp.h" #include "trace.h" #include "qemu/log.h" +#include "qapi/qmp/qerror.h" #define TYPE_AM53C974_DEVICE "am53c974" @@ -369,6 +370,7 @@ static int esp_pci_scsi_init(PCIDevice *dev) if (!d->hotplugged) { scsi_bus_legacy_handle_cmdline(&s->bus, &err); if (err != NULL) { + qerror_report_err(err); error_free(err); return -1; } diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index db7d4b8..4ffab03 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -19,7 +19,7 @@ #include "hw/pci/pci.h" #include "hw/scsi/scsi.h" #include "sysemu/dma.h" -#include "qemu/error-report.h" +#include "qapi/qmp/qerror.h" //#define DEBUG_LSI //#define DEBUG_LSI_REG @@ -2119,6 +2119,7 @@ static int lsi_scsi_init(PCIDevice *dev) if (!d->hotplugged) { scsi_bus_legacy_handle_cmdline(&s->bus, &err); if (err != NULL) { + qerror_report_err(err); error_free(err); return -1; } diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 4852237..69ffdbd 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -28,6 +28,7 @@ #include "hw/scsi/scsi.h" #include "block/scsi.h" #include "trace.h" +#include "qapi/qmp/qerror.h" #include "mfi.h" @@ -2409,6 +2410,7 @@ static int megasas_scsi_init(PCIDevice *dev) if (!d->hotplugged) { scsi_bus_legacy_handle_cmdline(&s->bus, &err); if (err != NULL) { + qerror_report_err(err); error_free(err); return -1; } diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index dca9576..f8de569 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -273,7 +273,6 @@ void scsi_bus_legacy_handle_cmdline(SCSIBus *bus, Error **errp) scsi_bus_legacy_add_drive(bus, blk_by_legacy_dinfo(dinfo), unit, false, -1, NULL, &err); if (err != NULL) { - error_report("%s", error_get_pretty(err)); error_propagate(errp, err); break; } -- 1.9.3