December 2024 - Devel - libvirt List Archives

[RFC 0/4] meson: Enable -Wundef
by Andrea Bolognani 27 Feb '25

27 Feb '25

A few days ago I have posted a patch[1] that addresses an issue introduced when a meson check was dropped but some uses of the corresponding WITH_ macro were not removed at the same time. That got me thinking about what we can do to prevent such scenarios from happening again in the future. I have come up with something that I think would be effective, but since applying the approach throughout the entire codebase would require a non-trivial amount of work, I figured I'd ask for feedback before embarking on it. The idea is that there are two types of macros we can use for conditional compilation: external ones, coming from the OS or other libraries, and internal ones, which are the result of meson tests. The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only defined if they apply, so it is correct to check for their presence with #ifdef. Using #if will also work, as undefined macros evaluate to zero, but it's not good practice to use them that way. If -Wundef has been passed to the compiler, those incorrect uses will be reported (only on platforms where they are not defined, of course). The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar, but in this case we control their definition. This means that using means that the feature is not available on the machine we're building on, but it could also mean that we've removed the meson check and forgot to update all users of the macro. In this case, -Wundef would work 100% reliably to detect the issue: if the meson check doesn't exist, neither will the macro, regardless of what platform we're building on. So the approach I'm suggesting is to use a syntax-check rule to ensure that internal macros are only ever checked with #if instead of Of course this requires a full sweep to fix all cases in which we're not already doing things according to the proposal. Should be fairly easy, if annoying. A couple of examples are included here for demonstration purposes. The bigger impact is going to be on the build system. Right now we generally only define WITH_ macros if the check passed, but that will have to change and the result is going to be quite a bit of additional meson code I'm afraid. Thoughts? [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/SKJ… Andrea Bolognani (4): configmake: Check for WIN32 correctly meson: Always define WITH_*_DECL macros syntax-check: Ensure WITH_ macros are used correctly meson: Enable -Wundef build-aux/syntax-check.mk | 5 +++++ configmake.h.in | 2 +- meson.build | 3 +++ tests/virmockstathelpers.c | 28 ++++++++++++++-------------- 4 files changed, 23 insertions(+), 15 deletions(-) -- 2.43.2

4 8

[PATCH v5 00/18] *** qemu: block: Support block disk along with throttle filters ***
by Harikumar R 12 Feb '25

12 Feb '25

