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

* Stefan Berger (stefanb(a)linux.vnet.ibm.com) wrote: > Below is David Alan's original patch with lots of changes. > > In particular, it now parses the following two XML descriptions, one > for 802.1Qbg and 802.1Qbh and stored the data internally. The actual > triggering of the switch setup protocol has not been implemented > here but the relevant code to do that should go into the functions > associatePortProfileId() and disassociatePortProfileId(). For future work, have you thought about preassociate on migration?

> > Some of the changes from V1 to V2: > - parser and XML generator have been separated into their own > functions so they can be re-used elsewhere (passthrough case > for example) > - Adapted XML parser and generator support the above shown type > (802.1Qbg, 802.1Qbh). > - Adapted schema to above XML > - Adapted test XML to above XML > - Passing through the VM's UUID which seems to be necessary for > 802.1Qbh -- sorry no host UUID > - adding virtual function ID to association function, in case it's > necessary to use (for SR-IOV) > > Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com&gt; This is looking like pretty complete infrastructure to me. Have you been able generate test (dis)associate messages through this framework w/ the vsi patches? Pending that (I can see you are able to at least generate debug messages showing the thing work), and some minor nits below.

Acked-by: Chris Wright <chrisw(a)redhat.com&gt; > >From a945107f047c7cd71f9c1b74fd74c47d8cdc3670 Mon Sep 17 00:00:00 2001 > From: David Allan <dallan(a)redhat.com&gt; > Date: Fri, 12 Mar 2010 13:25:04 -0500 > Subject: [PATCH 1/1] POC of port profile id support > > * Modified schema per DanPB's feedback > * Added test for modified schema > --- > docs/schemas/domain.rng | 69 ++++++++++++++ > src/conf/domain_conf.c | 155 +++++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 35 +++++++ > src/qemu/qemu_conf.c | 18 +-- > src/qemu/qemu_conf.h | 5 - > src/qemu/qemu_driver.c | 17 +-- > src/util/macvtap.c | 151 +++++++++++++++++++++++++++----- > src/util/macvtap.h | 10 +- > tests/domainschemadata/portprofile.xml | 36 +++++++ > 9 files changed, 446 insertions(+), 50 deletions(-) > create mode 100644 tests/domainschemadata/portprofile.xml >

> Index: libvirt-acl/src/conf/domain_conf.c > =================================================================== > --- libvirt-acl.orig/src/conf/domain_conf.c > +++ libvirt-acl/src/conf/domain_conf.c > @@ -242,6 +242,11 @@ VIR_ENUM_IMPL(virDomainNetdevMacvtap, VI > "private", > "bridge") > > +VIR_ENUM_IMPL(virVirtualPort, VIR_VIRTUALPORT_TYPE_LAST, > + "none", > + "802.1Qbg", > + "802.1Qbh") > + > VIR_ENUM_IMPL(virDomainClockOffset, VIR_DOMAIN_CLOCK_OFFSET_LAST, > "utc", > "localtime", > @@ -1807,6 +1812,145 @@ cleanup: > } > > > +static void static int so we can capture an error?

