On Tue, May 11, 2010 at 06:54:18AM -0400, Stefan Berger wrote:
"Daniel P. Berrange" <berrange@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 ?
The XML parsers shouldn't rely on platform #defines, so add a constant in the XML parser itself. Regardless of this, the actual implementation in qemu_driver.c should also check length boundary if the kernel API its using has a fixed buffer size Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|