From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55988) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFOdD-0002Cg-4g for qemu-devel@nongnu.org; Thu, 07 Aug 2014 10:25:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XFOd5-0006Z5-3N for qemu-devel@nongnu.org; Thu, 07 Aug 2014 10:25:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51471) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFOd4-0006Yy-RO for qemu-devel@nongnu.org; Thu, 07 Aug 2014 10:25:19 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s77EPHv7020188 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Thu, 7 Aug 2014 10:25:18 -0400 Date: Thu, 7 Aug 2014 16:25:15 +0200 From: Kevin Wolf Message-ID: <20140807142515.GE3374@noname.redhat.com> References: <1407229913-16862-1-git-send-email-famz@redhat.com> <1407229913-16862-3-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1407229913-16862-3-git-send-email-famz@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2] scsi-bus: Convert DeviceClass init to realize List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com Am 05.08.2014 um 11:11 hat Fam Zheng geschrieben: > Replace "init/destroy" with "realize/unrealize" in SCSIDeviceClass, > which has errp as a parameter. So all the implementations now uses > error_setg instead of error_report for reporting error. > > Also in lsi53c895a, report the error when initializing the if=scsi > devices, before dropping it, because the callee's error_report is > changed to error_segs. > > Signed-off-by: Fam Zheng > --- > hw/scsi/lsi53c895a.c | 2 ++ > hw/scsi/scsi-bus.c | 64 ++++++++++++++++++------------------- > hw/scsi/scsi-disk.c | 78 ++++++++++++++++++++++++---------------------- > hw/scsi/scsi-generic.c | 37 +++++++++++----------- > include/hw/scsi/scsi.h | 7 +++-- > tests/qemu-iotests/051.out | 4 +-- > 6 files changed, 96 insertions(+), 96 deletions(-) > > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index 786d848..dbc98a0 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -19,6 +19,7 @@ > #include "hw/pci/pci.h" > #include "hw/scsi/scsi.h" > #include "sysemu/dma.h" > +#include "qemu/error-report.h" > > //#define DEBUG_LSI > //#define DEBUG_LSI_REG > @@ -2121,6 +2122,7 @@ static int lsi_scsi_init(PCIDevice *dev) > if (!d->hotplugged) { > scsi_bus_legacy_handle_cmdline(&s->bus, &err); > if (err != NULL) { > + error_report("%s", error_get_pretty(err)); > error_free(err); > return -1; > } Wouldn't qerror_report_err() be more useful? Or is already a QMP error emitted in a different place in the callchain? The same question is true for the added error_report() calls in patch 1. > @@ -169,43 +168,40 @@ static int scsi_qdev_init(DeviceState *qdev) > d = scsi_device_find(bus, dev->channel, dev->id, ++lun); > } while (d && d->lun == lun && lun < bus->info->max_lun); > if (d && d->lun == lun) { > - error_report("no free lun"); > - goto err; > + error_setg(errp, "no free lun"); > + return; > } > dev->lun = lun; > } else { > d = scsi_device_find(bus, dev->channel, dev->id, dev->lun); > assert(d); > if (d->lun == dev->lun && dev != d) { > - error_report("lun already used by '%s'", d->qdev.id); > - goto err; > + error_setg(errp, "lun already used by '%s'", d->qdev.id); > + return; > } > } > > QTAILQ_INIT(&dev->requests); > - rc = scsi_device_init(dev); > - if (rc == 0) { > + scsi_device_realize(dev, &local_err); > + if (local_err) { > dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb, > dev); > + error_propagate(errp, local_err); > } Maybe I'm misunderstanding something, but it looks to me as if the handler was previously installed in case of success, and now it's only installed on failure? > > if (bus->info->hotplug) { > bus->info->hotplug(bus, dev); > } > - > -err: > - return rc; > } > diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out > index d7b0f50..f6d9dc1 100644 > --- a/tests/qemu-iotests/051.out > +++ b/tests/qemu-iotests/051.out > @@ -122,7 +122,7 @@ QEMU_PROG: -drive if=virtio: Device 'virtio-blk-pci' could not be initialized > > Testing: -drive if=scsi > QEMU X.Y.Z monitor - type 'help' for more information > -(qemu) QEMU_PROG: -drive if=scsi: Device needs media, but drive is empty > +(qemu) QEMU_PROG: Device needs media, but drive is empty > QEMU_PROG: Device initialization failed. > QEMU_PROG: Initialization of device lsi53c895a failed The old error message was certainly more useful. Not sure if there's a good way to retain it, though. Kevin