Re: [PATCH] scsi: Don't ignore most usb-storage properties

[Kevin Wolf]

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