This looks good to me. Acked-by: Kyle Mestery <kmestery@cisco.com> On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:

Both of these functions returned void, but it's convenient for them to return a const char* of the char* that is passed in. This was you can call the function and use the result in the same expression/arg. --- src/util/uuid.c | 6 ++++-- src/util/uuid.h | 6 +++--- src/util/virmacaddr.c | 13 +++++++++++-- src/util/virmacaddr.h | 4 ++-- 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/src/util/uuid.c b/src/util/uuid.c index 1efde69..54e7bb6 100644 --- a/src/util/uuid.c +++ b/src/util/uuid.c @@ -182,9 +182,10 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) { * * Converts the raw UUID into printable format, with embedded '-' * - * Returns 0 in case of success and -1 in case of error. + * Returns a pointer to the resulting character string. */ -void virUUIDFormat(const unsigned char *uuid, char *uuidstr) +const char * +virUUIDFormat(const unsigned char *uuid, char *uuidstr) { snprintf(uuidstr, VIR_UUID_STRING_BUFLEN, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x", @@ -193,6 +194,7 @@ void virUUIDFormat(const unsigned char *uuid, char *uuidstr) uuid[8], uuid[9], uuid[10], uuid[11], uuid[12], uuid[13], uuid[14], uuid[15]); uuidstr[VIR_UUID_STRING_BUFLEN-1] = '\0'; + return uuidstr; }

diff --git a/src/util/uuid.h b/src/util/uuid.h index da56a92..54e0573 100644 --- a/src/util/uuid.h +++ b/src/util/uuid.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007, 2011 Red Hat, Inc. + * Copyright (C) 2007, 2011, 2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -35,7 +35,7 @@ int virUUIDParse(const char *uuidstr, unsigned char *uuid) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;

