1. Patchew
  2. Libvirt
  3. View series

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

Minglei Liu posted 1 patch 6 months, 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20231007081209.40305-1-minglei.liu@smartx.com
src/qemu/qemu_command.c                                    | 7 +++++++
.../disk-cdrom-bus-other.x86_64-latest.args                | 2 +-
2 files changed, 8 insertions(+), 1 deletion(-)
[PATCH] qemu: add 'media=cdrom' attribute for usb CDROM
Posted by Minglei Liu 6 months, 4 weeks ago 
From: "minglei.liu" <minglei.liu@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.
---
 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");
+
     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"}' \
 -device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-2-format","id":"usb-disk0","removable":false}' \
 -device '{"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":false}' \
-- 
2.41.0
Re: [PATCH] qemu: add 'media=cdrom' attribute for usb CDROM
Posted by Peter Krempa 6 months, 4 weeks ago 
On Sat, Oct 07, 2023 at 16:12:09 +0800, Minglei Liu wrote:
> From: "minglei.liu" <minglei.liu@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
Re: [PATCH] qemu: add 'media=cdrom' attribute for usb CDROM
Posted by Han Han 6 months, 4 weeks ago 
On Sat, Oct 7, 2023 at 4:12 PM Minglei Liu <minglei.liu@smartx.com> wrote:

> From: "minglei.liu" <minglei.liu@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.
>
From the results on qemu-kvm-8.1.1-1.fc39.x86_6, the parameter 'media' is
not supported for -blockdev:
qemu-kvm -blockdev
'{"driver":"file","filename":"/tmp/vdb","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap","media":"cdrom"}'
qemu-kvm: -blockdev
{"driver":"file","filename":"/tmp/vdb","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap","media":"cdrom"}:
Parameter 'media' is unexpected

> ---
>  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");
> +
>      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"}'
> \
>  -device
> '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-2-format","id":"usb-disk0","removable":false}'
> \
>  -device
> '{"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":false}'
> \
> --
> 2.41.0
>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.