9:52 a.m.
There is some validation of NetDefs (<interface>) that can't be done
until runtime, due to not knowing the exact configuration until that
time. This needs to happen in 3 places: 1) in the qemu commandline
when a new guest is started, 2) when an <interface> is hotplugged, and
3) when an <interface> is updated. Until now, there some (but not all)
validation copy-pasted between (1) and (2), and none of that was
present for (3). These patches move all the validation from (1) (which
is the most complete) into a separate function, and then call that
function from all three places, so the exact same validation is done
in all cases.
(These patches are a followup to a patch I sent *long* ago in a naive
attempt to fix https://bugzilla.redhat.com/1502754 - my original patch
did fix the problem when starting a guest with an invalid <interface>
config, but not when hotplugging or updating such an interface.)
NB: I noticed that if someone tries to specify <bandwidth> for an
interface type that doesn't support it, we only give a warning, not an
error, due to a fear that there are existing management apps that add
<bandwidth> to all types of interfaces, and we don't want them to
suddenly fail to run (I got this info from the BZ associated with the
commit that added the warning). This seems to me to be going a bit too
far - I think that (at least upstream) we should turn this into an
error, and let the regression testing of said management apps catch
the behavior change so they can fix their code. (insert
Kermit-drinking-coffee meme here)
Laine Stump (3):
conf: make arg to virDomainNetGetActualVirtPortProfile() a const
qemu: move runtime netdev validation into a separate function
qemu: call common NetDef validation for hotplug and device update
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 2 +-
src/qemu/qemu_command.c | 52 +--------------------------
src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 4 +++
src/qemu/qemu_hotplug.c | 32 +++++------------
6 files changed, 95 insertions(+), 77 deletions(-)
--
2.21.0
9:52 a.m.
New subject: [libvirt] [PATCH 1/3] conf: make arg to virDomainNetGetActualVirtPortProfile() a const
It needs to be used by a function that only has a const pointer to
virDomainNetDef.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 96e9223e21..848c831330 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29678,7 +29678,7 @@ virDomainNetGetActualHostdev(virDomainNetDefPtr iface)
}
virNetDevVPortProfilePtr
-virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface)
+virDomainNetGetActualVirtPortProfile(const virDomainNetDef *iface)
{
switch (iface->type) {
case VIR_DOMAIN_NET_TYPE_DIRECT:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 82631ecb07..b688ee2b83 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3181,7 +3181,7 @@ const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr
iface);
int virDomainNetGetActualDirectMode(virDomainNetDefPtr iface);
virDomainHostdevDefPtr virDomainNetGetActualHostdev(virDomainNetDefPtr iface);
virNetDevVPortProfilePtr
-virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface);
+virDomainNetGetActualVirtPortProfile(const virDomainNetDef *iface);
virNetDevBandwidthPtr
virDomainNetGetActualBandwidth(virDomainNetDefPtr iface);
virNetDevVlanPtr virDomainNetGetActualVlan(virDomainNetDefPtr iface);
--
2.21.0
9:52 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: move runtime netdev validation into a separate function
The same validation should be done for both static network devices and
hotplugged devices, but they are currently inconsistent. Move all the
relevant validation from qemuBuildInterfaceCommandLine() into the new
function qemuDomainValidateActualNetDef() and call the latter from
the former.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/qemu/qemu_command.c | 52 +--------------------------
src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 4 +++
3 files changed, 85 insertions(+), 51 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f795f2e987..2acae3bf33 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8352,50 +8352,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
if (!bootindex)
bootindex = net->info.bootIndex;
- /* Currently nothing besides TAP devices supports multiqueue. */
- if (net->driver.virtio.queues > 0 &&
- !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
- actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
- actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
- actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Multiqueue network is not supported for: %s"),
- virDomainNetTypeToString(actualType));
+ if (qemuDomainValidateActualNetDef(net, qemuCaps) < 0)
return -1;
- }
-
- /* and only TAP devices support nwfilter rules */
- if (net->filter) {
- virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net);
- if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
- actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("filterref is not supported for "
- "network interfaces of type %s"),
- virDomainNetTypeToString(actualType));
- return -1;
- }
- if (vport && vport->virtPortType != VIR_NETDEV_VPORT_PROFILE_NONE) {
- /* currently none of the defined virtualport types support iptables */
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("filterref is not supported for "
- "network interfaces with virtualport type %s"),
- virNetDevVPortTypeToString(vport->virtPortType));
- return -1;
- }
- }
-
- if (net->backend.tap &&
- !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
- actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Custom tap device path is not supported for: %s"),
- virDomainNetTypeToString(actualType));
- return -1;
- }
switch (actualType) {
case VIR_DOMAIN_NET_TYPE_NETWORK:
@@ -8458,14 +8416,6 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
requireNicdev = true;
- if (net->driver.virtio.queues > 1 &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("multi-queue is not supported for vhost-user "
- "with this QEMU binary"));
- goto cleanup;
- }
-
if (qemuInterfaceVhostuserConnect(driver, logManager, secManager,
cmd, def, net, qemuCaps, &chardev) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bd247628cb..ebbe1a85db 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5300,6 +5300,86 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev,
}
+
+int
+qemuDomainValidateActualNetDef(const virDomainNetDef *net,
+ virQEMUCapsPtr qemuCaps)
+{
+ /*
+ * Validations that can only be properly checked at runtime (after
+ * an <interface type='network'> has been resolved to its actual
+ * type.
+ *
+ * (In its current form this function can still be called before
+ * the actual type has been resolved (e.g. at domain definition
+ * time), but only if the validations would SUCCEED for
+ * type='network'.)
+ */
+ virDomainNetType actualType = virDomainNetGetActualType(net);
+
+ /* Only tap/macvtap devices support multiqueue. */
+ if (net->driver.virtio.queues > 1) {
+
+ if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+ actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
+ actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
+ actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
+ actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("multiqueue network is not supported for: %s"),
+ virDomainNetTypeToString(actualType));
+ return -1;
+ }
+
+ if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
+ qemuCaps && !virQEMUCapsGet(qemuCaps,
QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("multiqueue network is not supported for vhost-user
"
+ "with this QEMU binary"));
+ return -1;
+ }
+ }
+
+ /*
+ * Only standard tap devices support nwfilter rules, and even then only
+ * when *not* connected to an OVS bridge or midonet (indicated by having
+ * a <virtualport> element in the config)
+ */
+ if (net->filter) {
+ virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net);
+ if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+ actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
+ actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("filterref is not supported for "
+ "network interfaces of type %s"),
+ virDomainNetTypeToString(actualType));
+ return -1;
+ }
+ if (vport && vport->virtPortType != VIR_NETDEV_VPORT_PROFILE_NONE) {
+ /* currently none of the defined virtualport types support iptables */
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("filterref is not supported for "
+ "network interfaces with virtualport type %s"),
+ virNetDevVPortTypeToString(vport->virtPortType));
+ return -1;
+ }
+ }
+
+ if (net->backend.tap &&
+ !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+ actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
+ actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Custom tap device path is not supported for: %s"),
+ virDomainNetTypeToString(actualType));
+ return -1;
+ }
+
+ return 0;
+ }
+
+
static int
qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net)
{
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 3e87e75c3a..f53ea146e1 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1215,3 +1215,7 @@ qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg,
virDomainEventSuspendedDetailType
qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason);
+
+int
+qemuDomainValidateActualNetDef(const virDomainNetDef *net,
+ virQEMUCapsPtr qemuCaps);
--
2.21.0
3:02 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: move runtime netdev validation into a separate function
On 9/13/19 4:52 PM, Laine Stump wrote:
The same validation should be done for both static network devices
and
hotplugged devices, but they are currently inconsistent. Move all the
relevant validation from qemuBuildInterfaceCommandLine() into the new
function qemuDomainValidateActualNetDef() and call the latter from
the former.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/qemu/qemu_command.c | 52 +--------------------------
src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 4 +++
3 files changed, 85 insertions(+), 51 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f795f2e987..2acae3bf33 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8352,50 +8352,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
if (!bootindex)
bootindex = net->info.bootIndex;
- /* Currently nothing besides TAP devices supports multiqueue. */
- if (net->driver.virtio.queues > 0 &&
- !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
- actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
- actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
- actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Multiqueue network is not supported for: %s"),
- virDomainNetTypeToString(actualType));
+ if (qemuDomainValidateActualNetDef(net, qemuCaps) < 0)
return -1;
- }
-
- /* and only TAP devices support nwfilter rules */
- if (net->filter) {
- virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net);
- if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
- actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("filterref is not supported for "
- "network interfaces of type %s"),
- virDomainNetTypeToString(actualType));
- return -1;
- }
- if (vport && vport->virtPortType != VIR_NETDEV_VPORT_PROFILE_NONE) {
- /* currently none of the defined virtualport types support iptables */
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("filterref is not supported for "
- "network interfaces with virtualport type %s"),
- virNetDevVPortTypeToString(vport->virtPortType));
- return -1;
- }
- }
-
- if (net->backend.tap &&
- !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
- actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Custom tap device path is not supported for: %s"),
- virDomainNetTypeToString(actualType));
- return -1;
- }
switch (actualType) {
case VIR_DOMAIN_NET_TYPE_NETWORK:
@@ -8458,14 +8416,6 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
requireNicdev = true;
- if (net->driver.virtio.queues > 1 &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("multi-queue is not supported for vhost-user "
- "with this QEMU binary"));
- goto cleanup;
- }
-
if (qemuInterfaceVhostuserConnect(driver, logManager, secManager,
cmd, def, net, qemuCaps, &chardev) <
0)
goto cleanup;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bd247628cb..ebbe1a85db 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5300,6 +5300,86 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev,
}
+
3 empty lines instead of 2.
+int
+qemuDomainValidateActualNetDef(const virDomainNetDef *net,
+ virQEMUCapsPtr qemuCaps)
+{
+ /*
+ * Validations that can only be properly checked at runtime (after
+ * an <interface type='network'> has been resolved to its actual
+ * type.
+ *
+ * (In its current form this function can still be called before
+ * the actual type has been resolved (e.g. at domain definition
+ * time), but only if the validations would SUCCEED for
+ * type='network'.)
+ */
+ virDomainNetType actualType = virDomainNetGetActualType(net);
+
+ /* Only tap/macvtap devices support multiqueue. */
+ if (net->driver.virtio.queues > 1) {
I don't think that this is right. Take VIR_DOMAIN_NET_TYPE_USER for instance. It
doesn't allow anything but queues == 0.
+
+ if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+ actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
+ actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
+ actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
+ actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("multiqueue network is not supported for: %s"),
+ virDomainNetTypeToString(actualType));
+ return -1;
+ }
+> + if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
This is actually where a single queue can be permitred. At least according to the original
code.
+ qemuCaps && !virQEMUCapsGet(qemuCaps,
QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
NULL is never passed to qemuCaps, so no need to check it.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
+ _("multiqueue network is not supported for vhost-user
"
+ "with this QEMU binary"));
+ return -1;
+ }
+ }
+
+ /*
+ * Only standard tap devices support nwfilter rules, and even then only
+ * when *not* connected to an OVS bridge or midonet (indicated by having
+ * a <virtualport> element in the config)
+ */
+ if (net->filter) {
+ virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net);
+ if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+ actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
+ actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("filterref is not supported for "
+ "network interfaces of type %s"),
+ virDomainNetTypeToString(actualType));
+ return -1;
+ }
+ if (vport && vport->virtPortType != VIR_NETDEV_VPORT_PROFILE_NONE) {
+ /* currently none of the defined virtualport types support iptables */
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("filterref is not supported for "
+ "network interfaces with virtualport type %s"),
+ virNetDevVPortTypeToString(vport->virtPortType));
+ return -1;
+ }
+ }
+
+ if (net->backend.tap &&
+ !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+ actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
+ actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Custom tap device path is not supported for: %s"),
+ virDomainNetTypeToString(actualType));
+ return -1;
+ }
+
+ return 0;
+ }
s/^ //
ACK with this squashed in:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ebbe1a85db..fa0dd888c8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5300,7 +5300,6 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev,
}
-
int
qemuDomainValidateActualNetDef(const virDomainNetDef *net,
virQEMUCapsPtr qemuCaps)
@@ -5318,8 +5317,7 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
virDomainNetType actualType = virDomainNetGetActualType(net);
/* Only tap/macvtap devices support multiqueue. */
- if (net->driver.virtio.queues > 1) {
-
+ if (net->driver.virtio.queues > 0) {
if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
@@ -5331,8 +5329,9 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
return -1;
}
- if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
- qemuCaps && !virQEMUCapsGet(qemuCaps,
QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
+ if (net->driver.virtio.queues > 1 &&
+ actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("multiqueue network is not supported for vhost-user
"
"with this QEMU binary"));
@@ -5377,7 +5376,7 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
}
return 0;
- }
+}
Michal
4:36 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: move runtime netdev validation into a separate function
On 9/13/19 4:02 PM, Michal Prívozník wrote:
On 9/13/19 4:52 PM, Laine Stump wrote:
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5300,6 +5300,86 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef
*dev,
> }
>
>
> +
3 empty lines instead of 2.
:-O
> +int
> +qemuDomainValidateActualNetDef(const virDomainNetDef *net,
> + virQEMUCapsPtr qemuCaps)
> +{
> + /*
> + * Validations that can only be properly checked at runtime (after
> + * an <interface type='network'> has been resolved to its actual
> + * type.
> + *
> + * (In its current form this function can still be called before
> + * the actual type has been resolved (e.g. at domain definition
> + * time), but only if the validations would SUCCEED for
> + * type='network'.)
> + */
> + virDomainNetType actualType = virDomainNetGetActualType(net);
> +
> + /* Only tap/macvtap devices support multiqueue. */
> + if (net->driver.virtio.queues > 1) {
I don't think that this is right. Take VIR_DOMAIN_NET_TYPE_USER for instance. It
doesn't allow anything but queues == 0.
Right, I hadn't thought of the other non-tap types.
> +
> + if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
> + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> + actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("multiqueue network is not supported for:
%s"),
> + virDomainNetTypeToString(actualType));
> + return -1;
> + }
> +> + if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
This is actually where a single queue can be permitred. At least according to the
original code.
> + qemuCaps && !virQEMUCapsGet(qemuCaps,
QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
NULL is never passed to qemuCaps, so no need to check it.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("multiqueue network is not supported for
vhost-user "
> + "with this QEMU binary"));
> + return -1;
> + }
> + }
> +
> + /*
> + * Only standard tap devices support nwfilter rules, and even then only
> + * when *not* connected to an OVS bridge or midonet (indicated by having
> + * a <virtualport> element in the config)
> + */
> + if (net->filter) {
> + virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net);
> + if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("filterref is not supported for "
> + "network interfaces of type %s"),
> + virDomainNetTypeToString(actualType));
> + return -1;
> + }
> + if (vport && vport->virtPortType !=
VIR_NETDEV_VPORT_PROFILE_NONE) {
> + /* currently none of the defined virtualport types support iptables */
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("filterref is not supported for "
> + "network interfaces with virtualport type
%s"),
> + virNetDevVPortTypeToString(vport->virtPortType));
> + return -1;
> + }
> + }
> +
> + if (net->backend.tap &&
> + !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Custom tap device path is not supported for:
%s"),
> + virDomainNetTypeToString(actualType));
> + return -1;
> + }
> +
> + return 0;
> + }
s/^ //
That's strange - I ran make syntax-check multiple times, and it always
finds those...
ACK with this squashed in:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ebbe1a85db..fa0dd888c8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5300,7 +5300,6 @@ qemuDomainWatchdogDefValidate(const virDomainWatchdogDef *dev,
}
-
int
qemuDomainValidateActualNetDef(const virDomainNetDef *net,
virQEMUCapsPtr qemuCaps)
@@ -5318,8 +5317,7 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
virDomainNetType actualType = virDomainNetGetActualType(net);
/* Only tap/macvtap devices support multiqueue. */
- if (net->driver.virtio.queues > 1) {
-
+ if (net->driver.virtio.queues > 0) {
if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
@@ -5331,8 +5329,9 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
return -1;
}
- if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
- qemuCaps && !virQEMUCapsGet(qemuCaps,
QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
+ if (net->driver.virtio.queues > 1 &&
+ actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("multiqueue network is not supported for vhost-user
"
"with this QEMU binary"));
@@ -5377,7 +5376,7 @@ qemuDomainValidateActualNetDef(const virDomainNetDef *net,
}
return 0;
- }
+}
That all looks agreeable to me.
Thanks!
6:51 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: move runtime netdev validation into a separate function
On 9/13/19 4:02 PM, Michal Prívozník wrote:
On 9/13/19 4:52 PM, Laine Stump wrote:
> + qemuCaps && !virQEMUCapsGet(qemuCaps,
QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
NULL is never passed to qemuCaps, so no need to check it.
I had put that in so I could also call it from
qemuDomainDeviceDefValidateNetwork() (which doesn't have qemuCaps or
even a domainObjPtr to get it from) and catch as many invalid configs as
possible at parse time rather than runtime. But I wasn't happy with the
patch to do that so I haven't sent it (and anyway it looks like I can
just add qemuCaps to the args for that function), so I'll take it out
before pushing.
9:52 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: call common NetDef validation for hotplug and device update
qemuDomainAttachNetDevice() (hotplug) previously had some of the
validation that is in qemuDomainValidateActualNetDef(), but it was
incomplete. qemuDomainChangeNet() had none of that validation, but it
is all appropriate in both cases.
This is the final piece of a previously partial resolution to
https://bugzilla.redhat.com/1502754
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 32 ++++++++------------------------
1 file changed, 8 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index bd8868b0f7..0530bdd990 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1188,32 +1188,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
goto cleanup;
}
- actualType = virDomainNetGetActualType(net);
-
- /* Currently only TAP/macvtap devices supports multiqueue. */
- if (net->driver.virtio.queues > 0 &&
- !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
- actualType == VIR_DOMAIN_NET_TYPE_DIRECT ||
- actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
- actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Multiqueue network is not supported for: %s"),
- virDomainNetTypeToString(actualType));
+ /* final validation now that we have full info on the type */
+ if (qemuDomainValidateActualNetDef(net, priv->qemuCaps) < 0)
return -1;
- }
- /* and only TAP devices support nwfilter rules */
- if (net->filter &&
- !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
- actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("filterref is not supported for "
- "network interfaces of type %s"),
- virDomainNetTypeToString(actualType));
- return -1;
- }
+ actualType = virDomainNetGetActualType(net);
if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0)
goto cleanup;
@@ -3542,6 +3521,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainNetDefPtr newdev = dev->data.net;
virDomainNetDefPtr *devslot = NULL;
virDomainNetDefPtr olddev;
@@ -3749,6 +3729,10 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
goto cleanup;
}
+ /* final validation now that we have full info on the type */
+ if (qemuDomainValidateActualNetDef(newdev, priv->qemuCaps) < 0)
+ goto cleanup;
+
newType = virDomainNetGetActualType(newdev);
if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
--
2.21.0
3:02 p.m.
On 9/13/19 4:52 PM, Laine Stump wrote:
There is some validation of NetDefs (<interface>) that
can't be done
until runtime, due to not knowing the exact configuration until that
time. This needs to happen in 3 places: 1) in the qemu commandline
when a new guest is started, 2) when an <interface> is hotplugged, and
3) when an <interface> is updated. Until now, there some (but not all)
validation copy-pasted between (1) and (2), and none of that was
present for (3). These patches move all the validation from (1) (which
is the most complete) into a separate function, and then call that
function from all three places, so the exact same validation is done
in all cases.
(These patches are a followup to a patch I sent *long* ago in a naive
attempt to fix https://bugzilla.redhat.com/1502754 - my original patch
did fix the problem when starting a guest with an invalid <interface>
config, but not when hotplugging or updating such an interface.)
NB: I noticed that if someone tries to specify <bandwidth> for an
interface type that doesn't support it, we only give a warning, not an
error, due to a fear that there are existing management apps that add
<bandwidth> to all types of interfaces, and we don't want them to
suddenly fail to run (I got this info from the BZ associated with the
commit that added the warning). This seems to me to be going a bit too
far - I think that (at least upstream) we should turn this into an
error, and let the regression testing of said management apps catch
the behavior change so they can fix their code. (insert
Kermit-drinking-coffee meme here)
Yes, that was exactly the reasoning we had when introducing the warning.
Initially, when I implemented QoS it did not error out on unsupported
interface type, but I guess we can do so now.
Laine Stump (3):
conf: make arg to virDomainNetGetActualVirtPortProfile() a const
qemu: move runtime netdev validation into a separate function
qemu: call common NetDef validation for hotplug and device update
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 2 +-
src/qemu/qemu_command.c | 52 +--------------------------
src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 4 +++
src/qemu/qemu_hotplug.c | 32 +++++------------
6 files changed, 95 insertions(+), 77 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com> but please see my
comment to 2/3 before pushing.
Michal
4:22 p.m.
On 9/13/19 4:02 PM, Michal Prívozník wrote:
On 9/13/19 4:52 PM, Laine Stump wrote:
> There is some validation of NetDefs (<interface>) that can't be done
> until runtime, due to not knowing the exact configuration until that
> time. This needs to happen in 3 places: 1) in the qemu commandline
> when a new guest is started, 2) when an <interface> is hotplugged, and
> 3) when an <interface> is updated. Until now, there some (but not all)
> validation copy-pasted between (1) and (2), and none of that was
> present for (3). These patches move all the validation from (1) (which
> is the most complete) into a separate function, and then call that
> function from all three places, so the exact same validation is done
> in all cases.
>
> (These patches are a followup to a patch I sent *long* ago in a naive
> attempt to fix https://bugzilla.redhat.com/1502754 - my original patch
> did fix the problem when starting a guest with an invalid <interface>
> config, but not when hotplugging or updating such an interface.)
>
> NB: I noticed that if someone tries to specify <bandwidth> for an
> interface type that doesn't support it, we only give a warning, not an
> error, due to a fear that there are existing management apps that add
> <bandwidth> to all types of interfaces, and we don't want them to
> suddenly fail to run (I got this info from the BZ associated with the
> commit that added the warning). This seems to me to be going a bit too
> far - I think that (at least upstream) we should turn this into an
> error, and let the regression testing of said management apps catch
> the behavior change so they can fix their code. (insert
> Kermit-drinking-coffee meme here)
Yes, that was exactly the reasoning we had when introducing the warning.
Initially, when I implemented QoS it did not error out on unsupported
interface type, but I guess we can do so now.
Yep, I completely understand your reluctance at the time to make it a
full fledged error - especially if the patch was going to be backported
to a downstream build during a minor update :-)
I'm going to add some more things to this new validation function; I'll
see if I can slip in a patch for this too.
>
>
> Laine Stump (3):
> conf: make arg to virDomainNetGetActualVirtPortProfile() a const
> qemu: move runtime netdev validation into a separate function
> qemu: call common NetDef validation for hotplug and device update
>
> src/conf/domain_conf.c | 2 +-
> src/conf/domain_conf.h | 2 +-
> src/qemu/qemu_command.c | 52 +--------------------------
> src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_domain.h | 4 +++
> src/qemu/qemu_hotplug.c | 32 +++++------------
> 6 files changed, 95 insertions(+), 77 deletions(-)
>
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com> but please see my
comment to 2/3 before pushing.
Thanks!
1895
days inactive
1897
days old
8 comments
2 participants
participants (2)
-
Laine Stump -
Michal Prívozník