Re: [PATCH v3 12/36] conf: don't pass interface type into virNetDevBandwidthParse

[Laine Stump <laine@xxxxxxxxx>]

On 3/19/19 8:46 AM, 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". Switch to an explicit boolean
> to control its usage.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>


Or we could just always look for floor and parse it, and decide whether 
or not it's valid somewhere later in validation. But this definitely 
doesn't harm anything, so


Reviewed-by: Laine Stump <laine@xxxxxxxxx>


> ---
>   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 35d965d2a3..d283feaca6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11147,7 +11147,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
>       if (bandwidth_node &&
>           virNetDevBandwidthParse(&actual->bandwidth,
>                                   bandwidth_node,
> -                                actual->type) < 0)
> +                                actual->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)
>           goto error;
>   
>       vlanNode = virXPathNode("./vlan", ctxt);
> @@ -11486,7 +11486,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 87bf158049..274f482bfc 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1187,7 +1187,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)
>           goto cleanup;
>   
>       vlanNode = virXPathNode("./vlan", ctxt);
> @@ -1681,7 +1681,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) \


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list