-void virUUIDFormat(const unsigned char *uuid, - char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +const char *virUUIDFormat(const unsigned char *uuid, + char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

#endif /* __VIR_UUID_H__ */ diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c index c07976b..e207927 100644 --- a/src/util/virmacaddr.c +++ b/src/util/virmacaddr.c @@ -176,14 +176,23 @@ virMacAddrParse(const char* str, virMacAddrPtr addr) return -1; }

-void virMacAddrFormat(const virMacAddrPtr addr, - char *str) +/* virMacAddrFormat + * Converts the binary mac address in addr into a NULL-terminated + * character string in str. It is assumed that the memory pointed to + * by str is at least VIR_MAC_STRING_BUFLEN bytes long. + * + * Returns a pointer to the resulting character string. + */ +const char * +virMacAddrFormat(const virMacAddrPtr addr, + char *str) { snprintf(str, VIR_MAC_STRING_BUFLEN, "%02X:%02X:%02X:%02X:%02X:%02X", addr->addr[0], addr->addr[1], addr->addr[2], addr->addr[3], addr->addr[4], addr->addr[5]); str[VIR_MAC_STRING_BUFLEN-1] = '\0'; + return str; }

void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN], diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h index fdac362..4c5074c 100644 --- a/src/util/virmacaddr.h +++ b/src/util/virmacaddr.h @@ -44,8 +44,8 @@ int virMacAddrCmpRaw(const virMacAddrPtr mac1, void virMacAddrSet(virMacAddrPtr dst, const virMacAddrPtr src); void virMacAddrSetRaw(virMacAddrPtr dst, const unsigned char s[VIR_MAC_BUFLEN]); void virMacAddrGetRaw(virMacAddrPtr src, unsigned char dst[VIR_MAC_BUFLEN]); -void virMacAddrFormat(const virMacAddrPtr addr, - char *str); +const char *virMacAddrFormat(const virMacAddrPtr addr, + char *str); void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN], virMacAddrPtr addr); int virMacAddrParse(const char* str, -- 1.7.11.2

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 08/14/2012 09:04 AM, Laine Stump wrote:

This patch adds three utility functions that operate on virNetDevVPortProfile objects.

* virNetDevVPortProfileCheckComplete() - verifies that all attributes required for the type of the given virtport are specified.

* virNetDevVPortProfileCheckNoExtras() - verifies that there are no attributes specified which are inappropriate for the type of the given virtport.

* virNetDevVPortProfileMerge3() - merges 3 virtports into a single, newly allocated virtport. If any attributes are specified in more than one of the three sources, and do not exactly match, an error is logged and the function fails.

These new functions depend on new fields in the virNetDevVPortProfile object that keep track of whether or not each attribute was specified. Since the higher level parse function doesn't yet set those fields, these functions are not actually usable yet (but that's okay, because they also aren't yet used - all of that functionality comes in a later patch.)

Note that these three functions return 0 on success and -1 on failure. This may seem odd for the first two Check functions, since they could also easily return true/false, but since they actually log an error when the requested condition isn't met (and should result in a failure of the calling function), I thought 0/-1 was more appropriate. --- src/libvirt_private.syms | 3 + src/util/virnetdevvportprofile.c | 313 +++++++++++++++++++++++++++++++++++++++ src/util/virnetdevvportprofile.h | 17 ++- 3 files changed, 332 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c023dbf..89fb1f4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1427,8 +1427,11 @@ virNetDevVethDelete;

# virnetdevvportprofile.h virNetDevVPortProfileAssociate; +virNetDevVPortProfileCheckComplete; +virNetDevVPortProfileCheckNoExtras; virNetDevVPortProfileDisassociate; virNetDevVPortProfileEqual; +virNetDevVPortProfileMerge3; virNetDevVPortProfileOpTypeFromString; virNetDevVPortProfileOpTypeToString;

diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 6db04f7..e686fd9 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -120,6 +120,319 @@ virNetDevVPortProfileEqual(virNetDevVPortProfilePtr a, virNetDevVPortProfilePtr return true; }

+/* virNetDevVPortProfileCheckComplete() checks that all attributes + * required for the type of virtport are specified. When + * generateMissing is true, any missing attribute that can be + * autogenerated, will be (instanceid, interfaceid). If virtport == + * NULL or virtPortType == NONE, then the result is always 0 + * (success). If a required attribute is missing, an error is logged + * and -1 is returned. + */ +int +virNetDevVPortProfileCheckComplete(virNetDevVPortProfilePtr virtport, + bool generateMissing) +{ + const char *missing = NULL; + + if (!virtport || virtport->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE) + return 0; + + switch (virtport->virtPortType) { + case VIR_NETDEV_VPORT_PROFILE_8021QBG: + if (!virtport->managerID_specified) { + missing = "managerid"; + } else if (!virtport->typeID_specified) { + missing = "typeid"; + } else if (!virtport->typeIDVersion_specified) { + missing = "typeidversion"; + } else if (!virtport->instanceID_specified) { + if (generateMissing) { + if (virUUIDGenerate(virtport->instanceID) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot generate a random uuid for instanceid")); + return -1; + } + virtport->instanceID_specified = true; + } else { + missing = "instanceid"; + } + } + break; + + case VIR_NETDEV_VPORT_PROFILE_8021QBH: + if (!virtport->profileID[0]) + missing = "profileid"; + break; + + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: + /* profileid is optional for openvswitch */ + if (!virtport->interfaceID_specified) { + if (generateMissing) { + if (virUUIDGenerate(virtport->interfaceID) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot generate a random uuid for interfaceid")); + return -1; + } + virtport->interfaceID_specified = true; + } else { + missing = "interfaceid"; + } + } + break; + } + + if (missing) { + virReportError(VIR_ERR_XML_ERROR, + _("missing %s in <virtualport type='%s'>"), missing, + virNetDevVPortTypeToString(virtport->virtPortType)); + return -1; + } + + return 0; +} + +/* virNetDevVPortProfileCheckNoExtras() checks that there are no + * attributes specified in this virtport that are inappropriate for + * the type. if virtport == NULL or virtPortType == NONE, then the + * result is always 0 (success). If an extra attribute is present, + * an error is logged and -1 is returned. + */ +int +virNetDevVPortProfileCheckNoExtras(virNetDevVPortProfilePtr virtport) +{ + const char *extra = NULL; + + if (!virtport || virtport->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE) + return 0; + + switch (virtport->virtPortType) { + case VIR_NETDEV_VPORT_PROFILE_8021QBG: + if (virtport->profileID[0]) + extra = "profileid"; + else if (virtport->interfaceID_specified) + extra = "interfaceid"; + break; + + case VIR_NETDEV_VPORT_PROFILE_8021QBH: + if (virtport->managerID_specified) + extra = "managerid"; + else if (virtport->typeID_specified) + extra = "typeid"; + else if (virtport->typeIDVersion_specified) + extra = "typeidversion"; + else if (virtport->instanceID_specified) + extra = "instanceid"; + else if (virtport->interfaceID_specified) + extra = "interfaceid"; + break; + + case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: + if (virtport->managerID_specified) + extra = "managerid"; + else if (virtport->typeID_specified) + extra = "typeid"; + else if (virtport->typeIDVersion_specified) + extra = "typeidversion"; + else if (virtport->instanceID_specified) + extra = "instanceid"; + break; + } + + if (extra) { + virReportError(VIR_ERR_XML_ERROR, + _("extra %s unsupported in <virtualport type='%s'>"), + extra, + virNetDevVPortTypeToString(virtport->virtPortType)); + return -1; + } + + return 0; +} + +/* virNetDevVPortProfileMerge() - merge the attributes in mods into + * orig. If anything that is set in mods has already been set in orig + * *and doesn't match*, log an error and return -1, otherwise return 0. + */ +static int +virNetDevVPortProfileMerge(virNetDevVPortProfilePtr orig, + virNetDevVPortProfilePtr mods) +{ + enum virNetDevVPortProfile otype; + + if (!orig || !mods) + return 0; + + otype = orig->virtPortType; + + if (mods->virtPortType != VIR_NETDEV_VPORT_PROFILE_NONE) { + if (otype != VIR_NETDEV_VPORT_PROFILE_NONE && + otype != mods->virtPortType) { + virReportError(VIR_ERR_XML_ERROR, + _("attempt to merge virtualports " + "with mismatched types (%s and %s)"), + virNetDevVPortTypeToString(otype), + virNetDevVPortTypeToString(mods->virtPortType)); + return -1; + } + otype = orig->virtPortType = mods->virtPortType; + } + + if (mods->managerID_specified && + (otype == VIR_NETDEV_VPORT_PROFILE_8021QBG || + otype == VIR_NETDEV_VPORT_PROFILE_NONE)) { + if (orig->managerID_specified && + (orig->managerID != mods->managerID)) { + virReportError(VIR_ERR_XML_ERROR, + _("attempt to merge virtualports " + "with mismatched managerids (%d and %d)"), + orig->managerID, mods->managerID); + return -1; + } + orig->managerID = mods->managerID; + orig->managerID_specified = true; + } + + if (mods->typeID_specified && + (otype == VIR_NETDEV_VPORT_PROFILE_8021QBG || + otype == VIR_NETDEV_VPORT_PROFILE_NONE)) { + if (orig->typeID_specified && + (orig->typeID != mods->typeID)) { + virReportError(VIR_ERR_XML_ERROR, + _("attempt to merge virtualports " + "with mismatched typeids (%d and %d)"), + orig->typeID, mods->typeID); + return -1; + } + orig->typeID = mods->typeID; + orig->typeID_specified = true; + } + + if (mods->typeIDVersion_specified && + (otype == VIR_NETDEV_VPORT_PROFILE_8021QBG || + otype == VIR_NETDEV_VPORT_PROFILE_NONE)) { + if (orig->typeIDVersion_specified && + (orig->typeIDVersion != mods->typeIDVersion)) { + virReportError(VIR_ERR_XML_ERROR, + _("attempt to merge virtualports with " + "mismatched typeidversions (%d and %d)"), + orig->typeIDVersion, mods->typeIDVersion); + return -1; + } + orig->typeIDVersion = mods->typeIDVersion; + orig->typeIDVersion_specified = true; + } + + if (mods->instanceID_specified && + (otype == VIR_NETDEV_VPORT_PROFILE_8021QBG || + otype == VIR_NETDEV_VPORT_PROFILE_NONE)) { + if (orig->instanceID_specified && + memcmp(orig->instanceID, mods->instanceID, + sizeof(orig->instanceID))) { + char origuuid[VIR_UUID_STRING_BUFLEN]; + char modsuuid[VIR_UUID_STRING_BUFLEN]; + + virReportError(VIR_ERR_XML_ERROR, + _("attempt to merge virtualports with " + "mismatched instanceids ('%s' and '%s')"), + virUUIDFormat(orig->instanceID, origuuid), + virUUIDFormat(mods->instanceID, modsuuid)); + return -1; + } + memcpy(orig->instanceID, mods->instanceID, sizeof(orig->instanceID)); + orig->instanceID_specified = true; + } + + if (mods->interfaceID_specified && + (otype == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH || + otype == VIR_NETDEV_VPORT_PROFILE_NONE)) { + if (orig->interfaceID_specified && + memcmp(orig->interfaceID, mods->interfaceID, + sizeof(orig->interfaceID))) { + char origuuid[VIR_UUID_STRING_BUFLEN]; + char modsuuid[VIR_UUID_STRING_BUFLEN]; + + virReportError(VIR_ERR_XML_ERROR, + _("attempt to merge virtualports with " + "mismatched interfaceids ('%s' and '%s')"), + virUUIDFormat(orig->interfaceID, origuuid), + virUUIDFormat(mods->interfaceID, modsuuid)); + return -1; + } + memcpy(orig->interfaceID, mods->interfaceID, sizeof(orig->interfaceID)); + orig->interfaceID_specified = true; + } + + if (mods->profileID[0] && + (otype == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH || + otype == VIR_NETDEV_VPORT_PROFILE_8021QBH || + otype == VIR_NETDEV_VPORT_PROFILE_NONE)) { + if (orig->profileID[0] && + STRNEQ(orig->profileID, mods->profileID)) { + virReportError(VIR_ERR_XML_ERROR, + _("attempt to merge virtualports with " + "mismatched profileids ('%s' and '%s')"), + orig->profileID, mods->profileID); + return -1; + } + if (virStrcpyStatic(orig->profileID, mods->profileID)) { + /* this should never happen - it indicates mods->profileID + * isn't properly null terminated. */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("corrupted profileid string")); + return -1; + } + } + return 0; +} + +/* virNetDevVPortProfileMerge3() - create a new virNetDevVPortProfile + * that is a combination of the three input profiles. fromInterface is + * highest priority and fromPortgroup is lowest. As lower priority + * objects' attributes are merged in, if the attribute is unset in the + * result object, it is set from the lower priority object, but if it + * is already set in the result and the lower priority object wants to + * change it, that is an error. + */ + +int virNetDevVPortProfileMerge3(virNetDevVPortProfilePtr *result, + virNetDevVPortProfilePtr fromInterface, + virNetDevVPortProfilePtr fromNetwork, + virNetDevVPortProfilePtr fromPortgroup) +{ + int ret = -1; + *result = NULL; + + if ((!fromInterface || (fromInterface->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE)) && + (!fromNetwork || (fromNetwork->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE)) && + (!fromPortgroup || (fromPortgroup->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE))) { + return 0; + } + + /* at least one of the source profiles is non-empty */ + if (VIR_ALLOC(*result) < 0) { I get a compile error here: cc1: warnings being treated as errors ../../src/util/virnetdevvportprofile.c: In function 'virNetDevVPortProfileMerge3': ../../src/util/virnetdevvportprofile.c:412: error: implicit declaration of function 'VIR_ALLOC' [-Wimplicit-function-declaration] ../../src/util/virnetdevvportprofile.c:412: error: nested extern declaration of 'VIR_ALLOC' [-Wnested-externs] + virReportOOMError(); + return ret; + } + + /* start with the interface's profile. There are no pointers in a + * virtualPortProfile, so a shallow copy is sufficient. + */ + if (fromInterface) + **result = *fromInterface; + + if (virNetDevVPortProfileMerge(*result, fromNetwork) < 0) + goto error; + if (virNetDevVPortProfileMerge(*result, fromPortgroup) < 0) + goto error; + + ret = 0; + +error: + if (ret < 0) + VIR_FREE(*result); and here ... ../../src/util/virnetdevvportprofile.c:432: error: implicit declaration of function 'VIR_FREE' [-Wimplicit-function-declaration] ../../src/util/virnetdevvportprofile.c:432: error: nested extern declaration of 'VIR_FREE' [-Wnested-externs] + return ret; +} +

#if WITH_VIRTUALPORT

diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h index 3c16bfe..d23c284 100644 --- a/src/util/virnetdevvportprofile.h +++ b/src/util/virnetdevvportprofile.h @@ -58,18 +58,24 @@ VIR_ENUM_DECL(virNetDevVPortProfileOp) typedef struct _virNetDevVPortProfile virNetDevVPortProfile; typedef virNetDevVPortProfile *virNetDevVPortProfilePtr; struct _virNetDevVPortProfile { - enum virNetDevVPortProfile virtPortType; + int virtPortType; /* enum virNetDevVPortProfile */ /* these members are used when virtPortType == 802.1Qbg */ uint8_t managerID; + bool managerID_specified; uint32_t typeID; /* 24 bit valid */ + bool typeID_specified; uint8_t typeIDVersion; + bool typeIDVersion_specified; unsigned char instanceID[VIR_UUID_BUFLEN]; + bool instanceID_specified;

/* this member is used when virtPortType == 802.1Qbh|openvswitch */ + /* this is a null-terminated character string */ char profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];

/* this member is used when virtPortType == openvswitch */ unsigned char interfaceID[VIR_UUID_BUFLEN]; + bool interfaceID_specified; /* NB - if virtPortType == NONE, any/all of the items could be used */ };

@@ -77,6 +83,15 @@ struct _virNetDevVPortProfile { bool virNetDevVPortProfileEqual(virNetDevVPortProfilePtr a, virNetDevVPortProfilePtr b);

+int virNetDevVPortProfileCheckComplete(virNetDevVPortProfilePtr virtport, + bool generateMissing); +int virNetDevVPortProfileCheckNoExtras(virNetDevVPortProfilePtr virtport); + +int virNetDevVPortProfileMerge3(virNetDevVPortProfilePtr *result, + virNetDevVPortProfilePtr fromInterface, + virNetDevVPortProfilePtr fromNetwork, + virNetDevVPortProfilePtr fromPortgroup); + int virNetDevVPortProfileAssociate(const char *ifname, const virNetDevVPortProfilePtr virtPort, const virMacAddrPtr macaddr,

-- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 08/15/2012 05:32 PM, Laine Stump wrote:

On 08/15/2012 10:44 AM, Viktor Mihajlovski wrote:

...

On 08/14/2012 09:04 AM, Laine Stump wrote:

...

+ /* at least one of the source profiles is non-empty */ + if (VIR_ALLOC(*result) < 0) {

I get a compile error here: cc1: warnings being treated as errors ../../src/util/virnetdevvportprofile.c: In function 'virNetDevVPortProfileMerge3': ../../src/util/virnetdevvportprofile.c:412: error: implicit declaration of function 'VIR_ALLOC' [-Wimplicit-function-declaration] ../../src/util/virnetdevvportprofile.c:412: error: nested extern declaration of 'VIR_ALLOC' [-Wnested-externs]

Ah - you must be configured --vithout-virtualport (do you have a very old kernel?) Can you try this patch to see if it solves your problem? If so I'll push it as a build breaker:

diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index e686fd9..a789a34 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -24,6 +24,7 @@

#include "virnetdevvportprofile.h" #include "virterror_internal.h" +# include "memory.h"

#define VIR_FROM_THIS VIR_FROM_NET

@@ -52,7 +53,6 @@ VIR_ENUM_IMPL(virNetDevVPortProfileOp, VIR_NETDEV_VPORT_PROFILE_OP_LAST,

# include "virnetlink.h" # include "virfile.h" -# include "memory.h" # include "logging.h" # include "virnetdev.h"

right ... this was on Ubuntu 10.04 (2.6.32). With the suggested change I can get over this issue. Still stuck with the problem below though: https://www.redhat.com/archives/libvir-list/2012-August/msg01064.html Well, that's what you get with a prehistoric environments :-) -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

This looks good to me. Acked-by: Kyle Mestery <kmestery@cisco.com> On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:

This function has several calls to increase the buffer indent by 6, then decrease it again, then increase, then decrease. Additionally, there were several printfs that had 6 spaces at the beginning of the line.

virDomainActualNetDefFormat, which is called by virDomainNetDefFormat, had similar ugliness.

This patch changes both functions to just increase the indent at the beginning, decrease it at (well, just before*) the end, and remove all of the occurences of 6/8 spaces at the beginning of lines.

*The indent had to be reset before the end of the function because virDomainDeviceInfoFormat assumes a 0 indent and is called from many other places, and I didn't want to do an overhaul of every caller of that function. A separate patch to switch all of domain_conf.c would be a useful exercise, but my current goal is unrelated to that, so I'll leave it for another day. --- src/conf/domain_conf.c | 73 +++++++++++++++++++++++--------------------------- 1 file changed, 33 insertions(+), 40 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5990634..8f1f244 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11560,21 +11560,22 @@ virDomainActualNetDefFormat(virBufferPtr buf, return -1; }

- virBufferAsprintf(buf, " <actual type='%s'", type); + virBufferAsprintf(buf, "<actual type='%s'", type); if (def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && def->data.hostdev.def.managed) { virBufferAddLit(buf, " managed='yes'"); } virBufferAddLit(buf, ">\n");

+ virBufferAdjustIndent(buf, 2); switch (def->type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: - virBufferEscapeString(buf, " <source bridge='%s'/>\n", + virBufferEscapeString(buf, "<source bridge='%s'/>\n", def->data.bridge.brname); break;

case VIR_DOMAIN_NET_TYPE_DIRECT: - virBufferAddLit(buf, " <source"); + virBufferAddLit(buf, "<source"); if (def->data.direct.linkdev) virBufferEscapeString(buf, " dev='%s'", def->data.direct.linkdev); @@ -11590,12 +11591,10 @@ virDomainActualNetDefFormat(virBufferPtr buf, break;

case VIR_DOMAIN_NET_TYPE_HOSTDEV: - virBufferAdjustIndent(buf, 8); if (virDomainHostdevSourceFormat(buf, &def->data.hostdev.def, flags, true) < 0) { return -1; } - virBufferAdjustIndent(buf, -8); break;

case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -11606,14 +11605,13 @@ virDomainActualNetDefFormat(virBufferPtr buf, return -1; }

- virBufferAdjustIndent(buf, 8); if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) return -1; if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) return -1; - virBufferAdjustIndent(buf, -8);

- virBufferAddLit(buf, " </actual>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</actual>\n"); return 0; }

@@ -11637,14 +11635,15 @@ virDomainNetDefFormat(virBufferPtr buf, } virBufferAddLit(buf, ">\n");

+ virBufferAdjustIndent(buf, 6); virBufferAsprintf(buf, - " <mac address='%02x:%02x:%02x:%02x:%02x:%02x'/>\n", + "<mac address='%02x:%02x:%02x:%02x:%02x:%02x'/>\n", def->mac.addr[0], def->mac.addr[1], def->mac.addr[2], def->mac.addr[3], def->mac.addr[4], def->mac.addr[5]);

switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: - virBufferEscapeString(buf, " <source network='%s'", + virBufferEscapeString(buf, "<source network='%s'", def->data.network.name); virBufferEscapeString(buf, " portgroup='%s'", def->data.network.portgroup); @@ -11655,39 +11654,41 @@ virDomainNetDefFormat(virBufferPtr buf, break;

case VIR_DOMAIN_NET_TYPE_ETHERNET: - virBufferEscapeString(buf, " <source dev='%s'/>\n", + virBufferEscapeString(buf, "<source dev='%s'/>\n", def->data.ethernet.dev); if (def->data.ethernet.ipaddr) - virBufferAsprintf(buf, " <ip address='%s'/>\n", + virBufferAsprintf(buf, "<ip address='%s'/>\n", def->data.ethernet.ipaddr); break;

case VIR_DOMAIN_NET_TYPE_BRIDGE: - virBufferEscapeString(buf, " <source bridge='%s'/>\n", + virBufferEscapeString(buf, "<source bridge='%s'/>\n", def->data.bridge.brname); - if (def->data.bridge.ipaddr) - virBufferAsprintf(buf, " <ip address='%s'/>\n", + if (def->data.bridge.ipaddr) { + virBufferAsprintf(buf, "<ip address='%s'/>\n", def->data.bridge.ipaddr); + } break;

case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: - if (def->data.socket.address) - virBufferAsprintf(buf, " <source address='%s' port='%d'/>\n", + if (def->data.socket.address) { + virBufferAsprintf(buf, "<source address='%s' port='%d'/>\n", def->data.socket.address, def->data.socket.port); - else - virBufferAsprintf(buf, " <source port='%d'/>\n", + } else { + virBufferAsprintf(buf, "<source port='%d'/>\n", def->data.socket.port); + } break;

case VIR_DOMAIN_NET_TYPE_INTERNAL: - virBufferEscapeString(buf, " <source name='%s'/>\n", + virBufferEscapeString(buf, "<source name='%s'/>\n", def->data.internal.name); break;

case VIR_DOMAIN_NET_TYPE_DIRECT: - virBufferEscapeString(buf, " <source dev='%s'", + virBufferEscapeString(buf, "<source dev='%s'", def->data.direct.linkdev); virBufferAsprintf(buf, " mode='%s'", virNetDevMacVLanModeTypeToString(def->data.direct.mode)); @@ -11695,12 +11696,10 @@ virDomainNetDefFormat(virBufferPtr buf, break;

case VIR_DOMAIN_NET_TYPE_HOSTDEV: - virBufferAdjustIndent(buf, 6); if (virDomainHostdevSourceFormat(buf, &def->data.hostdev.def, flags, true) < 0) { return -1; } - virBufferAdjustIndent(buf, -6); break;

case VIR_DOMAIN_NET_TYPE_USER: @@ -11708,26 +11707,22 @@ virDomainNetDefFormat(virBufferPtr buf, break; }

- virBufferAdjustIndent(buf, 6); if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) return -1; - virBufferAdjustIndent(buf, -6); - - virBufferEscapeString(buf, " <script path='%s'/>\n", + virBufferEscapeString(buf, "<script path='%s'/>\n", def->script); if (def->ifname && !((flags & VIR_DOMAIN_XML_INACTIVE) && (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) { /* Skip auto-generated target names for inactive config. */ - virBufferEscapeString(buf, " <target dev='%s'/>\n", - def->ifname); + virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname); } if (def->model) { - virBufferEscapeString(buf, " <model type='%s'/>\n", + virBufferEscapeString(buf, "<model type='%s'/>\n", def->model); if (STREQ(def->model, "virtio") && (def->driver.virtio.name || def->driver.virtio.txmode)) { - virBufferAddLit(buf, " <driver"); + virBufferAddLit(buf, "<driver"); if (def->driver.virtio.name) { virBufferAsprintf(buf, " name='%s'", virDomainNetBackendTypeToString(def->driver.virtio.name)); @@ -11748,27 +11743,25 @@ virDomainNetDefFormat(virBufferPtr buf, } } if (def->filter) { - virBufferAdjustIndent(buf, 6); if (virNWFilterFormatParamAttributes(buf, def->filterparams, def->filter) < 0) return -1; - virBufferAdjustIndent(buf, -6); }

if (def->tune.sndbuf_specified) { - virBufferAddLit(buf, " <tune>\n"); - virBufferAsprintf(buf, " <sndbuf>%lu</sndbuf>\n", - def->tune.sndbuf); - virBufferAddLit(buf, " </tune>\n"); + virBufferAddLit(buf, "<tune>\n"); + virBufferAsprintf(buf, " <sndbuf>%lu</sndbuf>\n", def->tune.sndbuf); + virBufferAddLit(buf, "</tune>\n"); }

- if (def->linkstate) - virBufferAsprintf(buf, " <link state='%s'/>\n", + if (def->linkstate) { + virBufferAsprintf(buf, "<link state='%s'/>\n", virDomainNetInterfaceLinkStateTypeToString(def->linkstate)); + }

- virBufferAdjustIndent(buf, 6); if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) return -1; + virBufferAdjustIndent(buf, -6);

if (virDomainDeviceInfoFormat(buf, &def->info, -- 1.7.11.2

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

This looks good to me. Acked-by: Kyle Mestery <kmestery@cisco.com> On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:

Until now, all attributes in a <virtualport> parameter list that were acceptable for a particular type, were also required. There were no optional attributes.

One of the aims of supporting <virtualport> in libvirt's virtual networks and portgroups is to allow specifying the group-wide parameters in the network's virtualport, and merge that with the interface's virtualport, which will have the instance-specific info (i.e. the interfaceid or instanceid).

Additionally, the guest's interface XML shouldn't need to know what type of network connection will be used prior to runtime - it could be openvswitch, 802.1Qbh, 802.1Qbg, or none of the above - but should still be able to specify instance-specific info just in case it turns out to be applicable.

Finally, up to now, the parser for virtualport has always generated a random instanceid/interfaceid when appropriate, making it impossible to leave it blank (which is what's required for virtualports within a network/portprofile definition).

This patch modifies the parser and formatter of the <virtualport> element in the following ways:

* because most of the attributes in a virNetDevVPortProfile are fixed size binary data with no reserved values, there is no way to embed a "this value wasn't specified" sentinel into the existing data. To solve this problem, the new *_specified fields in the virNetDevVPortProfile object that were added in a previous patch of this series are now set when the corresponding attribute is present during the parse.

* allow parsing/formatting a <virtualport> that has no type set. In this case, all fields are settable, but all are also optional.

* add a GENERATE_MISSING_DEFAULTS flag to the parser - if this flag is set and an instanceid/interfaceid is expected but not provided, a random one will be generated. This was previously the default behavior, but is now done only for virtualports inside an <interface> definition, not for those in <network> or <portgroup>.

* add a REQUIRE_ALL_ATTRIBUTES flag to the parser - if this flag is set the parser will call the new virNetDevVPortProfileCheckComplete() functions at the end of the parser to check for any missing attributes (based on type), and return failure if anything is missing. This used to be default behavior. Now it is only used for the virtualport defined inside an interface's <actual> element (by the time you've figured out the contents of <actual>, you should have all the necessary data to fill in the entire virtualport)

* add a REQUIRE_TYPE flag to the parser - if this flag is set, the parser will return an error if the virtualport has no type attribute. This also was previously the default behavior, but isn't needed in the case of the virtualport for a type='network' interface (i.e. the exact type isn't yet known), or the virtualport of a portgroup (i.e. the portgroup just has modifiers for the network's virtualport, which *does* require a type) - in those cases, the check will be done at domain startup, once the final virtualport is assembled (this is handled in the next patch). --- docs/schemas/networkcommon.rng | 114 ++++++-- src/conf/domain_conf.c | 25 +- src/conf/netdev_vport_profile_conf.c | 308 +++++++++++---------- src/conf/netdev_vport_profile_conf.h | 17 +- src/conf/network_conf.c | 7 +- tests/networkxml2xmlin/openvswitch-net.xml | 16 ++ tests/networkxml2xmlin/vepa-net.xml | 6 +- tests/networkxml2xmlout/openvswitch-net.xml | 16 ++ tests/networkxml2xmlout/vepa-net.xml | 6 +- tests/networkxml2xmltest.c | 1 + .../qemuxml2argv-net-virtio-network-portgroup.xml | 14 + 11 files changed, 344 insertions(+), 186 deletions(-) create mode 100644 tests/networkxml2xmlin/openvswitch-net.xml create mode 100644 tests/networkxml2xmlout/openvswitch-net.xml

diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index 2328892..f2c3330 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -15,22 +15,30 @@ <attribute name="type"> <value>802.1Qbg</value> </attribute> - <element name="parameters"> - <attribute name="managerid"> - <ref name="uint8range"/> - </attribute> - <attribute name="typeid"> - <ref name="uint24range"/> - </attribute> - <attribute name="typeidversion"> - <ref name="uint8range"/> - </attribute> - <optional> - <attribute name="instanceid"> - <ref name="UUID"/> - </attribute> - </optional> - </element> + <optional> + <element name="parameters"> + <optional> + <attribute name="managerid"> + <ref name="uint8range"/> + </attribute> + </optional> + <optional> + <attribute name="typeid"> + <ref name="uint24range"/> + </attribute> + </optional> + <optional> + <attribute name="typeidversion"> + <ref name="uint8range"/> + </attribute> + </optional> + <optional> + <attribute name="instanceid"> + <ref name="UUID"/> + </attribute> + </optional> + </element> + </optional> </element> </group> <group> @@ -38,11 +46,75 @@ <attribute name="type"> <value>802.1Qbh</value> </attribute> - <element name="parameters"> - <attribute name="profileid"> - <ref name="virtualPortProfileID"/> - </attribute> - </element> + <optional> + <element name="parameters"> + <optional> + <attribute name="profileid"> + <ref name="virtualPortProfileID"/> + </attribute> + </optional> + </element> + </optional> + </element> + </group> + <group> + <element name="virtualport"> + <attribute name="type"> + <value>openvswitch</value> + </attribute> + <optional> + <element name="parameters"> + <optional> + <attribute name="profileid"> + <ref name="virtualPortProfileID"/> + </attribute> + </optional> + <optional> + <attribute name="interfaceid"> + <ref name="UUID"/> + </attribute> + </optional> + </element> + </optional> + </element> + </group> + <group> + <!-- use this when no type attribute is present --> + <element name="virtualport"> + <optional> + <element name="parameters"> + <optional> + <attribute name="managerid"> + <ref name="uint8range"/> + </attribute> + </optional> + <optional> + <attribute name="typeid"> + <ref name="uint24range"/> + </attribute> + </optional> + <optional> + <attribute name="typeidversion"> + <ref name="uint8range"/> + </attribute> + </optional> + <optional> + <attribute name="instanceid"> + <ref name="UUID"/> + </attribute> + </optional> + <optional> + <attribute name="profileid"> + <ref name="virtualPortProfileID"/> + </attribute> + </optional> + <optional> + <attribute name="interfaceid"> + <ref name="UUID"/> + </attribute> + </optional> + </element> + </optional> </element> </group> </choice> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8f1f244..0cda374 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4404,8 +4404,13 @@ virDomainActualNetDefParseXML(xmlNodePtr node, if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE || actual->type == VIR_DOMAIN_NET_TYPE_DIRECT || actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* the virtualport in <actual> should always already + * have an instanceid/interfaceid if its required, + * so don't let the parser generate one */ if (!(actual->virtPortProfile - = virNetDevVPortProfileParse(virtPortNode))) { + = virNetDevVPortProfileParse(virtPortNode, + VIR_VPORT_XML_REQUIRE_ALL_ATTRIBUTES | + VIR_VPORT_XML_REQUIRE_TYPE))) { goto error; } } else { @@ -4558,12 +4563,20 @@ virDomainNetDefParseXML(virCapsPtr caps, mode = virXMLPropString(cur, "mode"); } else if (!def->virtPortProfile && xmlStrEqual(cur->name, BAD_CAST "virtualport")) { - if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK || - def->type == VIR_DOMAIN_NET_TYPE_BRIDGE || - def->type == VIR_DOMAIN_NET_TYPE_DIRECT || - def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!(def->virtPortProfile - = virNetDevVPortProfileParse(cur))) { + = virNetDevVPortProfileParse(cur, + VIR_VPORT_XML_GENERATE_MISSING_DEFAULTS))) { + goto error; + } + } else if (def->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + def->type == VIR_DOMAIN_NET_TYPE_DIRECT || + def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + if (!(def->virtPortProfile + = virNetDevVPortProfileParse(cur, + VIR_VPORT_XML_GENERATE_MISSING_DEFAULTS| + VIR_VPORT_XML_REQUIRE_ALL_ATTRIBUTES| + VIR_VPORT_XML_REQUIRE_TYPE))) { goto error; } } else { diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c index 31ee9b4..1adff10 100644 --- a/src/conf/netdev_vport_profile_conf.c +++ b/src/conf/netdev_vport_profile_conf.c @@ -37,7 +37,7 @@ VIR_ENUM_IMPL(virNetDevVPort, VIR_NETDEV_VPORT_PROFILE_LAST,

virNetDevVPortProfilePtr -virNetDevVPortProfileParse(xmlNodePtr node) +virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags) { char *virtPortType; char *virtPortManagerID = NULL; @@ -54,22 +54,22 @@ virNetDevVPortProfileParse(xmlNodePtr node) return NULL; }

- virtPortType = virXMLPropString(node, "type"); - if (!virtPortType) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing virtualportprofile type")); + if ((virtPortType = virXMLPropString(node, "type")) && + (virtPort->virtPortType = virNetDevVPortTypeFromString(virtPortType)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown virtualport type %s"), virtPortType); goto error; }

- if ((virtPort->virtPortType = virNetDevVPortTypeFromString(virtPortType)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown virtualportprofile type %s"), virtPortType); + if ((virtPort->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE) && + (flags & VIR_VPORT_XML_REQUIRE_TYPE)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing required virtualport type")); goto error; }

while (cur != NULL) { if (xmlStrEqual(cur->name, BAD_CAST "parameters")) { - virtPortManagerID = virXMLPropString(cur, "managerid"); virtPortTypeID = virXMLPropString(cur, "typeid"); virtPortTypeIDVersion = virXMLPropString(cur, "typeidversion"); @@ -78,132 +78,119 @@ virNetDevVPortProfileParse(xmlNodePtr node) virtPortInterfaceID = virXMLPropString(cur, "interfaceid"); break; } - cur = cur->next; }

- switch (virtPort->virtPortType) { - case VIR_NETDEV_VPORT_PROFILE_8021QBG: - if (virtPortManagerID != NULL && virtPortTypeID != NULL && - virtPortTypeIDVersion != NULL) { - unsigned int val; - - if (virStrToLong_ui(virtPortManagerID, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of managerid parameter")); - goto error; - } - - if (val > 0xff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value of managerid out of range")); - goto error; - } - - virtPort->managerID = (uint8_t)val; - - if (virStrToLong_ui(virtPortTypeID, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of typeid parameter")); - goto error; - } + if (virtPortManagerID) { + unsigned int val;

- if (val > 0xffffff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value for typeid out of range")); - goto error; - } - - virtPort->typeID = (uint32_t)val; + if (virStrToLong_ui(virtPortManagerID, NULL, 0, &val)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot parse value of managerid parameter")); + goto error; + } + if (val > 0xff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value of managerid out of range")); + goto error; + } + virtPort->managerID = (uint8_t)val; + virtPort->managerID_specified = true; + }

- if (virStrToLong_ui(virtPortTypeIDVersion, NULL, 0, &val)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse value of typeidversion parameter")); - goto error; - } + if (virtPortTypeID) { + unsigned int val;

- if (val > 0xff) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("value of typeidversion out of range")); - goto error; - } + if (virStrToLong_ui(virtPortTypeID, NULL, 0, &val)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot parse value of typeid parameter")); + goto error; + } + if (val > 0xffffff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value for typeid out of range")); + goto error; + } + virtPort->typeID = (uint32_t)val; + virtPort->typeID_specified = true; + }

- virtPort->typeIDVersion = (uint8_t)val; - - if (virtPortInstanceID != NULL) { - if (virUUIDParse(virtPortInstanceID, - virtPort->instanceID)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse instanceid parameter as a uuid")); - goto error; - } - } else { - if (virUUIDGenerate(virtPort->instanceID)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot generate a random uuid for instanceid")); - goto error; - } - } + if (virtPortTypeIDVersion) { + unsigned int val;

- virtPort->virtPortType = VIR_NETDEV_VPORT_PROFILE_8021QBG; + if (virStrToLong_ui(virtPortTypeIDVersion, NULL, 0, &val)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot parse value of typeidversion parameter")); + goto error; + } + if (val > 0xff) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("value of typeidversion out of range")); + goto error; + } + virtPort->typeIDVersion = (uint8_t)val; + virtPort->typeIDVersion_specified = true; + }

- } else { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("a parameter is missing for 802.1Qbg description")); + if (virtPortInstanceID) { + if (virUUIDParse(virtPortInstanceID, virtPort->instanceID) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot parse instanceid parameter as a uuid")); goto error; } - break; - - case VIR_NETDEV_VPORT_PROFILE_8021QBH: - if (virtPortProfileID != NULL) { - if (virStrcpyStatic(virtPort->profileID, - virtPortProfileID) != NULL) { - virtPort->virtPortType = VIR_NETDEV_VPORT_PROFILE_8021QBH; - } else { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("profileid parameter too long")); - goto error; - } - } else { + virtPort->instanceID_specified = true; + } + + if (virtPortProfileID && + !virStrcpyStatic(virtPort->profileID, virtPortProfileID)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("profileid parameter too long")); + goto error; + } + + if (virtPortInterfaceID) { + if (virUUIDParse(virtPortInterfaceID, virtPort->interfaceID) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("profileid parameter is missing for 802.1Qbh description")); + _("cannot parse interfaceid parameter as a uuid")); goto error; } - break; - case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: - if (virtPortInterfaceID != NULL) { - if (virUUIDParse(virtPortInterfaceID, - virtPort->interfaceID)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot parse interfaceid parameter as a uuid")); - goto error; - } - } else { - if (virUUIDGenerate(virtPort->interfaceID)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot generate a random uuid for interfaceid")); + virtPort->interfaceID_specified = true; + } + + /* generate default instanceID/interfaceID if appropriate */ + if (flags & VIR_VPORT_XML_GENERATE_MISSING_DEFAULTS) { + if (!virtPort->instanceID_specified && + (virtPort->virtPortType == VIR_NETDEV_VPORT_PROFILE_8021QBG || + virtPort->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE)) { + if (virUUIDGenerate(virtPort->instanceID) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot generate a random uuid for instanceid")); goto error; } + virtPort->instanceID_specified = true; } - /* profileid is not mandatory for Open vSwitch */ - if (virtPortProfileID != NULL) { - if (virStrcpyStatic(virtPort->profileID, - virtPortProfileID) == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("profileid parameter too long")); + if (!virtPort->interfaceID_specified && + (virtPort->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH || + virtPort->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE)) { + if (virUUIDGenerate(virtPort->interfaceID) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot generate a random uuid for interfaceid")); goto error; } - } else { - virtPort->profileID[0] = '\0'; + virtPort->interfaceID_specified = true; } - break; + }

- default: - virReportError(VIR_ERR_XML_ERROR, - _("unexpected virtualport type %d"), virtPort->virtPortType); + /* check for required/unsupported attributes */ + + if ((flags & VIR_VPORT_XML_REQUIRE_ALL_ATTRIBUTES) && + (virNetDevVPortProfileCheckComplete(virtPort, false) < 0)) { goto error; }

+ if (virNetDevVPortProfileCheckNoExtras(virtPort) < 0) + goto error; + cleanup: VIR_FREE(virtPortManagerID); VIR_FREE(virtPortTypeID); @@ -224,53 +211,76 @@ int virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort, virBufferPtr buf) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; + enum virNetDevVPortProfile type; + bool noParameters;

- if (!virtPort || virtPort->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE) + if (!virtPort) return 0;

- virBufferAsprintf(buf, "<virtualport type='%s'>\n", - virNetDevVPortTypeToString(virtPort->virtPortType)); - - switch (virtPort->virtPortType) { - case VIR_NETDEV_VPORT_PROFILE_8021QBG: - virUUIDFormat(virtPort->instanceID, - uuidstr); - virBufferAsprintf(buf, - " <parameters managerid='%d' typeid='%d' " - "typeidversion='%d' instanceid='%s'/>\n", - virtPort->managerID, - virtPort->typeID, - virtPort->typeIDVersion, - uuidstr); - break; - - case VIR_NETDEV_VPORT_PROFILE_8021QBH: - virBufferAsprintf(buf, - " <parameters profileid='%s'/>\n", - virtPort->profileID); - break; - - case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH: - virUUIDFormat(virtPort->interfaceID, - uuidstr); - if (virtPort->profileID[0] == '\0') { - virBufferAsprintf(buf, " <parameters interfaceid='%s'/>\n", - uuidstr); + noParameters = !(virtPort->managerID_specified || + virtPort->typeID_specified || + virtPort->typeIDVersion_specified || + virtPort->instanceID_specified || + virtPort->profileID[0] || + virtPort->interfaceID_specified); + + type = virtPort->virtPortType; + if (type == VIR_NETDEV_VPORT_PROFILE_NONE) { + if (noParameters) + return 0; + virBufferAddLit(buf, "<virtualport>\n"); + } else { + if (noParameters) { + virBufferAsprintf(buf, "<virtualport type='%s'/>\n", + virNetDevVPortTypeToString(type)); + return 0; } else { - virBufferAsprintf(buf, " <parameters interfaceid='%s' " - "profileid='%s'/>\n", uuidstr, - virtPort->profileID); + virBufferAsprintf(buf, "<virtualport type='%s'>\n", + virNetDevVPortTypeToString(type)); } + } + virBufferAddLit(buf, " <parameters");

- break; + if (virtPort->managerID_specified && + (type == VIR_NETDEV_VPORT_PROFILE_8021QBG || + type == VIR_NETDEV_VPORT_PROFILE_NONE)) { + virBufferAsprintf(buf, " managerid='%d'", virtPort->managerID); + } + if (virtPort->typeID_specified && + (type == VIR_NETDEV_VPORT_PROFILE_8021QBG || + type == VIR_NETDEV_VPORT_PROFILE_NONE)) { + virBufferAsprintf(buf, " typeid='%d'", virtPort->typeID); + } + if (virtPort->typeIDVersion_specified && + (type == VIR_NETDEV_VPORT_PROFILE_8021QBG || + type == VIR_NETDEV_VPORT_PROFILE_NONE)) { + virBufferAsprintf(buf, " typeidversion='%d'", + virtPort->typeIDVersion); + } + if (virtPort->instanceID_specified && + (type == VIR_NETDEV_VPORT_PROFILE_8021QBG || + type == VIR_NETDEV_VPORT_PROFILE_NONE)) { + char uuidstr[VIR_UUID_STRING_BUFLEN];

- default: - virReportError(VIR_ERR_XML_ERROR, - _("unexpected virtualport type %d"), virtPort->virtPortType); - return -1; + virUUIDFormat(virtPort->instanceID, uuidstr); + virBufferAsprintf(buf, " instanceid='%s'", uuidstr); + } + if (virtPort->interfaceID_specified && + (type == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH || + type == VIR_NETDEV_VPORT_PROFILE_NONE)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(virtPort->interfaceID, uuidstr); + virBufferAsprintf(buf, " interfaceid='%s'", uuidstr); + } + if (virtPort->profileID[0] && + (type == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH || + type == VIR_NETDEV_VPORT_PROFILE_8021QBH || + type == VIR_NETDEV_VPORT_PROFILE_NONE)) { + virBufferAsprintf(buf, " profileid='%s'", virtPort->profileID); }

+ virBufferAddLit(buf, "/>\n"); virBufferAddLit(buf, "</virtualport>\n"); return 0; } diff --git a/src/conf/netdev_vport_profile_conf.h b/src/conf/netdev_vport_profile_conf.h index 367bdf3..caf3519 100644 --- a/src/conf/netdev_vport_profile_conf.h +++ b/src/conf/netdev_vport_profile_conf.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -28,8 +28,21 @@ # include "buf.h" # include "xml.h"

+typedef enum { + /* generate random defaults for interfaceID/interfaceID + * when appropriate + */ + VIR_VPORT_XML_GENERATE_MISSING_DEFAULTS = (1<<0), + /* fail if any attribute required for the specified + * type is missing + */ + VIR_VPORT_XML_REQUIRE_ALL_ATTRIBUTES = (1<<1), + /* fail if no type is specified */ + VIR_VPORT_XML_REQUIRE_TYPE = (1<<2), +} virNetDevVPortXMLFlags; + virNetDevVPortProfilePtr -virNetDevVPortProfileParse(xmlNodePtr node); +virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags);

int virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort, diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a3714d9..783c388 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -899,8 +899,9 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,

virtPortNode = virXPathNode("./virtualport", ctxt); if (virtPortNode && - (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode)))) + (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode, 0)))) { goto error; + }

bandwidth_node = virXPathNode("./bandwidth", ctxt); if (bandwidth_node && @@ -1010,8 +1011,10 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)

virtPortNode = virXPathNode("./virtualport", ctxt); if (virtPortNode && - (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode)))) + (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode, + VIR_VPORT_XML_REQUIRE_TYPE)))) { goto error; + }

nPortGroups = virXPathNodeSet("./portgroup", ctxt, &portGroupNodes); if (nPortGroups < 0) diff --git a/tests/networkxml2xmlin/openvswitch-net.xml b/tests/networkxml2xmlin/openvswitch-net.xml new file mode 100644 index 0000000..8aa1897 --- /dev/null +++ b/tests/networkxml2xmlin/openvswitch-net.xml @@ -0,0 +1,16 @@ +<network> + <name>openvswitch-net</name> + <uuid>81ff0d90-c92e-6742-64da-4a736edb9a8b</uuid> + <forward mode='bridge'/> + <virtualport type='openvswitch'/> + <portgroup name='bob' default='yes'> + <virtualport> + <parameters profileid='bob-profile'/> + </virtualport> + </portgroup> + <portgroup name='alice'> + <virtualport> + <parameters profileid='alice-profile'/> + </virtualport> + </portgroup> +</network> diff --git a/tests/networkxml2xmlin/vepa-net.xml b/tests/networkxml2xmlin/vepa-net.xml index b1a40c6..030c1d1 100644 --- a/tests/networkxml2xmlin/vepa-net.xml +++ b/tests/networkxml2xmlin/vepa-net.xml @@ -7,16 +7,16 @@ <interface dev="eth3"/> </forward> <virtualport type="802.1Qbg"> - <parameters managerid="11" typeid="1193047" typeidversion="2" instanceid="b153fa89-1b87-9719-ec12-99e0054fb844"/> + <parameters managerid="11" typeid="1193047" typeidversion="2"/> </virtualport> <portgroup name="bob" default="yes"> <virtualport type="802.1Qbg"> - <parameters managerid="12" typeid="2193047" typeidversion="3" instanceid="5d00e0ba-e15c-959c-fbb6-b595b0655735"/> + <parameters typeid="2193047" typeidversion="3"/> </virtualport> </portgroup> <portgroup name="alice"> <virtualport type="802.1Qbg"> - <parameters managerid="13" typeid="3193047" typeidversion="4" instanceid="70bf45f9-01a8-f5ee-3c0f-e25a0a2e44a6"/> + <parameters managerid="13"/> </virtualport> </portgroup> </network> diff --git a/tests/networkxml2xmlout/openvswitch-net.xml b/tests/networkxml2xmlout/openvswitch-net.xml new file mode 100644 index 0000000..8aa1897 --- /dev/null +++ b/tests/networkxml2xmlout/openvswitch-net.xml @@ -0,0 +1,16 @@ +<network> + <name>openvswitch-net</name> + <uuid>81ff0d90-c92e-6742-64da-4a736edb9a8b</uuid> + <forward mode='bridge'/> + <virtualport type='openvswitch'/> + <portgroup name='bob' default='yes'> + <virtualport> + <parameters profileid='bob-profile'/> + </virtualport> + </portgroup> + <portgroup name='alice'> + <virtualport> + <parameters profileid='alice-profile'/> + </virtualport> + </portgroup> +</network> diff --git a/tests/networkxml2xmlout/vepa-net.xml b/tests/networkxml2xmlout/vepa-net.xml index af13d0f..4d35a8a 100644 --- a/tests/networkxml2xmlout/vepa-net.xml +++ b/tests/networkxml2xmlout/vepa-net.xml @@ -7,16 +7,16 @@ <interface dev='eth3'/> </forward> <virtualport type='802.1Qbg'> - <parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='b153fa89-1b87-9719-ec12-99e0054fb844'/> + <parameters managerid='11' typeid='1193047' typeidversion='2'/> </virtualport> <portgroup name='bob' default='yes'> <virtualport type='802.1Qbg'> - <parameters managerid='12' typeid='2193047' typeidversion='3' instanceid='5d00e0ba-e15c-959c-fbb6-b595b0655735'/> + <parameters typeid='2193047' typeidversion='3'/> </virtualport> </portgroup> <portgroup name='alice'> <virtualport type='802.1Qbg'> - <parameters managerid='13' typeid='3193047' typeidversion='4' instanceid='70bf45f9-01a8-f5ee-3c0f-e25a0a2e44a6'/> + <parameters managerid='13'/> </virtualport> </portgroup> </network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 8641c41..5a5531a 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -104,6 +104,7 @@ mymain(void) DO_TEST("host-bridge-net"); DO_TEST("vepa-net"); DO_TEST("bandwidth-network"); + DO_TEST("openvswitch-net"); DO_TEST_FULL("passthrough-pf", VIR_NETWORK_XML_INACTIVE);

return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml index 3eb5c88..6d379a0 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml @@ -29,6 +29,20 @@ </virtualport> <model type='virtio'/> </interface> + <interface type='network'> + <mac address='10:11:22:33:44:55'/> + <source network='blue' portgroup='sam'/> + <virtualport> + <parameters instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f' interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> + </virtualport> + </interface> + <interface type='network'> + <mac address='22:11:22:33:44:55'/> + <source network='blue' portgroup='sam'/> + <virtualport type='802.1Qbh'> + <parameters profileid='testhis99'/> + </virtualport> + </interface> <memballoon model='virtio'/> </devices> </domain> -- 1.7.11.2

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

This looks good to me, with some minor nits below. Acked-by: Kyle Mestery <kmestery@cisco.com> On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:

One of the original ideas behind allowing a <virtualport> in an interface definition as well as in the <network> definition *and*one or more <portgroup>s within the network, was that guest-specific parameteres (like instanceid and interfaceid) could be given in the interface's virtualport, and more general things (portid, managerid, etc) could be given in the network and/or portgroup, with all the bits brought together at guest startup time and combined into a single virtualport to be used by the guest. This was somehow overlooked in the implementation, though - it simply picks the "most specific" virtualport, and uses the entire thing, with no attempt to merge in details from the others.

This patch uses virNetDevVPortProfileMerge3() to combine the three possible virtualports into one, then uses virNetDevVPortProfileCheck*() to verify that the resulting virtualport type is appropriate for the type of network, and that all the required attributes for that type are present.

An example of usage is this: assuming a <network> definitions on host ABC of:

<network> <name>testA</name> ... <virtualport type='openvswitch'/> ... <portgroup name='engineering'> <virtualport> <parameters profileid='eng'/> </virtualport> </portgroup> <portgroup name='sales'> <virtualport> <parameters profileid='sales'/> </virtualport> </portgroup> </network>

and the same <network> on host DEF of:

<network> <name>testA</name> ... <virtualport type='802.1Qbg'> <parameters typeid="1193047" typeidversion="2"/> </virtualport> ... <portgroup name='engineering'> <virtualport> <parameters managerid="11"/> </virtualport> </portgroup> <portgroup name='sales'> <virtualport> <parameters managerid="55"/> </virtualport> </portgroup> </network>

and a guest <interface> definition of:

<interface type='network'> <source network='testA' portgroup='sales'/> <virtualport> <parameters instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f" interfaceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"\> </virtualport> ... </interface>

If the guest was started on host ABC, the <virtualport> used would be:

<virtualport type='openvswitch'> <parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f' profileid='sales'/> </virtualport>

but if that guest was started on host DEF, the <virtualport> would be:

<virtualport type='802.1Qbg'> <parameters instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f" typeid="1193047" typeidversion="2" managerid="55"/> </virtualport>

Additionally, if none of the involved <virtualport>s had a specified type (this includes cases where no virtualport is given at all), --- docs/formatdomain.html.in | 72 ++++++++++++++++++++++++++++++++++++------- docs/formatnetwork.html.in | 27 ++++++++++++----- src/network/bridge_driver.c | 74 ++++++++++++++++++++++++++++++++------------- 3 files changed, 134 insertions(+), 39 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f97c630..f3b3fa8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2230,11 +2230,40 @@ the network; one network may have multiple portgroups defined, with each portgroup containing slightly different configuration information for different classes of network - connections. <span class="since">Since 0.9.4</span>). Also, - similar to <code>direct</code> network connections (described - below), a connection of type <code>network</code> may specify - a <code>virtportprofile</code> element, with configuration data - to be forwarded to a vepa or 802.1Qbh compliant switch. + connections. <span class="since">Since 0.9.4</span>. + </p> + <p> + Also, similar to <code>direct</code> network connections + (described below), a connection of type <code>network</code> may + specify a <code>virtualport</code> element, with configuration + data to be forwarded to a vepa (802.1Qbg) or 802.1Qbh compliant + switch (<span class="since">Since 0.8.2</span>), or to an + OpenvSwitch virtual switch (<span class="since">Since

If you look at http://openvswitch.org/, you can see they use "Open vSwitch" instead of "OpenvSwitch" as the name. Can you adjust the term in this patch to match that?

+ 0.9.11</span>). + </p> + <p> + Since the actual type of switch may vary depending on the + configuration in the <code><network></code> on the host, + it is acceptable to omit the virtualport <code>type</code> + attribute, and specify attributes from multiple different + virtualport types (and also to leave out certain attributes); at + domain startup time, a complete <code><virtualport></code> + element will be constructed by merging together the type and + attributes found in the which will be filled in from the network + or portgroup <code><virtualport></code>) + (<span class="since">Since 0.10.0</span>). For example, in order + to work properly with both an 802.1Qbh switch and an OpenvSwitch

