Re: [Qemu-devel] [PATCH 1/4] scsi: Clean up duplicated error in legacy if=scsi code

[Peter Crosthwaite]

On Fri, Feb 20, 2015 at 11:19 AM, Markus Armbruster <address@hidden> 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 <address@hidden>

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
>  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
>
>