[PATCH 0/1] qemuDomainChangeNet: check virtio options for non-virtio models

Hi, in our project, we create all of our domain interfaces regardless of model with a <driver><host csum='off'/></driver>. The documentation is not clear as to which models support offloading or not, and the domain validates. In libvirtd, since <driver> is present, this causes each network device to have its virDomainVirtioOptions *virtio to be non-NULL, with all members ABSENT, which is also deemed valid. However, clients receive the interface elements without the driver. When the modified value is sent back via updateDeviceFlags, the candidate's virtio attribute is thus NULL. The validation in qemuDomainChangeNet requires both structs to be equal or both NULL, which is violated by such a request. This can be seen also with how virsh domif-setlink operates: virsh # domiflist 5 Interface Type Source Model MAC -------------------------------------------------------- - udp - e1000 52:54:00:1c:10:42 - udp - e1000 52:54:00:09:29:9c - udp - e1000 52:54:00:1d:a6:8d - udp - e1000 52:54:00:07:4a:83 52:54:00:1c:10:42 up virsh # domif-setlink 5 52:54:00:1c:10:42 down error: Failed to update interface link state error: Operation not supported: cannot modify virtio network device driver options A workaround is to add an empty <driver> in the update request, which works until libvirt is restarted. The driver element on a non-virtio interface is not present when the domain is reloaded, thus the pointer is NULL in olddev, causing the inverse issue and same error. Our current workaround is to retry with the empty driver element only if the first call fails, but we think libvirt should handle this struct more consistently. This patch expands the check to the non-virtio case, where the olddev is assumed to be valid, and newdev's virtio options are validated to be NULL or all-ABSENT. Since the driver element is normally preserved for virtio models, the stricter equality condition is left unchanged in that case. I did not investigate further if the discrepancy between NULL and all-ABSENT values for virDomainVirtioOptions can cause a problem elsewhere, or in other device kinds. Though I do wonder if the options handling should be improved. One possibility is that non-virtio devices keep the value NULL, e.g. after validating that the driver element does not contain any of the options, whereas virtio devices always create the struct even with no driver element present on input. Another option is that NULL pointer is treated the same as all-ABSENT struct, e.g. in virDomainCheckVirtioOptionsAreAbsent and virDomainVirtioOptionsCheckABIStability; the former does that while the latter does not. These functions would also need to become callable from the qemu driver. Miroslav Los (1): qemuDomainChangeNet: check virtio options for non-virtio models src/qemu/qemu_hotplug.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) -- 2.25.1

In a domain created with an interface with a <driver> subelement, the device contains a non-NULL virDomainVirtioOptions struct, even for non-virtio NIC models. The subelement need not be present again after libvirt restarts, or when the interface is passed to clients. When clients such as virsh domif-setlink put back the modified interface XML, the new device's virtio attribute is NULL. This may fail the equality checks for virtio options in qemuDomainChangeNet, depending on whether libvird was restarted since define or not. This patch modifies the check for non-virtio models, to ignore olddev value of virtio (assumed valid), and to allow either NULL or a struct with all values ABSENT in the new virtio options. Signed-off-by: Miroslav Los <mirlos@cisco.com> --- src/qemu/qemu_hotplug.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2756d2aebd..1f4620d833 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3681,6 +3681,7 @@ qemuDomainChangeNet(virQEMUDriver *driver, bool needVlanUpdate = false; bool needIsolatedPortChange = false; bool needQueryRxFilter = false; + bool isVirtio = false; int ret = -1; int changeidx = -1; g_autoptr(virConnect) conn = NULL; @@ -3742,7 +3743,9 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; } - if (virDomainNetIsVirtioModel(olddev) && + isVirtio = virDomainNetIsVirtioModel(olddev); + + if (isVirtio && (olddev->driver.virtio.name != newdev->driver.virtio.name || olddev->driver.virtio.txmode != newdev->driver.virtio.txmode || olddev->driver.virtio.ioeventfd != newdev->driver.virtio.ioeventfd || @@ -3769,12 +3772,18 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; } - if (!!olddev->virtio != !!newdev->virtio || - (olddev->virtio && newdev->virtio && - (olddev->virtio->iommu != newdev->virtio->iommu || - olddev->virtio->ats != newdev->virtio->ats || - olddev->virtio->packed != newdev->virtio->packed || - olddev->virtio->page_per_vq != newdev->virtio->page_per_vq))) { + if ((isVirtio && + (!!olddev->virtio != !!newdev->virtio || + (olddev->virtio && newdev->virtio && + (olddev->virtio->iommu != newdev->virtio->iommu || + olddev->virtio->ats != newdev->virtio->ats || + olddev->virtio->packed != newdev->virtio->packed || + olddev->virtio->page_per_vq != newdev->virtio->page_per_vq)))) || + (!isVirtio && newdev->virtio && + (newdev->virtio->iommu != 0 || + newdev->virtio->ats != 0 || + newdev->virtio->packed != 0 || + newdev->virtio->page_per_vq != 0))) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify virtio network device driver options")); goto cleanup; -- 2.25.1

On 7/4/24 17:44, Miroslav Los via Devel wrote:
In a domain created with an interface with a <driver> subelement, the device contains a non-NULL virDomainVirtioOptions struct, even for non-virtio NIC models. The subelement need not be present again after libvirt restarts, or when the interface is passed to clients.
When clients such as virsh domif-setlink put back the modified interface XML, the new device's virtio attribute is NULL. This may fail the equality checks for virtio options in qemuDomainChangeNet, depending on whether libvird was restarted since define or not.
This patch modifies the check for non-virtio models, to ignore olddev value of virtio (assumed valid), and to allow either NULL or a struct with all values ABSENT in the new virtio options.
Signed-off-by: Miroslav Los <mirlos@cisco.com> --- src/qemu/qemu_hotplug.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and merged. Congratulations on your first libvirt contribution! Michal
participants (2)
-
Michal Prívozník
-
Miroslav Los