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