On Fri, 2010-05-21 at 16:46 -0700, Chris Wright wrote:
> * Stefan Berger (stefanb@xxxxxxxxxxxxxxxxxx) 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?
You mean first to do a preassociate and then after migration has
succeeded a (full) associate. I had thought about it. I believe the
current code would need to know what the actual cause was for it being
called. We currently handle it I believe correctly for
- 'direct' type of interface creation and destruction
- resume after a suspend
Now if we wanted to pre-associate on migration on the destination host,
we would need to know that the function was called due to a migration
operation in order not to call the associate function but rather the
pre-associate one. The I believe upon failure of migration the
disassociate would still be call-able on the target host, but in
addition we would need to call the associate function after the
migration was successful. We're not doing this. I suppose also that
nothing would be allowed to go wrong once one switches from
pre-associate to associate in such a scenario since the migration
succeeded and a failure to associate couldn't be handled gracefully
anymore (by failing 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@xxxxxxxxxx>
>
> 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.
I have been able to communicate with a self-hacked dummy server for
802.1Qbg that receives a netlink message and responds with a netlink
message via unicast to libvirt. With that server I am currently sending
the response immediately to the RTM_SETLINK message, which I believe
isn't what the kernel is doing. Rather it should send it back upon
RTM_GETLINK. If that server is missing or indicates failure to start, I
am timing out and fail the start of the VM. The disassociate part is in
the path of the tear down code and is being invoked as well. I haven't
tested much with that for 802.1Qbg.
>
> Acked-by: Chris Wright <chrisw@xxxxxxxxxx>
>
> > >From a945107f047c7cd71f9c1b74fd74c47d8cdc3670 Mon Sep 17 00:00:00 2001
> > From: David Allan <dallan@xxxxxxxxxx>
> > 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?
Ok.
>
> > +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.
Done.
>
> Ditto for parsing the whole thing...
>
> > + val > 0xff)
> > + break;
>
> s/break/goto out/
>
Ok.
> > +
> > + 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
Ok. However, I'd still allow the omission of <virtualport/> so I can
start a macvtap without having to have a (future) switch.
>
> > + }
> > +
>
> ret = 0;
>
> out:
>
> > + VIR_FREE(virtPortManagerID);
> > + VIR_FREE(virtPortTypeID);
> > + VIR_FREE(virtPortTypeIDVersion);
> > + VIR_FREE(virtPortInstanceID);
> > + VIR_FREE(virtPortProfileID);
> > + VIR_FREE(virtPortType);
>
> return ret;
Ok.
> > +}
> > +
> > +
> > +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?
True. Next would be to hanlde the **res_ifname in this function, but
that I'd leave to a separate patch.
>
> > 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.
They would not even appear here. Problem is just I cannot even test
anything with macvtap anymore without having a profile which then in
turn requires either a daemon or an enic kernel driver.
>
> > +
> > + 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.
>
If I do that then please send me hardware :-)
I'll post v8 after some testing - while I still can :-)
Stefan
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list