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

[Fiona Ebner]

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