Same "Open vSwitch" adjustment here.

+ switch, you may choose to specify no type, but both + an <code>instanceid</code> (in case the switch is 802.1Qbh) and + an <code>interfaceid</code> (in case the switch is OpenvSwitch)

Same "Open vSwitch" adjustment here.

+ (you may also omit the other attributes, such as managerid, + typeid, or profileid, to be filled in from the + network's <code><virtualport></code>). If you want to + limit a guest to connecting only to certain types of switches, + you can specify the virtualport type, but still omit some/all of + the parameters - in this case if the host's network has a + different type of virtualport, connection of the interface will + fail. </p>

<pre> @@ -2248,8 +2277,8 @@ <source network='default' portgroup='engineering'/> <target dev='vnet7'/> <mac address="00:11:22:33:44:55"/> - <virtualport type='802.1Qbg'> - <parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> + <virtualport> + <parameters instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> </virtualport>

</interface> @@ -2261,7 +2290,7 @@ <p> <strong><em> This is the recommended config for general guest connectivity on - hosts with static wired networking configs + hosts with static wired networking configs. </em></strong> </p>

@@ -2276,19 +2305,40 @@ configuration is whatever is used on the LAN. This provides the guest VM full incoming & outgoing net access just like a physical machine. </p> - + <p> + On Linux systems, the bridge device is normally a standard Linux + host bridge. On hosts that support OpenvSwitch, it is also

Same "Open vSwitch" adjustment here.

+ possible to connect to an openvswitch bridge device by adding + a <code><virtualport type='openvswitch'/></code> to the + interface definition. (<span class="since">Since + 0.9.11</span>). The OpenvSwitch type virtualport accepts two + parameters in its <code><parameters></code> element - + an <code>interfaceid</code> which is a standard uuid used to + uniquely identify this particular interface to OpenvSwitch (if

One more "Open vSwitch".

+ you do no specify one, a random interfaceid will be generated + for you when you first define the interface), and an + optional <code>profileid</code> which is sent to OpenvSwitch as

Same "Open vSwitch" adjustment here.

+ the interfaces "port-profile". + </p> <pre> ... <devices> + ... <interface type='bridge'> <source bridge='br0'/> </interface> - ... <interface type='bridge'> - <source bridge='br0'/> + <source bridge='br1'/> <target dev='vnet7'/> <mac address="00:11:22:33:44:55"/> </interface> + <interface type='bridge'> + <source bridge='ovsbr'/> + <virtualport type='openvswitch'/> + <parameters profileid='menial' interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> + </virtualport> + </interface> + ... </devices> ...</pre>

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 7e8e991..0ba4d2d 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -147,10 +147,17 @@ This network describes either 1) an existing host bridge that was configured outside of libvirt (if a <code><bridge name='xyz'/></code> element has been - specified), or 2) an interface or group of interfaces to - be used for a "direct" connection via macvtap using - macvtap's "bridge" mode (if the forward element has one or - more <code><interface></code> subelements) + specified, <span class="since">Since 0.9.4</span>), 2) an + existing OpenvSwitch bridge that was configured outside of