*** BLURB HERE *** Chun Feng Wu (17): schema: Add new domain elements to support multiple throttle groups schema: Add new domain elements to support multiple throttle filters config: Introduce ThrottleGroup and corresponding XML parsing config: Introduce ThrottleFilter and corresponding XML parsing qemu: monitor: Add support for ThrottleGroup operations tests: Test qemuMonitorJSONGetThrottleGroup and qemuMonitorJSONUpdateThrottleGroup remote: New APIs for ThrottleGroup lifecycle management qemu: Refactor qemuDomainSetBlockIoTune to extract common methods qemu: Implement qemu driver for throttle API qemu: helper: throttle filter nodename and preparation processing qemu: block: Support block disk along with throttle filters config: validate: Verify iotune, throttle group and filter qemuxmlconftest: Add 'throttlefilter' tests test_driver: Test throttle group lifecycle APIs virsh: Refactor iotune options for re-use virsh: Add support for throttle group operations virsh: Add option "throttle-groups" to "attach_disk" Harikumar Rajkumar (1): tests: Test qemuxmlactivetestThrottleGroup docs/formatdomain.rst | 47 ++ docs/manpages/virsh.rst | 135 +++- include/libvirt/libvirt-domain.h | 21 + src/conf/domain_conf.c | 398 ++++++++++ src/conf/domain_conf.h | 45 ++ src/conf/domain_validate.c | 119 ++- src/conf/schemas/domaincommon.rng | 293 ++++---- src/conf/virconftypes.h | 4 + src/driver-hypervisor.h | 22 + src/libvirt-domain.c | 174 +++++ src/libvirt_private.syms | 8 + src/libvirt_public.syms | 7 + src/qemu/qemu_block.c | 136 ++++ src/qemu/qemu_block.h | 49 ++ src/qemu/qemu_command.c | 180 +++++ src/qemu/qemu_command.h | 6 + src/qemu/qemu_domain.c | 73 +- src/qemu/qemu_driver.c | 619 +++++++++++++--- src/qemu/qemu_hotplug.c | 33 + src/qemu/qemu_monitor.c | 34 + src/qemu/qemu_monitor.h | 14 + src/qemu/qemu_monitor_json.c | 134 ++++ src/qemu/qemu_monitor_json.h | 14 + src/remote/remote_daemon_dispatch.c | 44 ++ src/remote/remote_driver.c | 40 ++ src/remote/remote_protocol.x | 48 +- src/remote_protocol-structs | 28 + src/test/test_driver.c | 452 ++++++++---- tests/qemumonitorjsontest.c | 86 +++ .../throttlefilter-in.xml | 392 ++++++++++ .../throttlefilter-out.xml | 393 ++++++++++ tests/qemuxmlactivetest.c | 1 + .../throttlefilter-invalid.x86_64-latest.err | 1 + .../throttlefilter-invalid.xml | 89 +++ .../throttlefilter.x86_64-latest.args | 55 ++ .../throttlefilter.x86_64-latest.xml | 105 +++ tests/qemuxmlconfdata/throttlefilter.xml | 95 +++ tests/qemuxmlconftest.c | 2 + tools/virsh-completer-domain.c | 82 +++ tools/virsh-completer-domain.h | 16 + tools/virsh-domain.c | 680 ++++++++++++++---- 41 files changed, 4649 insertions(+), 525 deletions(-) create mode 100644 tests/qemustatusxml2xmldata/throttlefilter-in.xml create mode 100644 tests/qemustatusxml2xmldata/throttlefilter-out.xml create mode 100644 tests/qemuxmlconfdata/throttlefilter-invalid.x86_64-latest.err create mode 100644 tests/qemuxmlconfdata/throttlefilter-invalid.xml create mode 100644 tests/qemuxmlconfdata/throttlefilter.x86_64-latest.args create mode 100644 tests/qemuxmlconfdata/throttlefilter.x86_64-latest.xml create mode 100644 tests/qemuxmlconfdata/throttlefilter.xml -- 2.39.5 (Apple Git-154)

4 51

[PATCH] nodedev: udev: Hook up virFileWaitForExist to address uevent, race of pci device
by Guoyi Tu 06 Feb '25

06 Feb '25

this commit addresses the same issue that as commit 1af45804 does. the following message is copying from that commit: If we find ourselves in the situation that the 'add' uevent has been fired earlier than the sysfs tree for a device was created, we should use the best-effort approach and give kernel some predetermined amount of time, thus waiting for the attributes to be ready rather than discarding the device from our device list forever. If those don't appear in the given time frame, we need to move on, since libvirt can't wait indefinitely. Signed-off-by: Guoyi Tu <tugy(a)chinatelecom.cn> --- src/node_device/node_device_udev.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1d8486f623..4a1786c21c 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -427,10 +427,19 @@ udevProcessPCI(virNodeDeviceDriverState *driver_state, virPCIEDeviceInfo *pci_express = NULL; virPCIDevice *pciDev = NULL; virPCIDeviceAddress devAddr = { 0 }; + g_autofree char *linkpath = NULL; int ret = -1; char *p; bool privileged = false; + linkpath = g_strdup_printf("%s/config", udev_device_get_syspath(device)); + if (virFileWaitForExists(linkpath, 10, 100) < 0) { + virReportSystemError(errno, + _("failed to wait for file '%1$s' to appear"), + linkpath); + goto cleanup; + } + VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) { privileged = driver_state->privileged; } -- 2.27.0 -- Guoyi

2 2

[PATCH] docs: fix 'virsh domstats --vcpu' measure units and descriptions
by Fabricio Duarte 05 Feb '25

05 Feb '25

