[libvirt] [PATCH] vepa: parsing for 802.1Qb{g|h} XML

[Stefan Berger]

"Daniel P. Berrange" <berrange at redhat.com> wrote on 05/11/2010 06:25:05 
AM:


> 
> On Mon, May 10, 2010 at 07:57:37PM -0400, Stefan Berger wrote:
> > Below is David Alan's original patch with lots of changes. 
> > 
> > In particular, it now parses the following XML and stored the data
> > internally. No sending of netlink messages has been implemented here.
> > 
> >    <interface type='direct'>
> >       <source dev='static' mode='vepa'/>
> >       <model type='virtio'/>
> >       <vsi managerid='12' typeid='0x123456' typeidversion='1'
> >            instanceid='fa9b7fff-b0a0-4893-8e0e-beef4ff18f8f' />
> >       <filterref filter='clean-traffic'/>
> >     </interface>
> > 
> >     <interface type='direct'>
> >       <source dev='static' mode='vepa'/>
> >       <model type='virtio'/>
> >       <vsi profileid='my_profile'/>
> >     </interface>
> 
> Could we have an explanation of what these attributes all mean in
> the commit message. 


To summarize here for now:
The 1st provides parameters necessary to run a protocol between the 
host and the switch to setup switch parameters for a VM's
traffic (for example). The protocol will be run by LLDPAD (daemon) 
getting the parameters passed via netlink messages where libvirt 
will (likely) send the message in form of a (netlink) multicast to
be ignored by the kernel and handled by LLDPAD. The 1st is to
support the (pre-)standard 802.1Qbg.

The 2nd one provides a similar parameter necessary also for
running a protocol between the host and the switch. In this case
there will be support by the Ethernet adapter's firmware to run the
protocol and libvirt will trigger it via a netlink message digested
by the kernel + driver. This is to support the (pre-)standard 802.1Qbh.

> 
> Also, when we have 2 different sets of attributes, we normally use
> a type attribute on the element to tell the parser what set of
> data to expect. So I think this should gain a 'type' attribute here.
> 
> > @@ -1873,6 +1879,20 @@ virDomainNetDefParseXML(virCapsPtr caps,
> >                         xmlStrEqual(cur->name, BAD_CAST "source")) {
> >                  dev  = virXMLPropString(cur, "dev");
> >                  mode = virXMLPropString(cur, "mode");
> > +            } else if ((vsiManagerID == NULL) &&
> > +                       (vsiTypeID == NULL) &&
> > +                       (vsiTypeIDVersion == NULL) &&
> > +                       (vsiInstanceID == NULL) &&
> > +                       (vsiProfileID == NULL) &&
> > +                       (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) &&
> > +                       xmlStrEqual(cur->name, BAD_CAST "vsi")) {
> > +                vsiManagerID = virXMLPropString(cur, "managerid");
> > +                vsiTypeID = virXMLPropString(cur, "typeid");
> > +                vsiTypeIDVersion = virXMLPropString(cur,
> > "typeidversion");
> > +                vsiInstanceID = virXMLPropString(cur, "instanceid");
> > +#ifdef IFLA_VF_PORT_PROFILE_MAX
> > +                vsiProfileID = virXMLPropString(cur, "profileid");
> > +#endif
> 
> XML parsing routines should not be #ifdefd. The XML format is formally
> defined by the schema and must never change based on compile time 
options.
> 

Ok. I'll do away with this then.

> >              } else if ((network == NULL) &&
> >                         ((def->type == VIR_DOMAIN_NET_TYPE_SERVER) ||
> >                          (def->type == VIR_DOMAIN_NET_TYPE_CLIENT) ||
> > @@ -2049,6 +2069,51 @@ virDomainNetDefParseXML(virCapsPtr caps,
> >          } else
> >              def->data.direct.mode =
> > VIR_DOMAIN_NETDEV_MACVTAP_MODE_VEPA;
> > 
> > +        vsi = &def->data.direct.vsiProfile;
> > +
> > +#ifdef IFLA_VF_PORT_PROFILE_MAX
> > +        if (vsiProfileID != NULL) {
> > +            if (virStrcpyStatic(vsi->u.vsi8021Qbh.profileID,
> > +                                vsiProfileID) != NULL) {
> > +                vsi->vsiType = VIR_VSI_8021QBH;
> > +                break;
> > +            }
> > +        }
> > +#endif
> 
> Likewise here

Ok.

> > 
> > 
> > +#define IFLA_VF_PORT_PROFILE_MAX 40
> > +enum virVSIType {
> > +    VIR_VSI_INVALID,
> 
> This isn't really 'INVALID' - this is better named 'NONE'
> since its simply an indication that this interface does not
> have any VSI info defined


Will change it.

> 
> > +    VIR_VSI_8021QBG,
> > +#ifdef IFLA_VF_PORT_PROFILE_MAX
> > +    VIR_VSI_8021QBH,
> > +#endif
> 
> And here, etc

Ok.

> 
> > +};
> > +
> > +/* profile data for macvtap (VEPA) */
> > +typedef struct _virVSIProfileDef virVSIProfileDef;
> > +typedef virVSIProfileDef *virVSIProfileDefPtr;
> > +struct _virVSIProfileDef {
> > +    enum virVSIType   vsiType;
> > +    union {
> > +        struct {
> > +            uint8_t       managerID;
> > +            uint32_t      typeID; // 24 bit valid
> > +            uint8_t       typeIDVersion;
> > +            unsigned char instanceID[VIR_UUID_BUFLEN];
> > +        } vsi8021Qbg;
> > +#ifdef IFLA_VF_PORT_PROFILE_MAX
> > +        struct {
> > +            char          profileID[IFLA_VF_PORT_PROFILE_MAX];
> > +        } vsi8021Qbh;
> > +#endif

The size of this character array is supposed to be 40 chars as per a 
kernel #define that will become available
through some future kernel include and #define. I'd like to restrict the 
size of the profile id somewhere
when parsing it... What's the best way to do that? Introduce a constant 
that also has 40 as value ?

   Stefan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100511/16d493b5/attachment-0001.htm>