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

"Daniel P. Berrange" <berrange(a)redhat.com&gt; 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 ?