The fields are in nanoseconds, not microseconds. Also fixes the description of `vcpu.<num>.wait`, as it does not actually represent the time waiting on I/O. Signed-off-by: Fabricio Duarte <fabricio.duarte.jr(a)gmail.com> --- docs/manpages/virsh.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 2e525d3fac..b0a21e019a 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2428,14 +2428,14 @@ When selecting the *--state* group the following fields are returned: * ``vcpu.<num>.state`` - state of the virtual CPU <num>, as number from virVcpuState enum * ``vcpu.<num>.time`` - virtual cpu time spent by virtual - CPU <num> (in microseconds) -* ``vcpu.<num>.wait`` - virtual cpu time spent by virtual - CPU <num> waiting on I/O (in microseconds) + CPU <num> (in nanoseconds) +* ``vcpu.<num>.wait`` - time the vCPU <num> wants to run, but the host + scheduler has something else running ahead of it (in nanoseconds) * ``vcpu.<num>.halted`` - virtual CPU <num> is halted: yes or no (may indicate the processor is idle or even disabled, depending on the architecture) * ``vcpu.<num>.delay`` - time the vCPU <num> thread was enqueued by the - host scheduler, but was waiting in the queue instead of running. + host scheduler, but was waiting in the queue instead of running (in nanoseconds). Exposed to the VM as a steal time. This group of statistics also reports additional hypervisor-originating per-vCPU -- 2.39.2

2 1

Investigating MAC Address Conflict Resolution in libvirt: Log Analysis and Code Location Inquiry
by Xuda Zhang 05 Feb '25

05 Feb '25

Dear Team, I am reaching out regarding an issue I encountered with libvirt and MAC address conflicts. Below is a summary of the situation: 1. Initially, the vNIC's MAC address was different from the target VM's MAC address. 2. After modifying the vNIC's MAC address to match the VM's MAC address, the network was interrupted. 3. After rebooting the VM, the vNIC's MAC address was automatically modified again. I have observed the following kernel logs during this process: Dec 24 16:59:40 zstack-manager kernel: br_enp2s0: port 14(vnic43.0) entered disabled stateDec 24 16:59:40 zstack-manager kernel: device vnic43.0 left promiscuous modeDec 24 16:59:40 zstack-manager kernel: br_enp2s0: port 14(vnic43.0) entered disabled stateDec 24 17:00:11 zstack-manager kernel: br_enp2s0: port 14(vnic43.0) entered blocking stateDec 24 17:00:11 zstack-manager kernel: br_enp2s0: port 14(vnic43.0) entered disabled stateDec 24 17:00:11 zstack-manager kernel: device vnic43.0 entered promiscuous modeDec 24 17:00:11 zstack-manager kernel: br_enp2s0: port 14(vnic43.0) entered blocking stateDec 24 17:00:11 zstack-manager kernel: br_enp2s0: port 14(vnic43.0) entered forwarding state I am looking to understand the underlying code that handles the automatic modification of the vNIC's MAC address after the conflict and how the network interruption occurs. Can you help direct me to the relevant code segment or provide any insights into this behavior? Thank you for your assistance. Best regards,

4 7

[PATCH v3 00/17] hw/microblaze: Allow running cross-endian vCPUs
by Philippe Mathieu-Daudé 05 Feb '25

05 Feb '25

