[libvirt] [PATCH v5 01/24] conf: allow bandwidth parsing / formatting to include class ID

[Daniel P. Berrangé]

On Sun, May 19, 2019 at 06:16:54PM -0400, Laine Stump wrote:
> On 5/14/19 11:48 AM, Daniel P. Berrangé wrote:
> > The domain conf actual network def stores a <class id='3'/> element
> > separately from the <bandwidth>. The class ID should really just be
> > an attribute on the <bandwidth> element. We can't change existing
> > XML, and this isn't visible to users since it is internal XML only.
> > When we expose the new network port XML to users though, we should
> > get the design right.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> >   src/conf/domain_conf.c           |  6 ++++--
> >   src/conf/netdev_bandwidth_conf.c | 30 ++++++++++++++++++++++++++++--
> >   src/conf/netdev_bandwidth_conf.h |  2 ++
> >   src/conf/network_conf.c          |  8 ++++----
> >   tests/virnetdevbandwidthtest.c   |  1 +
> >   5 files changed, 39 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index a3a514136b..011d789feb 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -11270,6 +11270,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> >       bandwidth_node = virXPathNode("./bandwidth", ctxt);
> >       if (bandwidth_node &&
> >           virNetDevBandwidthParse(&actual->bandwidth,
> > +                                NULL,
> >                                   bandwidth_node,
> >                                   actual->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)
> >           goto error;
> > @@ -11609,6 +11610,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> >                   }
> >               } else if (virXMLNodeNameEqual(cur, "bandwidth")) {
> >                   if (virNetDevBandwidthParse(&def->bandwidth,
> > +                                            NULL,
> >                                               cur,
> >                                               def->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)
> >                       goto error;
> > @@ -25006,7 +25008,7 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf,
> >           return -1;
> >       if (virNetDevVPortProfileFormat(virDomainNetGetActualVirtPortProfile(def), buf) < 0)
> >           return -1;
> > -    if (virNetDevBandwidthFormat(virDomainNetGetActualBandwidth(def), buf) < 0)
> > +    if (virNetDevBandwidthFormat(virDomainNetGetActualBandwidth(def), 0, buf) < 0)
> >           return -1;
> >       return 0;
> >   }
> > @@ -25383,7 +25385,7 @@ virDomainNetDefFormat(virBufferPtr buf,
> >               return -1;
> >           if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0)
> >               return -1;
> > -        if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0)
> > +        if (virNetDevBandwidthFormat(def->bandwidth, 0, buf) < 0)
> >               return -1;
> >           /* ONLY for internal status storage - format the ActualNetDef
> > diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c
> > index 014941836d..9af2173b7b 100644
> > --- a/src/conf/netdev_bandwidth_conf.c
> > +++ b/src/conf/netdev_bandwidth_conf.c
> > @@ -99,6 +99,7 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate)
> >   /**
> >    * virNetDevBandwidthParse:
> >    * @bandwidth: parsed bandwidth
> > + * @class_id: parsed class ID
> >    * @node: XML node
> >    * @allowFloor: whether "floor" setting is supported
> >    *
> > @@ -110,6 +111,7 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate)
> >    */
> >   int
> >   virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
> > +                        unsigned int *class_id,
> 
> 
> Is there a reason you keep it separate from virNetDevBandwidthPtr, rather
> than just making it an attribute of the object, just as it is an object of
> the XML element? You'd still need to pass a bool into the Parse and Format
> functions, but it might make the code simpler (or at least easier to
> understand) elsewhere. Or it might be a NOP; I haven't thought it through.

As you say, we'd have to pass around an extra parameter to regardless
in the parse/format methods to say whether to include the class ID or
not.

If we did include it inthe bandwidth struct, that also implies more
refactoring across other APIs which pass bandwidth & class id (sometimes
even two class ids), which I didn't want to get into just yet.

> >                           bool allowFloor)
> >   {
> > @@ -117,6 +119,7 @@ virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
> >       virNetDevBandwidthPtr def = NULL;
> >       xmlNodePtr cur;
> >       xmlNodePtr in = NULL, out = NULL;
> > +    char *class_id_prop = NULL;
> >       if (VIR_ALLOC(def) < 0)
> >           return ret;
> > @@ -127,6 +130,22 @@ virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
> >           goto cleanup;
> >       }
> > +    class_id_prop = virXMLPropString(node, "classID");
> 
> 
> +1 on changing it from a single attribute inside its own element to a simple
> attribute :-)
> 
> 
> Reviewed-by: Laine Stump <laine at laine.org>
> 
> 
> (I'm fine with keeping class_id separate, just thought I'd mention it. Oh,
> and I assume later patches will end up adding stuff to test this addition to
> the parsing/formatting :-))

Yep, the next patch in the series has tests


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|