On Fri, Feb 20, 2015 at 11:19 AM, Markus Armbruster <arm...@redhat.com> wrote:
> 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" and "virtio-scsi-device")
>
> 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", "spapr-vscsi")
>
> 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 <arm...@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
> ---
> hw/scsi/esp-pci.c | 2 ++
> hw/scsi/lsi53c895a.c | 3 ++-
> hw/scsi/megasas.c | 2 ++
> hw/scsi/scsi-bus.c | 1 -
> hw/scsi/spapr_vscsi.c | 2 ++
> 5 files changed, 8 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;
> }
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index 3639235..f76d334 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -39,6 +39,7 @@
> #include "hw/ppc/spapr.h"
> #include "hw/ppc/spapr_vio.h"
> #include "viosrp.h"
> +#include "qapi/qmp/qerror.h"
>
> #include <libfdt.h>
>
> @@ -1224,6 +1225,7 @@ static int spapr_vscsi_init(VIOsPAPRDevice *dev)
> if (!dev->qdev.hotplugged) {
> scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> if (err != NULL) {
> + qerror_report_err(err);
> error_free(err);
> return -1;
> }
> --
> 1.9.3
>
>