"Open vSwitch"

+ libvirt (if both a <code><bridge name='xyz'/></code> + element <b>and</b> a <code><virtualport + type='openvswitch'/></code> have been + specified <span class="since">Since 0.10.0</span>) 3) an + interface or group of interfaces to be used for a "direct" + connection via macvtap using macvtap's "bridge" mode (if + the forward element has one or + more <code><interface></code> + subelements, <span class="since">Since 0.9.4</span>) (see <a href="formatdomain.html#elementsNICSDirect">Direct attachment to physical interface</a> for descriptions of the various macvtap modes). libvirt doesn't attempt to @@ -337,9 +344,15 @@ default portgroup will be used. If no portgroup is given in the interface definition, and there is no default portgroup, then none will be used. Any <code><bandwidth></code> - or <code><virtualport></code> specified directly in the - domain XML will take precedence over any setting in the chosen - portgroup. + + specified directly in the domain XML will take precedence over + any setting in the chosen portgroup. if + a <code><virtualport></code> is specified in the portgroup + (and/or directly in the network definition), the multiple + virtualports will be merged, and any parameter that is specified + in more than one virtualport, and is not identical, will be + considered an error, and will prevent the interface from + starting. </p>

<h3><a name="elementsAddress">Addressing</a></h3> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d9646fb..ec99e4d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -61,6 +61,7 @@ #include "virnetdev.h" #include "virnetdevbridge.h" #include "virnetdevtap.h" +#include "virnetdevvportprofile.h"

