[PATCH] qemu: add 'media=cdrom' attribute for usb CDROM

[Peter Krempa]

On Sat, Oct 07, 2023 at 16:12:09 +0800, Minglei Liu wrote:
> From: "minglei.liu" <minglei.liu at smartx.com>
> 
> In commit 1328a83, the 'media=cdrom' attribute was removed from -drive.
> However, this attribute is still essential for usb cdrom and is still
> supported in qemu 8.1.1. Therefore, we need to reintroduce this attribute
> for usb cdrom.

This just states that it _is_ needed but not why. Please add
justification next time.

Your patch is also missing ceritifcation that it conforms with the
Developer Certificate of origin:

https://www.libvirt.org/hacking.html#developer-certificate-of-origin

> ---
>  src/qemu/qemu_command.c                                    | 7 +++++++
>  .../disk-cdrom-bus-other.x86_64-latest.args                | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 8a7b80719f..42f3f8f740 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1705,6 +1705,13 @@ qemuBuildDriveStr(virDomainDiskDef *disk)
>  
>      virBufferAsprintf(&opt, "if=sd,index=%d", virDiskNameToIndex(disk->dst));
>  
> +    /* While this is a frontend attribute, it only makes sense to be used when
> +     * legacy -drive is used. In modern qemu the 'ide-cd' or 'scsi-cd' are used.
> +     * currently only usb cdrom need this attribute */
> +    if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
> +        disk->bus == VIR_DOMAIN_DISK_BUS_USB)
> +        virBufferAddLit(&opt, ",media=cdrom");

This code path is exclusively used for '-drive if=sd'. All other code
paths use -blockdev. Thus this hunk and it's explanation makes no sense.
This will never be executed for USB cdroms.

If you want to add USB cdrom device frontend properties you need to
modify qemuBuildDiskDeviceProps.

Please note though that the 'usb-storage' device does not have a 'media'
parameter or anything similar.

> +
>      if (disk->src->readonly)
>          virBufferAddLit(&opt, ",readonly=on");
>  
> diff --git a/tests/qemuxml2argvdata/disk-cdrom-bus-other.x86_64-latest.args b/tests/qemuxml2argvdata/disk-cdrom-bus-other.x86_64-latest.args
> index de5fa083d8..38093423cf 100644
> --- a/tests/qemuxml2argvdata/disk-cdrom-bus-other.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/disk-cdrom-bus-other.x86_64-latest.args
> @@ -27,7 +27,7 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
>  -no-shutdown \
>  -boot strict=on \
>  -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
> --blockdev '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
> +-blockdev '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap","media":"cdrom"}' \
>  -blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"raw","file":"libvirt-2-storage"}' \

And this makes no sense either. You are modifying a test output file
which will not be formatted like this by the code. In fact with this
patch the test suite will fail.

Also -blockdev is the device backend so the placement doesn't make any
sense either.

For the next version please add a justification (description what the
actual problem is) in the first place. Do not attempt to modify anything
'-drive' related. Those code paths are deprecated and we don't add new
features for them. As noted, nowadays it's used only for disk bus='sd'.a

Also make sure you run the test-suite before posting patches