| Message ID | 20240131130607.24117-1-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: Don't ignore most usb-storage properties | expand |
Hi, we got a user report about bootindex for an 'usb-storage' device not working anymore [0] and I reproduced it and bisected it to this patch. Am 31.01.24 um 14:06 schrieb Kevin Wolf: > @@ -399,11 +397,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, > object_property_add_child(OBJECT(bus), name, OBJECT(dev)); > g_free(name); > > + s = SCSI_DEVICE(dev); > + s->conf = *conf; > + > qdev_prop_set_uint32(dev, "scsi-id", unit); > - if (bootindex >= 0) { > - object_property_set_int(OBJECT(dev), "bootindex", bootindex, > - &error_abort); > - } The fact that this is not called anymore means that the 'set' method for the property is also not called. Here, that method is device_set_bootindex() (as configured by scsi_dev_instance_init() -> device_add_bootindex_property()). Therefore, the device is never registered via add_boot_device_path() meaning that the bootindex property does not have the desired effect anymore. Is it necessary to keep the object_property_set_{bool,int} and qdev_prop_set_enum calls around for these potential side effects? Would it even be necessary to introduce new similar calls for the newly supported properties? Or is there an easy alternative to s->conf = *conf; that does trigger the side effects? > if (object_property_find(OBJECT(dev), "removable")) { > qdev_prop_set_bit(dev, "removable", removable); > } > @@ -414,19 +411,12 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, > object_unparent(OBJECT(dev)); > return NULL; > } > - if (!object_property_set_bool(OBJECT(dev), "share-rw", share_rw, errp)) { > - object_unparent(OBJECT(dev)); > - return NULL; > - } > - > - qdev_prop_set_enum(dev, "rerror", rerror); > - qdev_prop_set_enum(dev, "werror", werror); > > if (!qdev_realize_and_unref(dev, &bus->qbus, errp)) { > object_unparent(OBJECT(dev)); > return NULL; > } > - return SCSI_DEVICE(dev); > + return s; > } > > void scsi_bus_legacy_handle_cmdline(SCSIBus *bus) [0]: https://forum.proxmox.com/threads/149772/post-679433 Best Regards, Fiona
Am 01.07.2024 um 15:08 hat Fiona Ebner geschrieben: > Hi, > > we got a user report about bootindex for an 'usb-storage' device not > working anymore [0] and I reproduced it and bisected it to this patch. > > Am 31.01.24 um 14:06 schrieb Kevin Wolf: > > @@ -399,11 +397,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, > > object_property_add_child(OBJECT(bus), name, OBJECT(dev)); > > g_free(name); > > > > + s = SCSI_DEVICE(dev); > > + s->conf = *conf; > > + > > qdev_prop_set_uint32(dev, "scsi-id", unit); > > - if (bootindex >= 0) { > > - object_property_set_int(OBJECT(dev), "bootindex", bootindex, > > - &error_abort); > > - } > > The fact that this is not called anymore means that the 'set' method > for the property is also not called. Here, that method is > device_set_bootindex() (as configured by scsi_dev_instance_init() -> > device_add_bootindex_property()). Therefore, the device is never > registered via add_boot_device_path() meaning that the bootindex > property does not have the desired effect anymore. Hmm, yes, seem I missed this side effect. Bringing back this one object_property_set_int() call would be the easiest fix, but I wonder if an explicit add_boot_device_path() call (and allowing that one to fail gracefully instead of directly calling exit()) wouldn't be better than re-setting a property to its current value just for the side effect. > Is it necessary to keep the object_property_set_{bool,int} and > qdev_prop_set_enum calls around for these potential side effects? Would > it even be necessary to introduce new similar calls for the newly > supported properties? Or is there an easy alternative to > s->conf = *conf; > that does trigger the side effects? I don't think the other properties whose setter we don't call any more have side effects. They are processed during .realize, which is what I probably expected for bootindex, too. And that's really how all properties should be if we ever want to move forward with the .instance_config approach for creating QOM objects because then we won't call any setters during object creation any more, they would only be for runtime changes. Kevin
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index 10c4e8288d..c3d5e17e38 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -199,10 +199,7 @@ 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, + int unit, bool removable, BlockConf *conf, const char *serial, Error **errp); void scsi_bus_set_ua(SCSIBus *bus, SCSISense sense); void scsi_bus_legacy_handle_cmdline(SCSIBus *bus); diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 0a2eb11c56..f37737e6b6 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -373,15 +373,13 @@ static void scsi_qdev_unrealize(DeviceState *qdev) /* handle legacy '-drive if=scsi,...' cmd line args */ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, - int unit, bool removable, int bootindex, - bool share_rw, - BlockdevOnError rerror, - BlockdevOnError werror, + int unit, bool removable, BlockConf *conf, const char *serial, Error **errp) { const char *driver; char *name; DeviceState *dev; + SCSIDevice *s; DriveInfo *dinfo; if (blk_is_sg(blk)) { @@ -399,11 +397,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, object_property_add_child(OBJECT(bus), name, OBJECT(dev)); g_free(name); + s = SCSI_DEVICE(dev); + s->conf = *conf; + qdev_prop_set_uint32(dev, "scsi-id", unit); - if (bootindex >= 0) { - object_property_set_int(OBJECT(dev), "bootindex", bootindex, - &error_abort); - } if (object_property_find(OBJECT(dev), "removable")) { qdev_prop_set_bit(dev, "removable", removable); } @@ -414,19 +411,12 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk, object_unparent(OBJECT(dev)); return NULL; } - if (!object_property_set_bool(OBJECT(dev), "share-rw", share_rw, errp)) { - object_unparent(OBJECT(dev)); - return NULL; - } - - qdev_prop_set_enum(dev, "rerror", rerror); - qdev_prop_set_enum(dev, "werror", werror); if (!qdev_realize_and_unref(dev, &bus->qbus, errp)) { object_unparent(OBJECT(dev)); return NULL; } - return SCSI_DEVICE(dev); + return s; } void scsi_bus_legacy_handle_cmdline(SCSIBus *bus) @@ -434,6 +424,12 @@ void scsi_bus_legacy_handle_cmdline(SCSIBus *bus) Location loc; DriveInfo *dinfo; int unit; + BlockConf conf = { + .bootindex = -1, + .share_rw = false, + .rerror = BLOCKDEV_ON_ERROR_AUTO, + .werror = BLOCKDEV_ON_ERROR_AUTO, + }; loc_push_none(&loc); for (unit = 0; unit <= bus->info->max_target; unit++) { @@ -443,10 +439,7 @@ 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, - BLOCKDEV_ON_ERROR_AUTO, - BLOCKDEV_ON_ERROR_AUTO, - NULL, &error_fatal); + unit, false, &conf, NULL, &error_fatal); } loc_pop(&loc); } diff --git a/hw/usb/dev-storage-classic.c b/hw/usb/dev-storage-classic.c index 84d19752b5..50a3ad6285 100644 --- a/hw/usb/dev-storage-classic.c +++ b/hw/usb/dev-storage-classic.c @@ -67,10 +67,7 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp) scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(dev), &usb_msd_scsi_info_storage); 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); + &s->conf, dev->serial, errp); blk_unref(blk); if (!scsi_dev) { return;
usb-storage is for the most part just a wrapper around an internally created scsi-disk device. It uses DEFINE_BLOCK_PROPERTIES() to offer all of the usual block device properties to the user, but then only forwards a few select properties to the internal device while the rest is silently ignored. This changes scsi_bus_legacy_add_drive() to accept a whole BlockConf instead of some individual values inside of it so that usb-storage can now pass the whole configuration to the internal scsi-disk. This enables the remaining block device properties, e.g. logical/physical_block_size or discard_granularity. Buglink: https://issues.redhat.com/browse/RHEL-22375 Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/scsi/scsi.h | 5 +---- hw/scsi/scsi-bus.c | 33 +++++++++++++-------------------- hw/usb/dev-storage-classic.c | 5 +---- 3 files changed, 15 insertions(+), 28 deletions(-)