Missing review: 4 (new) & 10 Since v2: - Addressed Richard's review comments Since v1: - Make device endianness configurable (Edgar) - Convert more Xilinx devices - Avoid preprocessor #if (Richard) - Add R-b tags Make machines endianness-agnostic, allowing to run a big-endian vCPU on the little-endian 'qemu-system-microblazeel' binary, and a little endian one on the big-endian 'qemu-system-microblaze' binary. Tests added, following combinations covered: - little-endian vCPU using little-endian binary (in-tree) - little-endian vCPU using big-endian binary (new) - big-endian vCPU using little-endian binary (new) - big-endian vCPU using big-endian binary (in-tree) To make a target endian-agnostic we need to remove the MO_TE uses. In order to do that, we propagate the MemOp from earlier in the call stack, or we extract it from the vCPU env (on MicroBlaze the CPU endianness is exposed by the 'ENDI' bit). Next step: Look at unifying binaries. Please review, Phil. Philippe Mathieu-Daudé (17): hw/microblaze: Restrict MemoryRegionOps are implemented as 32-bit hw/microblaze: Propagate CPU endianness to microblaze_load_kernel() hw/intc/xilinx_intc: Make device endianness configurable RFC hw/net/xilinx_ethlite: Simplify by having configurable endianness RFC hw/timer/xilinx_timer: Allow down to 8-bit memory access hw/timer/xilinx_timer: Make device endianness configurable hw/char/xilinx_uartlite: Make device endianness configurable hw/ssi/xilinx_spi: Make device endianness configurable hw/ssi/xilinx_spips: Make device endianness configurable hw/arm/xlnx-zynqmp: Use &error_abort for programming errors target/microblaze: Explode MO_TExx -> MO_TE | MO_xx target/microblaze: Set MO_TE once in do_load() / do_store() target/microblaze: Introduce mo_endian() helper target/microblaze: Consider endianness while translating code hw/microblaze: Support various endianness for s3adsp1800 machines tests/functional: Explicit endianness of microblaze assets tests/functional: Add microblaze cross-endianness tests hw/microblaze/boot.h | 4 +- include/hw/ssi/xilinx_spips.h | 1 + target/microblaze/cpu.h | 7 +++ hw/arm/xilinx_zynq.c | 1 + hw/arm/xlnx-zynqmp.c | 40 +++++-------- hw/char/xilinx_uartlite.c | 31 ++++++---- hw/intc/xilinx_intc.c | 50 ++++++++++++---- hw/microblaze/boot.c | 8 +-- hw/microblaze/petalogix_ml605_mmu.c | 4 +- hw/microblaze/petalogix_s3adsp1800_mmu.c | 58 ++++++++++++++++--- hw/microblaze/xlnx-zynqmp-pmu.c | 2 +- hw/net/xilinx_ethlite.c | 42 +++++++++----- hw/ppc/virtex_ml507.c | 1 + hw/ssi/xilinx_spi.c | 24 +++++--- hw/ssi/xilinx_spips.c | 36 +++++++----- hw/timer/xilinx_timer.c | 33 +++++++---- target/microblaze/translate.c | 49 ++++++++++------ .../functional/test_microblaze_s3adsp1800.py | 27 ++++++++- .../test_microblazeel_s3adsp1800.py | 25 +++++++- 19 files changed, 309 insertions(+), 134 deletions(-) -- 2.45.2

4 27

[RFC PATCH 0/5] qemu: Route hostdevs to multiple nested SMMUs
by Nathan Chen 30 Jan '25

30 Jan '25

