On 6/29/23 13:03, Michal Prívozník wrote:
> On 6/5/23 07:07, Han Han wrote:
>> Allow //disk/target@removable for scsi disk devices, since QEMU has support
>> the removable attribute for scsi-hd device from v0.14.0[1].
>>
>> [1]: 419e691f8e: scsi-disk: Allow overriding SCSI INQUIRY removable bit
>>
>> Signed-off-by: Han Han <hhan@redhat.com>
>> ---
>>  docs/formatdomain.rst                              |  2 +-
>>  src/conf/domain_validate.c                         |  5 +++--
>>  src/qemu/qemu_command.c                            |  6 ++++--
>>  ...gs => disk-device-removable.x86_64-latest.args} | 14 +++++++++-----
>>  ...ice-removable.xml => disk-device-removable.xml} |  5 +++++
>>  tests/qemuxml2argvtest.c                           |  2 +-
>>  6 files changed, 23 insertions(+), 11 deletions(-)
>>  rename tests/qemuxml2argvdata/{disk-usb-device-removable.x86_64-latest.args => disk-device-removable.x86_64-latest.args} (70%)
>>  rename tests/qemuxml2argvdata/{disk-usb-device-removable.xml => disk-device-removable.xml} (80%)
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index c3526439bf..676e4b9fd3 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -3094,7 +3094,7 @@ paravirtualized driver is specified via the ``disk`` element.
>>     CDROM or Floppy disk), the value can be either "open" or "closed", defaults
>>     to "closed". NB, the value of ``tray`` could be updated while the domain is
>>     running. The optional attribute ``removable`` sets the removable flag for USB
>> -   disks, and its value can be either "on" or "off", defaulting to "off".
>> +   or SCSI disks, and its value can be either "on" or "off", defaulting to "off".
>>     The optional attribute ``rotation_rate`` sets the rotation rate of the
>>     storage for disks on a SCSI, IDE, or SATA bus. Values in the range 1025 to
>>     65534 are used to indicate rotational media speed in revolutions per minute.
>> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
>> index 80d6a2ffd9..e7122f6297 100644
>> --- a/src/conf/domain_validate.c
>> +++ b/src/conf/domain_validate.c
>> @@ -881,9 +881,10 @@ virDomainDiskDefValidate(const virDomainDef *def,
>>      }
>> 
>>      if (disk->removable != VIR_TRISTATE_SWITCH_ABSENT &&
>> -        disk->bus != VIR_DOMAIN_DISK_BUS_USB) {
>> +        disk->bus != VIR_DOMAIN_DISK_BUS_USB &&
>> +        !(disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK)) {
>
> This is a long long line and could be split into two.
>
>>          virReportError(VIR_ERR_XML_ERROR, "%s",
>> -                       _("removable is only valid for usb disks"));
>> +                       _("removable is only valid for usb or scsi disks"));
>>          return -1;
>>      }
>> 
>
> The rest looks okay. I'd merge this but it's a feature and we're
> currently in the freeze, preparing for upcoming release (i.e. only bug
> fixes should be merged). I'll merge this after the release.
>
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>

Merged now.

Michal