<tt>"Daniel P. Berrange" <berrange@redhat.com> wrote on 05/11/2010 06:25:05 AM: </tt> <tt>> > 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. </tt> <tt>To summarize here for now:</tt> <tt>The 1st provides parameters necessary to run a protocol between the </tt> <tt>host and the switch to setup switch parameters for a VM's</tt> <tt>traffic (for example). The protocol will be run by LLDPAD (daemon) </tt> <tt>getting the parameters passed via netlink messages where libvirt </tt> <tt>will (likely) send the message in form of a (netlink) multicast to</tt> <tt>be ignored by the kernel and handled by LLDPAD. The 1st is to</tt> <tt>support the (pre-)standard 802.1Qbg.</tt> <tt>The 2nd one provides a similar parameter necessary also for</tt> <tt>running a protocol between the host and the switch. In this case</tt> <tt>there will be support by the Ethernet adapter's firmware to run the</tt> <tt>protocol and libvirt will trigger it via a netlink message digested</tt> <tt>by the kernel + driver. This is to support the (pre-)standard 802.1Qbh.</tt> <tt> > > 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. > </tt> <tt>Ok. I'll do away with this then.</tt> <tt> > > } 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</tt> <tt>Ok.</tt> <tt>> > > > > > +#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</tt> <tt>Will change it.</tt> <tt> > > > + VIR_VSI_8021QBG, > > +#ifdef IFLA_VF_PORT_PROFILE_MAX > > + VIR_VSI_8021QBH, > > +#endif > > And here, etc</tt> <tt>Ok.</tt> <tt> > > > +}; > > + > > +/* 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</tt> <tt>The size of this character array is supposed to be 40 chars as per a kernel #define that will become available</tt> <tt>through some future kernel include and #define. I'd like to restrict the size of the profile id somewhere</tt> <tt>when parsing it... What's the best way to do that? Introduce a constant that also has 40 as value ?</tt> <tt> Stefan</tt>