Hi, This is a draft solution for supporting multiple vSMMU instances in a qemu VM. Based on discussions/suggestions received for a previous RFC by Nicolin here[0], the association of vSMMUs to VFIO devices in VM PCIe topology should be moved out of qemu into libvirt. In addition, the nested SMMU nodes should be passed to qemu as pluggable devices. To address these changes, this patch series introduces a new "nestedSmmuv3" IOMMU model and "nestedSmmuv3" device type. Upon specifying the nestedSmmuv3 IOMMU model, nestedSmmuv3 devices will be auto-added to the VM definition based on the available SMMU nodes in the host's sysfs. The nestedSmmuv3 devices will each be attached to a separate PXB controller, and VFIO devices will be routed to PXBs based on their association with host SMMU nodes. This will maintain a VM PCIe topology that allows for multiple nested SMMUs per Nicolin's original qemu patch series in [0] and Shameer's work in [1] to remove VM topology changes from qemu and allow the nested SMMUs to be specified as pluggable devices. For instance, if we specify the nestedSmmuv3 IOMMU model and a hostdev for passthrough: <devices> <hostdev mode='subsystem' type='pci' managed='no'> <source> <address domain='0x0009' bus='0x01' slot='0x00' function='0x0'/> </source> </hostdev> <iommu model='nestedSmmuv3'/> </devices> Libvirt will scan sysfs and populate the VM definition with controllers and nestedSmmuv3 devices based on host config. So if /sys/bus/pci/devices/0009:01:00.0/iommu is a symlink to the host SMMU node represented by /sys/devices/platform/arm-smmu-v3.8.auto/iommu/smmu3.0x0000000016000000 and there are 3 host SMMU nodes under /sys/class/iommu/, we'll see three auto-added nestedSmmuv3 devices, each routed to a pcie-expander-bus controller. Then the hostdev will be routed to a PXB controller that has a matching host SMMU node associated with it: <devices> ... <controller type='pci' index='1' model='pcie-expander-bus'> <model name='pxb-pcie'/> <target busNr='254'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> </controller> <controller type='pci' index='2' model='pcie-expander-bus'> <model name='pxb-pcie'/> <target busNr='251'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </controller> <controller type='pci' index='3' model='pcie-expander-bus'> <model name='pxb-pcie'/> <target busNr='249'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </controller> <controller type='pci' index='4' model='pcie-root-port'> <model name='pcie-root-port'/> <target chassis='7' port='0x8'/> <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> </controller> <hostdev mode='subsystem' type='pci' managed='no'> <source> <address domain='0x0009' bus='0x01' slot='0x00' function='0x0'/> </source> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/> </hostdev> <iommu model='nestedSmmuv3'/> <nestedSmmuv3> <name>smmu3.0x0000000012000000</name> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> </nestedSmmuv3> <nestedSmmuv3> <name>smmu3.0x0000000016000000</name> <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> </nestedSmmuv3> <nestedSmmuv3> <name>smmu3.0x0000000011000000</name> <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/> </nestedSmmuv3> <iommu model='nestedSmmuv3'/> </devices> TODO: - No DMA mapping can found by UEFI when specifying multiple passthrough devices in the VM definition, and VM boot is subsequently blocked. We need to investigate this for the next revision, but we don't encounter this issue when passing through a single device. We'll include iommufd support in the next revision to narrow down whether the required fix would be outside of libvirt. - Shameer's qemu branch specifies nestedSmmuv3 bus number with "pci-bus" instead of "bus", so the libvirt compilation test args and qemu args in qemuBuildPCINestedSmmuv3DevProps() need to be modified to match this revision of qemu. It will be reverted to using "bus" in the next qemu revision. - This patchset decrements PXB busNr based on how many devices are attached downstream, and the libvirt documentation states we must reserve busNr for the PXB itself in addition to any devices attached downstream. When I launch a VM and a PXB has a pcie-root-port and hostdev attached downstream, busNrs 253, 252, and 251 are reserved. But the PXB itself already has a bus number assigned via the <address/> attribute, and I see 253 and 252 assigned to the hostdev and pcie-root-port in the VM but not 251. Should we decrement busNr based on libvirt documentation or do we only need two busNrs 253 and 252 in the example here? This series is on Github: https://github.com/NathanChenNVIDIA/libvirt/tree/nested-smmuv3-12-05-24 Thanks, Nathan [0] https://lore.kernel.org/qemu-devel/cover.1719361174.git.nicolinc@nvidia.com/ [1] https://lore.kernel.org/qemu-devel/20241108125242.60136-1-shameerali.koloth… Signed-off-by: Nathan Chen <nathanc(a)nvidia.com> Nathan Chen (5): conf: Add a nestedSmmuv3 IOMMU model qemu: Implement and auto-add a nestedSmmuv3 device type qemu: Create PXBs and auto-assign VFIO devs and nested SMMUs qemu: Update PXB busNr for nestedSmmuv3 controllers qemu: Add test case for specifying multiple nested SMMUs docs/formatdomain.rst | 25 ++- src/ch/ch_domain.c | 1 + src/conf/domain_addr.c | 26 ++- src/conf/domain_addr.h | 4 +- src/conf/domain_conf.c | 188 +++++++++++++++++ src/conf/domain_conf.h | 15 ++ src/conf/domain_postparse.c | 1 + src/conf/domain_validate.c | 24 +++ src/conf/schemas/domaincommon.rng | 17 ++ src/conf/virconftypes.h | 2 + src/libvirt_private.syms | 2 + src/lxc/lxc_driver.c | 6 + src/qemu/qemu_command.c | 64 +++++- src/qemu/qemu_command.h | 4 + src/qemu/qemu_domain.c | 2 + src/qemu/qemu_domain_address.c | 193 ++++++++++++++++++ src/qemu/qemu_driver.c | 3 + src/qemu/qemu_hotplug.c | 5 + src/qemu/qemu_postparse.c | 1 + src/qemu/qemu_validate.c | 16 ++ src/test/test_driver.c | 4 + tests/meson.build | 1 + .../iommu-nestedsmmuv3.aarch64-latest.args | 38 ++++ .../iommu-nestedsmmuv3.aarch64-latest.xml | 61 ++++++ tests/qemuxmlconfdata/iommu-nestedsmmuv3.xml | 29 +++ tests/qemuxmlconftest.c | 4 +- tests/schemas/device.rng.in | 1 + tests/virnestedsmmuv3mock.c | 57 ++++++ 28 files changed, 788 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxmlconfdata/iommu-nestedsmmuv3.aarch64-latest.args create mode 100644 tests/qemuxmlconfdata/iommu-nestedsmmuv3.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/iommu-nestedsmmuv3.xml create mode 100644 tests/virnestedsmmuv3mock.c -- 2.34.1

