10:44 a.m.
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
10:44 a.m.
New subject: [PATCH 1/1] qemuDomainChangeNet: check virtio options for non-virtio models
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(a)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
6:26 a.m.
New subject: [PATCH 1/1] qemuDomainChangeNet: check virtio options for
non-virtio models
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(a)cisco.com>
---
src/qemu/qemu_hotplug.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and merged. Congratulations on your first libvirt contribution!
Michal
137
days inactive
142
days old
2 comments
2 participants
participants (2)
-
Michal Prívozník -
Miroslav Los