| Message ID | 20180625164239.13794-1-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb-storage: Add rerror/werror properties | expand |
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
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
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(), };
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(-)