> +virVirtualPortProfileDefParseXML(xmlNodePtr node, > + virVirtualPortProfileDefPtr virtPort) > +{ > + char *virtPortType; > + char *virtPortManagerID = NULL; > + char *virtPortTypeID = NULL; > + char *virtPortTypeIDVersion = NULL; > + char *virtPortInstanceID = NULL; > + char *virtPortProfileID = NULL; > + xmlNodePtr cur = node->children; int ret = -1; > + > + virtPortType = virXMLPropString(node, "type"); > + if (!virtPortType) > + return; > + > + while (cur != NULL) { > + if (xmlStrEqual(cur->name, BAD_CAST "parameters")) { > + > + virtPortManagerID = virXMLPropString(cur, "managerid"); > + virtPortTypeID = virXMLPropString(cur, "typeid"); > + virtPortTypeIDVersion = virXMLPropString(cur, "typeidversion"); > + virtPortInstanceID = virXMLPropString(cur, "instanceid"); > + virtPortProfileID = virXMLPropString(cur, "profileid"); > + > + break; s/break/goto out/ > + } > + > + cur = cur->next; > + } > + > + virtPort->virtPortType = VIR_VIRTUALPORT_NONE; > + > + switch (virVirtualPortTypeFromString(virtPortType)) { > + > + case VIR_VIRTUALPORT_8021QBG: > + if (virtPortManagerID != NULL && virtPortTypeID != NULL && > + virtPortTypeIDVersion != NULL) { > + unsigned int val; > + > + if ((virStrToLong_ui(virtPortManagerID, NULL, 10, &val) && > + virStrToLong_ui(virtPortManagerID, NULL, 16, &val) ) || Why not a baseless strtol (0 instead of 10 and 16)? And I think this should give some virDomainReportError() so that we can figure out what's happening when the xml is incorrect.

Ditto for parsing the whole thing... > + val > 0xff) > + break; s/break/goto out/

> + > + virtPort->u.virtPort8021Qbg.managerID = (uint8_t)val; > + > + if ((virStrToLong_ui(virtPortTypeID, NULL, 10, &val) && > + virStrToLong_ui(virtPortTypeID, NULL, 16, &val) ) || > + val > 0xffffff) > + break; > + > + virtPort->u.virtPort8021Qbg.typeID = (uint32_t)val; > + > + if ((virStrToLong_ui(virtPortTypeIDVersion, NULL, 10, &val) && > + virStrToLong_ui(virtPortTypeIDVersion, NULL, 16, &val) ) || > + val > 0xff) > + break; > + > + virtPort->u.virtPort8021Qbg.typeIDVersion = (uint8_t)val; > + > + if (virtPortInstanceID != NULL) { > + if (virUUIDParse(virtPortInstanceID, virtPort->u.virtPort8021Qbg.instanceID)) > + break; > + } else { > + if (virUUIDGenerate(virtPort->u.virtPort8021Qbg.instanceID)) > + break; > + } > + > + virtPort->virtPortType = VIR_VIRTUALPORT_8021QBG; > + } > + break; > + > + case VIR_VIRTUALPORT_8021QBH: > + if (virtPortProfileID != NULL) { > + if (virStrcpyStatic(virtPort->u.virtPort8021Qbh.profileID, > + virtPortProfileID) != NULL) > + virtPort->virtPortType = VIR_VIRTUALPORT_8021QBH; > + } > + break; > + > + > + default: > + case VIR_VIRTUALPORT_NONE: > + case VIR_VIRTUALPORT_TYPE_LAST: > + break; error cases, no? goto out

> + } > + ret = 0; out: > + VIR_FREE(virtPortManagerID); > + VIR_FREE(virtPortTypeID); > + VIR_FREE(virtPortTypeIDVersion); > + VIR_FREE(virtPortInstanceID); > + VIR_FREE(virtPortProfileID); > + VIR_FREE(virtPortType); return ret;

> +} > + > + > +static void > +virVirtualPortProfileFormat(virBufferPtr buf, virVirtualPortProfileDefPtr virtPort, > + const char *indent) > +{ > + char uuidstr[VIR_UUID_STRING_BUFLEN]; > + > + if (virtPort->virtPortType == VIR_VIRTUALPORT_NONE) > + return; > + > + virBufferVSprintf(buf, "%s<virtualport type='%s'>\n", > + indent, virVirtualPortTypeToString(virtPort->virtPortType)); > + > + switch (virtPort->virtPortType) { > + case VIR_VIRTUALPORT_NONE: > + case VIR_VIRTUALPORT_TYPE_LAST: > + break; > + > + case VIR_VIRTUALPORT_8021QBG: > + virUUIDFormat(virtPort->u.virtPort8021Qbg.instanceID, > + uuidstr); > + virBufferVSprintf(buf, > + "%s <parameters managerid='%d' typeid='%d' " > + "typeidversion='%d' instanceid='%s'/>\n", > + indent, > + virtPort->u.virtPort8021Qbg.managerID, > + virtPort->u.virtPort8021Qbg.typeID, > + virtPort->u.virtPort8021Qbg.typeIDVersion, > + uuidstr); > + break; > + > + case VIR_VIRTUALPORT_8021QBH: > + virBufferVSprintf(buf, > + "%s <parameters profileid='%s'/>\n", > + indent, > + virtPort->u.virtPort8021Qbh.profileID); > + break; > + } > + > + virBufferVSprintf(buf, "%s</virtualport>\n", indent); > +} > + > + > /* Parse the XML definition for a network interface > * @param node XML nodeset to parse for net definition > * @return 0 on success, -1 on failure > @@ -1832,6 +1976,8 @@ virDomainNetDefParseXML(virCapsPtr caps, > char *devaddr = NULL; > char *mode = NULL; > virNWFilterHashTablePtr filterparams = NULL; > + virVirtualPortProfileDef virtPort; > + bool virtPortParsed = false; > > if (VIR_ALLOC(def) < 0) { > virReportOOMError(); > @@ -1873,6 +2019,11 @@ virDomainNetDefParseXML(virCapsPtr caps, > xmlStrEqual(cur->name, BAD_CAST "source")) { > dev = virXMLPropString(cur, "dev"); > mode = virXMLPropString(cur, "mode"); > + } else if ((virtPortParsed == false) && > + (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) && > + xmlStrEqual(cur->name, BAD_CAST "virtualport")) { > + virVirtualPortProfileDefParseXML(cur, &virtPort); > + virtPortParsed = true; > } else if ((network == NULL) && > ((def->type == VIR_DOMAIN_NET_TYPE_SERVER) || > (def->type == VIR_DOMAIN_NET_TYPE_CLIENT) || > @@ -2048,6 +2199,8 @@ virDomainNetDefParseXML(virCapsPtr caps, > } else > def->data.direct.mode = VIR_DOMAIN_NETDEV_MACVTAP_MODE_VEPA; > > + def->data.direct.virtPortProfile = virtPort; > + > def->data.direct.linkdev = dev; > dev = NULL; > > @@ -5140,6 +5293,8 @@ virDomainNetDefFormat(virBufferPtr buf, > virBufferVSprintf(buf, " mode='%s'", > virDomainNetdevMacvtapTypeToString(def->data.direct.mode)); > virBufferAddLit(buf, "/>\n"); > + virVirtualPortProfileFormat(buf, &def->data.direct.virtPortProfile, > + " "); > break; > > case VIR_DOMAIN_NET_TYPE_USER: > Index: libvirt-acl/src/conf/domain_conf.h > =================================================================== > --- libvirt-acl.orig/src/conf/domain_conf.h > +++ libvirt-acl/src/conf/domain_conf.h > @@ -259,6 +259,39 @@ enum virDomainNetdevMacvtapType { > }; > > > +enum virVirtualPortType { > + VIR_VIRTUALPORT_NONE, > + VIR_VIRTUALPORT_8021QBG, > + VIR_VIRTUALPORT_8021QBH, > + > + VIR_VIRTUALPORT_TYPE_LAST, > +}; > + > +# ifdef IFLA_VF_PORT_PROFILE_MAX > +# define LIBVIRT_IFLA_VF_PORT_PROFILE_MAX IFLA_VF_PORT_PROFILE_MAX > +# else > +# define LIBVIRT_IFLA_VF_PORT_PROFILE_MAX 40 > +# endif > + > +/* profile data for macvtap (VEPA) */ > +typedef struct _virVirtualPortProfileDef virVirtualPortProfileDef; > +typedef virVirtualPortProfileDef *virVirtualPortProfileDefPtr; > +struct _virVirtualPortProfileDef { > + enum virVirtualPortType virtPortType; > + union { > + struct { > + uint8_t managerID; > + uint32_t typeID; // 24 bit valid > + uint8_t typeIDVersion; > + unsigned char instanceID[VIR_UUID_BUFLEN]; > + } virtPort8021Qbg; > + struct { > + char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX]; > + } virtPort8021Qbh; > + } u; > +}; > + > + > /* Stores the virtual network interface configuration */ > typedef struct _virDomainNetDef virDomainNetDef; > typedef virDomainNetDef *virDomainNetDefPtr; > @@ -290,6 +323,7 @@ struct _virDomainNetDef { > struct { > char *linkdev; > int mode; > + virVirtualPortProfileDef virtPortProfile; > } direct; > } data; > char *ifname; > @@ -1089,6 +1123,7 @@ VIR_ENUM_DECL(virDomainSeclabel) > VIR_ENUM_DECL(virDomainClockOffset) > > VIR_ENUM_DECL(virDomainNetdevMacvtap) > +VIR_ENUM_DECL(virVirtualPort) > > VIR_ENUM_DECL(virDomainTimerName) > VIR_ENUM_DECL(virDomainTimerTrack) > Index: libvirt-acl/src/util/macvtap.c > =================================================================== > --- libvirt-acl.orig/src/util/macvtap.c > +++ libvirt-acl/src/util/macvtap.c > @@ -43,6 +43,7 @@ > > # include "util.h" > # include "memory.h" > +# include "logging.h" > # include "macvtap.h" > # include "interface.h" > # include "conf/domain_conf.h" > @@ -57,6 +58,16 @@ > # define MACVTAP_NAME_PREFIX "macvtap" > # define MACVTAP_NAME_PATTERN "macvtap%d" > > + > +static int associatePortProfileId(const char *macvtap_ifname, > + const virVirtualPortProfileDefPtr virtPort, > + int vf, > + const unsigned char *vmuuid); > + > +static int disassociatePortProfileId(const char *macvtap_ifname, > + const virVirtualPortProfileDefPtr virtPort); > + > + > static int nlOpen(void) > { > int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE); > @@ -567,39 +578,38 @@ configMacvtapTap(int tapfd, int vnet_hdr > > return 0; > } > - > - > /** > * openMacvtapTap: > * Create an instance of a macvtap device and open its tap character > * device. > * @tgifname: Interface name that the macvtap is supposed to have. May > * be NULL if this function is supposed to choose a name > - * @macaddress: The MAC address for the macvtap device > - * @linkdev: The interface name of the NIC to connect to the external bridge > - * @mode_str: String describing the mode. Valid are 'bridge', 'vepa' and > - * 'private'. > + * @net: pointer to the virDomainNetDef object describing the direct > + * type if an interface > * @res_ifname: Pointer to a string pointer where the actual name of the > * interface will be stored into if everything succeeded. It is up > * to the caller to free the string. > + * @vnet_hdr: Whether to enable IFF_VNET_HDR on the interface > + * @vmuuid: The (raw) UUID of the VM > * > * Returns file descriptor of the tap device in case of success, > * negative value otherwise with error reported. > * > + * Open a macvtap device and trigger the switch setup protocol > + * if valid port profile parameters were provided. > */ > int > openMacvtapTap(const char *tgifname, > - const unsigned char *macaddress, > - const char *linkdev, > - int mode, > + virDomainNetDefPtr net, Any reason to keep tgifname now that you're passing full virDomainNetDefPtr?

> char **res_ifname, > - int vnet_hdr) > + int vnet_hdr, > + const unsigned char *vmuuid) > { > const char *type = "macvtap"; > int c, rc; > char ifname[IFNAMSIZ]; > int retries, do_retry = 0; > - uint32_t macvtapMode = macvtapModeFromInt(mode); > + uint32_t macvtapMode = macvtapModeFromInt(net->data.direct.mode); > const char *cr_ifname; > int ifindex; > > @@ -616,7 +626,7 @@ openMacvtapTap(const char *tgifname, > return -1; > } > cr_ifname = tgifname; > - rc = link_add(type, macaddress, 6, tgifname, linkdev, > + rc = link_add(type, net->mac, 6, tgifname, net->data.direct.linkdev, > macvtapMode, &do_retry); > if (rc) > return -1; > @@ -626,7 +636,8 @@ create_name: > for (c = 0; c < 8192; c++) { > snprintf(ifname, sizeof(ifname), MACVTAP_NAME_PATTERN, c); > if (ifaceGetIndex(false, ifname, &ifindex) == ENODEV) { > - rc = link_add(type, macaddress, 6, ifname, linkdev, > + rc = link_add(type, net->mac, 6, ifname, > + net->data.direct.linkdev, > macvtapMode, &do_retry); > if (rc == 0) > break; > @@ -639,6 +650,14 @@ create_name: > cr_ifname = ifname; > } > > + if (associatePortProfileId(cr_ifname, > + &net->data.direct.virtPortProfile, > + -1, > + vmuuid) != 0) { > + rc = -1; > + goto link_del_exit; > + } > + > rc = ifaceUp(cr_ifname); > if (rc != 0) { > virReportSystemError(errno, > @@ -647,7 +666,7 @@ create_name: > "MAC address"), > cr_ifname); > rc = -1; > - goto link_del_exit; > + goto disassociate_exit; > } > > rc = openTap(cr_ifname, 10); > @@ -656,14 +675,18 @@ create_name: > if (configMacvtapTap(rc, vnet_hdr) < 0) { > close(rc); > rc = -1; > - goto link_del_exit; > + goto disassociate_exit; > } > *res_ifname = strdup(cr_ifname); > } else > - goto link_del_exit; > + goto disassociate_exit; > > return rc; > > +disassociate_exit: > + disassociatePortProfileId(cr_ifname, > + &net->data.direct.virtPortProfile); > + > link_del_exit: > link_del(cr_ifname); > > @@ -673,14 +696,102 @@ link_del_exit: > > /** > * delMacvtap: > - * @ifname : The name of the macvtap interface > + * @net: pointer to virDomainNetDef object > * > - * Delete an interface given its name. > + * Delete an interface given its name. Disassociate > + * it with the switch if port profile parameters > + * were provided. > */ > void > -delMacvtap(const char *ifname) > +delMacvtap(virDomainNetDefPtr net) > { > - link_del(ifname); > + if (net->ifname) { > + disassociatePortProfileId(net->ifname, > + &net->data.direct.virtPortProfile); > + link_del(net->ifname); > + } > } > > #endif > + > + > +/** > + * associatePortProfile > + * > + * @macvtap_ifname: The name of the macvtap device > + * @virtPort: pointer to the object holding port profile parameters > + * @vf: virtual function number, -1 if to be ignored > + * @vmuuid : the UUID of the virtual machine > + * > + * Associate a port on a swtich with a profile. This function > + * may notify a kernel driver or an external daemon to run > + * the setup protocol. If profile parameters were not supplied > + * by the user, then this function returns without doing > + * anything. > + * > + * Returns 0 in case of success, != 0 otherwise with error > + * having been reported. > + */ > +static int > +associatePortProfileId(const char *macvtap_ifname, > + const virVirtualPortProfileDefPtr virtPort, > + int vf, > + const unsigned char *vmuuid) > +{ > + int rc = 0; > + VIR_DEBUG("Associating port profile '%p' on link device '%s'", > + virtPort, macvtap_ifname); > + (void)vf; > + (void)vmuuid; > + > + switch (virtPort->virtPortType) { > + case VIR_VIRTUALPORT_NONE: > + case VIR_VIRTUALPORT_TYPE_LAST: > + break; If we capture these on parsing and error out, then these could be invalid here.

> + > + case VIR_VIRTUALPORT_8021QBG: > + > + break; > + > + case VIR_VIRTUALPORT_8021QBH: > + > + break; > + } > + > + return rc; > +} > + > + > +/** > + * disassociatePortProfile > + * > + * @macvtap_ifname: The name of the macvtap device > + * @virtPort: point to object holding port profile parameters > + * > + * Returns 0 in case of success, != 0 otherwise with error > + * having been reported. > + */ > +static int > +disassociatePortProfileId(const char *macvtap_ifname, > + const virVirtualPortProfileDefPtr virtPort) > +{ > + int rc = 0; > + VIR_DEBUG("Disassociating port profile id '%p' on link device '%s' ", > + virtPort, macvtap_ifname); > + > + switch (virtPort->virtPortType) { > + case VIR_VIRTUALPORT_NONE: > + case VIR_VIRTUALPORT_TYPE_LAST: > + break; If we capture these on parsing and error out, then these could be invalid here.