4 18

Re: [PATCH] Сheck snapshot disk is not NULL when searching it in the VM config
by Peter Krempa 29 Jan '25

29 Jan '25

On Mon, May 20, 2024 at 14:48:47 +0000, Efim Shevrin via Devel wrote: > Hello, > > > If vmdisk is NULL, shouldn't this function (qemuSnapshotDeleteValidate()) return an error? > > I think this qemuSnapshotDeleteValidate should not return an error. > > It seems to me that when vmdisk is NULL, this does not invalidate > the snapshot itself, but indicates that the config has changed since > the snapshot was done. And if the VM config has changed, this adds evidence that the snapshot should be deleted, > because the snapshot does not reflect the real vm config. > > Since we do not have an analogue of the --force option for deleting a snapshot, in the case when qemuSnapshotDeleteValidate returns > an error when vmdisk is NULL, we will never delete a snapshot which has invalid disk. Snapshot deletion does have something that can be considered force and that is the '--metadata' option that removes just the snapshot definition (metadata) and doesn't touch the disk images. > > Similarly, disk can be NULL too > Thank you for the comment regarding the disk variable. I`ve reworked patch. > > When creating a snapshot of a VM with multiple hard disks, > the snapshot takes into account the presence of all disks > in the system. If, over time, one of the disks is deleted, > the snapshot will continue to store knowledge of the deleted disk. > This results in the fact that at the moment of deleting the snapshot, > at the validation stage, a disk from the snapshot will be searched which > is not in the VM configuration. As a result, vmdisk variable will > be equal to NULL. Dereferencing a null pointer at the time of calling > virStorageSourceIsSameLocation(vmdisk->src, disk->src) > will result in SIGSEGV. Crashing is obviously not okay ... > Also, the disk variable can also be equal to NULL and this > requires to check that disk != NULL before calling the > virStorageSourceIsSameLocation function to avoid SIGSEGV. .. but going ahead with the snapshot deletion isn't always okay either. The disk isn't referenced by the VM so the disk state can't be merged, while the state would be merged for any other disk. When reverting back to a previous snapshot, which is still referencing the older state of the disk which was removed from the VM, the VM would see that the image state of disks that were present at deletion would contain the merged state, but only a partial state for the disk which was later removed.

3 3

[PATCH] qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition
by kaihuan 29 Jan '25

29 Jan '25

qemuDomainDiskByName() can return a NULL pointer on failure. But this returned value in qemuSnapshotDeleteValidate is not checked.It will make libvirtd crash. Signed-off-by: kaihuan <jungleman759(a)gmail.com> --- src/qemu/qemu_snapshot.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 18b2e478f6..bcbd913073 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -4242,8 +4242,19 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, virDomainDiskDef *vmdisk = NULL; virDomainDiskDef *disk = NULL; - vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name); - disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name); + if (!(vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("disk '%1$s' referenced by snapshot '%2$s' not found in the current definition"), + snapDisk->name, snap->def->name); + return -1; + } + + if (!(disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("disk '%1$s' referenced by snapshot '%2$s' not found in the VM definition of the deleted snapshot"), + snapDisk->name, snap->def->name); + return -1; + } if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, -- 2.33.1.windows.1

3 3

[PATCH] Support IDE/SATA disk 'product' parameter
by Adam Julis 29 Jan '25

29 Jan '25

Since we supported 'product' parameter for SCSI, just expanded existing solution makes IDE/SATA parameter works too. QEMU requires parameter 'model' in case of IDE/SATA (instead of 'product'), so the process of making JSON object is slightly modified for that. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/697 Signed-off-by: Adam Julis <ajulis(a)redhat.com> --- docs/formatdomain.rst | 7 ++-- src/qemu/qemu_command.c | 11 +++++- src/qemu/qemu_validate.c | 14 ++++++-- ...disk-product-build-error.x86_64-latest.err | 1 + .../disk-scsi-disk-product-build-error.xml | 34 +++++++++++++++++++ ...-disk-vendor-build-error.x86_64-latest.err | 1 + ... => disk-scsi-disk-vendor-build-error.xml} | 0 ...csi-disk-vpd-build-error.x86_64-latest.err | 1 - .../disk-scsi-disk-vpd.x86_64-latest.args | 4 +-- .../disk-scsi-disk-vpd.x86_64-latest.xml | 7 ++-- tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml | 4 +-- tests/qemuxmlconftest.c | 3 +- 12 files changed, 71 insertions(+), 16 deletions(-) create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml create mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err rename tests/qemuxmlconfdata/{disk-scsi-disk-vpd-build-error.xml => disk-scsi-disk-vendor-build-error.xml} (100%) delete mode 100644 tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 60bee8bd4f..c93a321401 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3551,12 +3551,13 @@ paravirtualized driver is specified via the ``disk`` element. :since:`Since 0.10.1` ``vendor`` If present, this element specifies the vendor of a virtual hard disk or - CD-ROM device. It must not be longer than 8 printable characters. - :since:`Since 1.0.1` + CD-ROM device. It must not be longer than 8 printable characters. Only for + devices using 'scsi' ``bus``. :since:`Since 1.0.1` ``product`` If present, this element specifies the product of a virtual hard disk or CD-ROM device. It must not be longer than 16 printable characters. - :since:`Since 1.0.1` + Only for devices using 'scsi' (:since:`Since 1.0.1`), 'sata' or 'ide' ``bus``. + :since:`Since 11.0.0` ``address`` If present, the ``address`` element ties the disk to a given slot of a controller (the actual ``<controller>`` device can often be inferred by diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dcb9c4934e..5c38858f5d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1628,6 +1628,11 @@ qemuBuildDiskDeviceProps(const virDomainDef *def, else driver = "ide-hd"; + if (virJSONValueObjectAdd(&props, + "S:model", disk->product, + NULL) < 0) + return NULL; + break; case VIR_DOMAIN_DISK_BUS_SCSI: @@ -1654,6 +1659,11 @@ qemuBuildDiskDeviceProps(const virDomainDef *def, } } + if (virJSONValueObjectAdd(&props, + "S:product", disk->product, + NULL) < 0) + return NULL; + break; case VIR_DOMAIN_DISK_BUS_VIRTIO: { @@ -1803,7 +1813,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def, "A:wwn", &wwn, "p:rotation_rate", disk->rotation_rate, "S:vendor", disk->vendor, - "S:product", disk->product, "T:removable", removable, "S:write-cache", qemuOnOffAuto(writeCache), "p:cyls", disk->geometry.cylinders, diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index aaa056379e..f0be236533 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2947,10 +2947,20 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, } } - if (disk->vendor || disk->product) { + if (disk->vendor) { if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only scsi disk supports vendor and product")); + _("Only scsi disk supports vendor")); + return -1; + } + } + + if (disk->product) { + if ((disk->bus != VIR_DOMAIN_DISK_BUS_IDE) && + (disk->bus != VIR_DOMAIN_DISK_BUS_SATA) && + (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only ide, sata and scsi disk supports product")); return -1; } } diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err new file mode 100644 index 0000000000..93dfac0d1e --- /dev/null +++ b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.x86_64-latest.err @@ -0,0 +1 @@ +unsupported configuration: Only ide, sata and scsi disk supports product diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml new file mode 100644 index 0000000000..da2fc59da3 --- /dev/null +++ b/tests/qemuxmlconfdata/disk-scsi-disk-product-build-error.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</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='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='virtio'/> + <product>ST3146707LC</product> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='sdb' bus='scsi'/> + <vendor>SEAGATE</vendor> + <product>ST3567807GD</product> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='1' model='lsilogic'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err new file mode 100644 index 0000000000..88bd9e5468 --- /dev/null +++ b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.x86_64-latest.err @@ -0,0 +1 @@ +unsupported configuration: Only scsi disk supports vendor diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.xml similarity index 100% rename from tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.xml rename to tests/qemuxmlconfdata/disk-scsi-disk-vendor-build-error.xml diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err b/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err deleted file mode 100644 index f70b7a774f..0000000000 --- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd-build-error.x86_64-latest.err +++ /dev/null @@ -1 +0,0 @@ -unsupported configuration: Only scsi disk supports vendor and product diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args index 4234a7e677..1d3aaf3819 100644 --- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args +++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.args @@ -30,9 +30,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \ -device '{"driver":"virtio-scsi-pci","id":"scsi0","bus":"pci.0","addr":"0x2"}' \ -device '{"driver":"lsi","id":"scsi1","bus":"pci.0","addr":"0x3"}' \ -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-2-storage","read-only":true}' \ --device '{"driver":"scsi-cd","bus":"scsi0.0","channel":0,"scsi-id":0,"lun":0,"device_id":"drive-scsi0-0-0-0","drive":"libvirt-2-storage","id":"scsi0-0-0-0","vendor":"SEAGATE","product":"ST3146707LC"}' \ +-device '{"model":"ST3146707LC","driver":"ide-cd","bus":"ide.0","unit":0,"drive":"libvirt-2-storage","id":"ide0-0-0"}' \ -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest2","node-name":"libvirt-1-storage","read-only":true}' \ --device '{"driver":"scsi-hd","bus":"scsi1.0","scsi-id":0,"device_id":"drive-scsi1-0-0","drive":"libvirt-1-storage","id":"scsi1-0-0","bootindex":1,"vendor":"SEA GATE","product":"ST67 807GD"}' \ +-device '{"product":"ST67 807GD","driver":"scsi-hd","bus":"scsi1.0","scsi-id":0,"device_id":"drive-scsi1-0-0","drive":"libvirt-1-storage","id":"scsi1-0-0","bootindex":1}' \ -audiodev '{"id":"audio1","driver":"none"}' \ -device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x4"}' \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml index 4b23fbfcfe..39148f6ce7 100644 --- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml +++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.x86_64-latest.xml @@ -20,9 +20,8 @@ <disk type='block' device='cdrom'> <driver name='qemu' type='raw'/> <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='sda' bus='scsi'/> + <target dev='sda' bus='ide'/> <readonly/> - <vendor>SEAGATE</vendor> <product>ST3146707LC</product> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> @@ -31,7 +30,6 @@ <source dev='/dev/HostVG/QEMUGuest2'/> <target dev='sdb' bus='scsi'/> <readonly/> - <vendor>SEA GATE</vendor> <product>ST67 807GD</product> <address type='drive' controller='1' bus='0' target='0' unit='0'/> </disk> @@ -45,6 +43,9 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </controller> <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <audio id='1' type='none'/> diff --git a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml index 36dd2a89ba..e3665d3afa 100644 --- a/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml +++ b/tests/qemuxmlconfdata/disk-scsi-disk-vpd.xml @@ -16,9 +16,8 @@ <emulator>/usr/bin/qemu-system-x86_64</emulator> <disk type='block' device='cdrom'> <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='sda' bus='scsi'/> + <target dev='sda' bus='ide'/> <readonly/> - <vendor>SEAGATE</vendor> <product>ST3146707LC</product> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> @@ -26,7 +25,6 @@ <source dev='/dev/HostVG/QEMUGuest2'/> <target dev='sdb' bus='scsi'/> <readonly/> - <vendor>SEA GATE</vendor> <product>ST67 807GD</product> <address type='drive' controller='1' bus='0' target='0' unit='0'/> </disk> diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c index 21b56dc94e..083b0ab7f6 100644 --- a/tests/qemuxmlconftest.c +++ b/tests/qemuxmlconftest.c @@ -1620,7 +1620,8 @@ mymain(void) DO_TEST_CAPS_LATEST("disk-scsi-disk-split"); DO_TEST_CAPS_LATEST("disk-scsi-disk-wwn"); DO_TEST_CAPS_LATEST("disk-scsi-disk-vpd"); - DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-vpd-build-error"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-vendor-build-error"); + DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-scsi-disk-product-build-error"); DO_TEST_CAPS_LATEST("controller-virtio-scsi"); DO_TEST_CAPS_LATEST("controller-scsi-auto"); DO_TEST_CAPS_LATEST("disk-sata-device"); -- 2.47.0

2 5