On 4/17/19 1:19 PM, Daniel P. Berrangé wrote:
> The virNetDevBandwidthParse method uses the interface type to decide
> whether to allow use of the "floor" parameter. Using the interface
> type is not convenient as callers may not have that available, but
> still wish to allow use of "floor".
Also, this is one of the things that gave rise to the distinction
between actualType == NETWORK and actualType == BRIDGE, and this patch
allows us to eliminate that (hopefully) without causing breakage.
> Switch to an explicit boolean
> to control its usage.
>
> Reviewed-by: Laine Stump <laine@laine.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> src/conf/domain_conf.c | 4 ++--
> src/conf/netdev_bandwidth_conf.c | 22 +++++++---------------
> src/conf/netdev_bandwidth_conf.h | 2 +-
> src/conf/network_conf.c | 4 ++--
> tests/virnetdevbandwidthtest.c | 2 +-
> 5 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 51aa48f421..0df3c2ed49 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11270,7 +11270,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> if (bandwidth_node &&
> virNetDevBandwidthParse(&actual->bandwidth,
> bandwidth_node,
> - actual->type) < 0)
> + def->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)
This bit here ^^^ doesn't compile, since def is undefined. You would
need to check "parent->type" instead.
> goto error;
>
> vlanNode = virXPathNode("./vlan", ctxt);
> @@ -11609,7 +11609,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> } else if (virXMLNodeNameEqual(cur, "bandwidth")) {
> if (virNetDevBandwidthParse(&def->bandwidth,
> cur,
> - def->type) < 0)
> + def->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)
> goto error;
> } else if (virXMLNodeNameEqual(cur, "vlan")) {
> if (virNetDevVlanParse(cur, ctxt, &def->vlan) < 0)
> diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c
> index 3113cde888..014941836d 100644
> --- a/src/conf/netdev_bandwidth_conf.c
> +++ b/src/conf/netdev_bandwidth_conf.c
> @@ -100,18 +100,18 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate)
> * virNetDevBandwidthParse:
> * @bandwidth: parsed bandwidth
> * @node: XML node
> - * @net_type: one of virDomainNetType
> + * @allowFloor: whether "floor" setting is supported
> *
> * Parse bandwidth XML and return pointer to structure.
> - * @net_type tell to which type will/is interface connected to.
> - * Pass -1 if this is not called on interface.
> + * The @allowFloor attribute indicates whether the caller
> + * is able to support use of the "floor" setting.
> *
> * Returns !NULL on success, NULL on error.
> */
> int
> virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
> xmlNodePtr node,
> - int net_type)
> + bool allowFloor)
> {
> int ret = -1;
> virNetDevBandwidthPtr def = NULL;
> @@ -162,17 +162,9 @@ virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
> goto cleanup;
> }
>
> - if (def->in->floor && net_type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> - if (net_type == -1) {
> - /* 'floor' on network isn't supported */
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("floor attribute isn't supported for "
> - "network's bandwidth yet"));
> - } else {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("floor attribute is supported only for "
> - "interfaces of type network"));
> - }
> + if (def->in->floor && !allowFloor) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("floor attribute is not supported for this config"));
> goto cleanup;
> }
> }
> diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h
> index cb1ffd29e0..7fe750ce27 100644
> --- a/src/conf/netdev_bandwidth_conf.h
> +++ b/src/conf/netdev_bandwidth_conf.h
> @@ -27,7 +27,7 @@
>
> int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
> xmlNodePtr node,
> - int net_type)
> + bool allowFloor)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> int virNetDevBandwidthFormat(virNetDevBandwidthPtr def,
> virBufferPtr buf);
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 78bff6f687..91562de269 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1188,7 +1188,7 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
>
> bandwidth_node = virXPathNode("./bandwidth", ctxt);
> if (bandwidth_node &&
> - virNetDevBandwidthParse(&def->bandwidth, bandwidth_node, -1) < 0)
> + virNetDevBandwidthParse(&def->bandwidth, bandwidth_node, false) < 0)
I think this is a bug, but even if it is, it is pre-existing. (The
bandwidth in a <portgroup> should be applied to any interface tagged
with that portgroup, i.e. it's not an attribute that's set for the
entire network, but for just one interface. So iiuc, allowFloor should
be true if the network is one that uses a routed connection (forward
mode route/nat/none))
> goto cleanup;
>
> vlanNode = virXPathNode("./vlan", ctxt);
> @@ -1682,7 +1682,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> }
>
> if ((bandwidthNode = virXPathNode("./bandwidth", ctxt)) &&
> - virNetDevBandwidthParse(&def->bandwidth, bandwidthNode, -1) < 0)
> + virNetDevBandwidthParse(&def->bandwidth, bandwidthNode, false) < 0)
> goto error;
>
> vlanNode = virXPathNode("./vlan", ctxt);
> diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
> index 96776fa033..23788b4164 100644
> --- a/tests/virnetdevbandwidthtest.c
> +++ b/tests/virnetdevbandwidthtest.c
> @@ -55,7 +55,7 @@ struct testSetStruct {
> \
> rc = virNetDevBandwidthParse(&(var), \
> ctxt->node, \
> - VIR_DOMAIN_NET_TYPE_NETWORK); \
> + true); \
> xmlFreeDoc(doc); \
> xmlXPathFreeContext(ctxt); \
> if (rc < 0) \
Assuming def->type is changed to parent->type in the first instance,
Reviewed-by: Laine Stump <laine@laine.org>
(also the issue in virNetworkPortGroupParseXML should be fixed, but in a
separate patch)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list