usb-storage: Add rerror/werror properties

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.

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

index e35137ea78..1a7290d563 100644
@@ -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);

index 9646743a7d..5905f6bf29 100644
@@ -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);
 }

index c99398b7f6..cd5551d94f 100644
@@ -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

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

Feel free to work whatever parts of my ramblings you find useful into your commit message.

Patch looks good to me too, but indeed why can't we deprecate usb-storage in favor of usb-bot + scsi-disk?

Thanks,

Paolo

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

  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

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 <[email protected]>
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

Message-id: [email protected]