On Fri, Aug 16, 2024 at 04:42:32PM +0200, Peter Krempa wrote:

Some code paths, such as if hotplug of an empty cdrom fails can cause that 'qemuBlockStorageSourceChainDetach' will be called with 'NULL' @data as there is no backend for the disk.

The above case became possible once we allowed hotplug of cdroms and subsequently fixed the case when users would hotplug an empty cdrom which ultimately caused the possibility of having no backend in the hotplug code path which was not possible before (see 'Fixes:' below and also the commit linked from there).

Make 'qemuBlockStorageSourceChainDetach' tolerate NULL @data by simply returning early.

Fixes: 894c6c5c1686cfbc1742493ed512a4795098b763 Resolves: https://issues.redhat.com/browse/RHEL-54550 Signed-off-by: Peter Krempa <pkrempa@redhat.com>

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

--- src/qemu/qemu_block.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index d6cdf521c4..6e90bae9f2 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1927,6 +1927,9 @@ qemuBlockStorageSourceChainDetach(qemuMonitor *mon, { size_t i;

+ if (!data) + return; + if (data->copyOnReadAttached) ignore_value(qemuMonitorBlockdevDel(mon, data->copyOnReadNodename));

-- 2.46.0

On Fri, Aug 16, 2024 at 04:42:33PM +0200, Peter Krempa wrote:

Attempting to start qemu with or hotplug an empty 'usb-storage' based disk results in the following error:

qemu-system-x86_64: -device {"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":true}: drive property not set

Reject such config at validation step and adjust tests.

It's weird that we even support usb cdrom as qemu cannot even emulate that, at least to my knowledge. Other than that the patch looks fine, but I cannot shake off the feeling of fishiness with this one. Could you bring me up to speed how this even happened to be "supported"? Thanks, Martin

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_validate.c | 6 ++++ .../disk-cdrom-bus-other.x86_64-latest.args | 5 ++-- .../disk-cdrom-bus-other.x86_64-latest.xml | 5 ---- .../qemuxmlconfdata/disk-cdrom-bus-other.xml | 5 ---- .../disk-cdrom-usb-empty.x86_64-latest.err | 1 + .../qemuxmlconfdata/disk-cdrom-usb-empty.xml | 29 +++++++++++++++++++ tests/qemuxmlconftest.c | 1 + 7 files changed, 39 insertions(+), 13 deletions(-) create mode 100644 tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err create mode 100644 tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index b885fe7c77..48abc3f850 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3037,6 +3037,12 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, return -1; }

+ if (virStorageSourceIsEmpty(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'usb' disk must not be empty")); + return -1; + } + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args index e0d2452a10..e1406af663 100644 --- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args +++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args @@ -27,9 +27,8 @@ 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","read-only":true}' \ --device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-2-storage","id":"usb-disk0","removable":false}' \ --device '{"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":false}' \ +-blockdev '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-1-storage","read-only":true}' \ +-device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-1-storage","id":"usb-disk0","removable":false}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml index 312e735947..f16fe63057 100644 --- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml @@ -23,11 +23,6 @@ <target dev='sda' bus='usb'/> <readonly/> </disk> - <disk type='file' device='cdrom'> - <driver name='qemu' type='raw'/> - <target dev='sdb' bus='usb'/> - <readonly/> - </disk> <controller type='usb' index='0' model='piix3-uhci'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml b/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml index 9564dc11be..119ce297a0 100644 --- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml +++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml @@ -20,11 +20,6 @@ <target dev='sda' bus='usb'/> <readonly/> </disk> - <disk type='file' device='cdrom'> - <driver name='qemu' type='raw'/> - <target dev='sdb' bus='usb'/> - <readonly/> - </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> <controller type='pci' index='0' model='pci-root'/> diff --git a/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err new file mode 100644 index 0000000000..a00cb0a4b2 --- /dev/null +++ b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err @@ -0,0 +1 @@ +unsupported configuration: 'usb' disk must not be empty diff --git a/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml new file mode 100644 index 0000000000..fc1cf45085 --- /dev/null +++ b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <target dev='sdb' bus='usb'/> + <readonly/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 89292af300..436d58b977 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1536,6 +1536,7 @@ mymain(void) DO_TEST_CAPS_LATEST("disk-cdrom"); DO_TEST_CAPS_LATEST("disk-cdrom-empty-network-invalid"); DO_TEST_CAPS_LATEST("disk-cdrom-bus-other"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-cdrom-usb-empty"); DO_TEST_CAPS_LATEST("disk-cdrom-network"); DO_TEST_CAPS_LATEST("disk-cdrom-tray"); DO_TEST_CAPS_LATEST("disk-floppy"); -- 2.46.0