The error handling policy was traditionally set with -drive, but with
-blockdev it is no longer possible to set frontend options. scsi-disk
(and other block devices) have long supported qdev properties to
configure the error handling policy, so let's add these options to
usb-storage as well and just forward them to the internal scsi-disk
instance.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/hw/scsi/scsi.h | 2 ++
hw/scsi/scsi-bus.c | 11 ++++++++++-
hw/usb/dev-storage.c | 2 ++
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index e35137ea78..1a7290d563 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -154,6 +154,8 @@ static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
int unit, bool removable, int bootindex,
bool share_rw,
+ BlockdevOnError rerror,
+ BlockdevOnError werror,
const char *serial, Error **errp);
void scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
void scsi_legacy_handle_cmdline(void);
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 9646743a7d..5905f6bf29 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -226,6 +226,8 @@ static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
int unit, bool removable, int bootindex,
bool share_rw,
+ BlockdevOnError rerror,
+ BlockdevOnError werror,
const char *serial, Error **errp)
{
const char *driver;
@@ -262,6 +264,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
object_unparent(OBJECT(dev));
return NULL;
}
+
+ qdev_prop_set_enum(dev, "rerror", rerror);
+ qdev_prop_set_enum(dev, "werror", werror);
+
object_property_set_bool(OBJECT(dev), true, "realized", &err);
if (err != NULL) {
error_propagate(errp, err);
@@ -285,7 +291,10 @@ void scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
}
qemu_opts_loc_restore(dinfo->opts);
scsi_bus_legacy_add_drive(bus, blk_by_legacy_dinfo(dinfo),
- unit, false, -1, false, NULL, &error_fatal);
+ unit, false, -1, false,
+ BLOCKDEV_ON_ERROR_AUTO,
+ BLOCKDEV_ON_ERROR_AUTO,
+ NULL, &error_fatal);
}
loc_pop(&loc);
}
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index c99398b7f6..cd5551d94f 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -625,6 +625,7 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
&usb_msd_scsi_info_storage, NULL);
scsi_dev = scsi_bus_legacy_add_drive(&s->bus, blk, 0, !!s->removable,
s->conf.bootindex, s->conf.share_rw,
+ s->conf.rerror, s->conf.werror,
dev->serial,
errp);
blk_unref(blk);
@@ -671,6 +672,7 @@ static const VMStateDescription vmstate_usb_msd = {
static Property msd_properties[] = {
DEFINE_BLOCK_PROPERTIES(MSDState, conf),
+ DEFINE_BLOCK_ERROR_PROPERTIES(MSDState, conf),
DEFINE_PROP_BIT("removable", MSDState, removable, 0, false),
DEFINE_PROP_END_OF_LIST(),
};
--
2.13.6
Kevin Wolf <kwolf@redhat.com> writes: > The error handling policy was traditionally set with -drive, but with > -blockdev it is no longer possible to set frontend options. scsi-disk > (and other block devices) have long supported qdev properties to > configure the error handling policy, so let's add these options to > usb-storage as well and just forward them to the internal scsi-disk > instance. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Let's see whether I remember / figure out how this stuff works. Not all devices support error handling policy, but scsi-disk does. The preferred way to configure error handling policy is with qdev properties rerror, werror. blkconf_apply_backend_options() propagates them to the associated BlockBackend. The outmoded way is with -drive parameters rerror, werror. blockdev_init() propagates them to the BlockBackend. If you use both ways, qdev properties win. usb-storage is a hack: it tries to look like a block device, but it's really a SCSI controller that can serve only a single scsi-disk device, which it creates automatically. We want to deprecate -drive. We also want to deprecate usb-storage, but I guess we're still not ready for that (it's a complicated story). To deprecate -drive without also deprecating usb-storage, we need to preserve features. The outmoded way works with usb-storage: the automatically created scsi-disk is associated with the BlockBackend that got its error handling policy from -drive. The preferred way doesn't, because usb-storage lacks the qdev properties. This patch adds them. Kind of sad, but it's either that or deprecating usb-storage. Patch looks good to me, thus Reviewed-by: Markus Armbruster <armbru@redhat.com> Feel free to work whatever parts of my ramblings you find useful into your commit message.
On 26/06/2018 10:35, Markus Armbruster wrote: > We also want to deprecate usb-storage, but > I guess we're still not ready for that (it's a complicated story). > > To deprecate -drive without also deprecating usb-storage, we need to > preserve features. Patch looks good to me too, but indeed why can't we deprecate usb-storage in favor of usb-bot + scsi-disk? Thanks, Paolo
On Tue, Jun 26, 2018 at 10:48:10AM +0200, Paolo Bonzini wrote: > On 26/06/2018 10:35, Markus Armbruster wrote: > > We also want to deprecate usb-storage, but > > I guess we're still not ready for that (it's a complicated story). > > > > To deprecate -drive without also deprecating usb-storage, we need to > > preserve features. > > Patch looks good to me too, but indeed why can't we deprecate > usb-storage in favor of usb-bot + scsi-disk? Presumably the migration state of usb-storage is different from the migration state of usb-bot + scsi-disk, so not a case where libvirt could silently switch from one to the other. It would need to be an explicit config change by apps/users. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 26/06/2018 10:53, Daniel P. Berrangé wrote:
> On Tue, Jun 26, 2018 at 10:48:10AM +0200, Paolo Bonzini wrote:
>> On 26/06/2018 10:35, Markus Armbruster wrote:
>>> We also want to deprecate usb-storage, but
>>> I guess we're still not ready for that (it's a complicated story).
>>>
>>> To deprecate -drive without also deprecating usb-storage, we need to
>>> preserve features.
>>
>> Patch looks good to me too, but indeed why can't we deprecate
>> usb-storage in favor of usb-bot + scsi-disk?
>
> Presumably the migration state of usb-storage is different from the
> migration state of usb-bot + scsi-disk, so not a case where libvirt
> could silently switch from one to the other. It would need to be an
> explicit config change by apps/users.
From a quick test, migrating
qemu-kvm \
-drive if=none,id=usb,file=$HOME/chipsec.img,snapshot=on \
-usb -device usb-storage,drive=usb -M q35
into
qemu-kvm \
-drive if=none,id=usb,file=$HOME/chipsec.img,snapshot=on \
-usb -device usb-bot -device scsi-disk,drive=usb -M q35
seems to work. Both of them indeed use vmstate_usb_msd for
migration.
Paolo
On Tue, Jun 26, 2018 at 10:48:10AM +0200, Paolo Bonzini wrote:
> On 26/06/2018 10:35, Markus Armbruster wrote:
> > We also want to deprecate usb-storage, but
> > I guess we're still not ready for that (it's a complicated story).
> >
> > To deprecate -drive without also deprecating usb-storage, we need to
> > preserve features.
>
> Patch looks good to me too, but indeed why can't we deprecate
> usb-storage in favor of usb-bot + scsi-disk?
I think we can. One blocking issue was hotplug support, but that got fixed a
while ago, and I'm not aware of any other issue ...
cheers,
Gerd
commit b78ecd0998094a8b7e0f14c4888f3a6b525d14ff
Author: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed Jun 15 11:46:58 2016 +0200
usb-bot: hotplug support
This patch marks usb-bot as hot-pluggable device, makes attached
property settable and turns off auto-attach in case the device
was hotplugged.
Hot-plugging a usb-bot device with one or more scsi devices can be
done this way now:
(1) device-add usb-bot,id=foo
(2) device-add scsi-{hd,cd},bus=foo.0,lun=0
(2b) optionally add more devices (luns 0 ... 15).
(3) qom-set foo.attached = true
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-id: 1465984019-28963-5-git-send-email-kraxel@redhat.com
© 2016 - 2025 Red Hat, Inc.