[libvirt] [PATCH 0/2] Two trivial MTU fixes
I'm inclined to pushed them as trivial, but since they have a bugzilla assigned I'd rather have a proper review. Michal Privoznik (2): qemu: Set iface MTU on hotplug qemuDomainChangeNet: Forbid changing MTU src/qemu/qemu_hotplug.c | 10 ++++++++++ 1 file changed, 10 insertions(+) -- 2.13.0
https://bugzilla.redhat.com/show_bug.cgi?id=1408701 While implementing MTU (572eda12ad and friends), I've forgotten to actually set MTU on the host NIC in case of hotplug. We correctly tell qemu on the monitor what the MTU should be, but we are not actually setting it on the host NIC. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6701bd9bc..8066acae3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1134,6 +1134,10 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } } + if (net->mtu && + virNetDevSetMTU(net->ifname, net->mtu) < 0) + goto cleanup; + for (i = 0; i < tapfdSize; i++) { if (qemuSecuritySetTapFDLabel(driver->securityManager, vm->def, tapfd[i]) < 0) -- 2.13.0
On 06/08/2017 07:50 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1408701
While implementing MTU (572eda12ad and friends), I've forgotten to actually set MTU on the host NIC in case of hotplug. We correctly tell qemu on the monitor what the MTU should be, but we are not actually setting it on the host NIC.
Well, it *was* being set by passing the configured MTU down into virNetDevTapCreateInBridgePort(), until I had to revert patch 2841e675, which automatically propagated the bridge's MTU back in the opposite direction (it turns out the same patch made MTU propagate in both directions).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6701bd9bc..8066acae3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1134,6 +1134,10 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } }
+ if (net->mtu && + virNetDevSetMTU(net->ifname, net->mtu) < 0) + goto cleanup;
On one hand, I dislike the idea that the MTU of the tap is now being set twice (once when it's created, to match the MTU of the bridge, and again here). On the other hand, by setting it here, you've taken care of type='direct' (macvtap) and type='ethernet' (tap not connected to any bridges) as well, which is a win, so ACK.
+ for (i = 0; i < tapfdSize; i++) { if (qemuSecuritySetTapFDLabel(driver->securityManager, vm->def, tapfd[i]) < 0)
Reviewed-by: Laine Stump <laine@laine.org>
https://bugzilla.redhat.com/show_bug.cgi?id=1447618 Currently, any attempt to change MTU on an interface that is plugged to a running domain is silently ignored. We should either do what's asked or error out. Well, we can update the host side of the interface, but we cannot change 'host_mtu' attribute for the virtio-net device. Therefore we have to error out. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8066acae3..d46956d98 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3138,6 +3138,12 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, /* vlan can be modified, and will be checked later */ /* linkstate can be modified */ + if (olddev->mtu != newdev->mtu) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot modify MTU")); + goto cleanup; + } + /* allocate new actual device to compare to old - we will need to * free it if we fail for any reason */ -- 2.13.0
On 06/08/2017 07:50 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1447618
Currently, any attempt to change MTU on an interface that is plugged to a running domain is silently ignored. We should either do what's asked or error out. Well, we can update the host side of the interface, but we cannot change 'host_mtu' attribute for the virtio-net device. Therefore we have to error out.
Interesting conundrum. There's nothing to stop a user from intervening directly in the guest and changing the guest's MTU from there. So if we allowed changing the mtu of the tap device this way, at least it would be *possible* to modify the MTU of an active interface. But if we did that, then behavior would be inconsistent between startup/hotplug time and runtime. So for now at least, ACK. Better to not allow something than to have it behave inconsistently.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
--- src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8066acae3..d46956d98 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3138,6 +3138,12 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, /* vlan can be modified, and will be checked later */ /* linkstate can be modified */
+ if (olddev->mtu != newdev->mtu) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot modify MTU")); + goto cleanup; + } + /* allocate new actual device to compare to old - we will need to * free it if we fail for any reason */
participants (2)
-
Laine Stump -
Michal Privoznik