#define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" @@ -2746,6 +2747,8 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) virNetworkObjPtr network; virNetworkDefPtr netdef; virPortGroupDefPtr portgroup; + virNetDevVPortProfilePtr virtport = NULL; + virNetworkForwardIfDefPtr dev = NULL; unsigned int num_virt_fns = 0; char **vfname = NULL; int ii; @@ -2820,11 +2823,33 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) goto cleanup; }

+ /* merge virtualports from interface, network, and portgroup to + * arrive at actual virtualport to use + */ + if (virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile, + iface->virtPortProfile, + netdef->virtPortProfile, + portgroup + ? portgroup->virtPortProfile : NULL) < 0) { + goto cleanup; + } + virtport = iface->data.network.actual->virtPortProfile; + if (virtport) { + /* only type='openvswitch' is allowed for bridges */ + if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("<virtualport type='%s'> not supported for network " + "'%s' which uses a bridge device"), + virNetDevVPortTypeToString(virtport->virtPortType), + netdef->name); + goto cleanup; + } + } + } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_BRIDGE) || (netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) || (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) || (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) { - virNetDevVPortProfilePtr virtport = NULL;

/* <forward type='bridge|private|vepa|passthrough'> are all * VIR_DOMAIN_NET_TYPE_DIRECT. @@ -2853,24 +2878,28 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) break; }

- /* Find the most specific virtportprofile and copy it */ - if (iface->virtPortProfile) { - virtport = iface->virtPortProfile; - } else { - if (portgroup) - virtport = portgroup->virtPortProfile; - else - virtport = netdef->virtPortProfile; + /* merge virtualports from interface, network, and portgroup to + * arrive at actual virtualport to use + */ + if (virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile, + iface->virtPortProfile, + netdef->virtPortProfile, + portgroup + ? portgroup->virtPortProfile : NULL) < 0) { + goto cleanup; } + virtport = iface->data.network.actual->virtPortProfile; if (virtport) { - if (VIR_ALLOC(iface->data.network.actual->virtPortProfile) < 0) { - virReportOOMError(); + /* make sure type is supported for macvtap connections */ + if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_8021QBG && + virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_8021QBH) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("<virtualport type='%s'> not supported for network " + "'%s' which uses a macvtap device"), + virNetDevVPortTypeToString(virtport->virtPortType), + netdef->name); goto cleanup; } - /* There are no pointers in a virtualPortProfile, so a shallow copy - * is sufficient - */ - *iface->data.network.actual->virtPortProfile = *virtport; }

/* If there is only a single device, just return it (caller will detect @@ -2883,8 +2912,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) netdef->name); goto cleanup; } else { - virNetworkForwardIfDefPtr dev = NULL; - /* pick an interface from the pool */

/* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require @@ -2967,13 +2994,18 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) virReportOOMError(); goto cleanup; } - /* we are now assured of success, so mark the allocation */ - dev->usageCount++; - VIR_DEBUG("Using physical device %s, usageCount %d", - dev->dev, dev->usageCount); } }

+ if (virNetDevVPortProfileCheckComplete(virtport, true) < 0) + goto cleanup; + + if (dev) { + /* we are now assured of success, so mark the allocation */ + dev->usageCount++; + VIR_DEBUG("Using physical device %s, usageCount %d", + dev->dev, dev->usageCount); + } ret = 0; cleanup: for (ii = 0; ii < num_virt_fns; ii++) -- 1.7.11.2

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list