[libvirt] [PATCH v2] RFC Libvirt + Openvswitch

Ansis Atteka aatteka at nicira.com
Wed Feb 15 17:00:44 UTC 2012


On Mon, Feb 13, 2012 at 9:56 PM, Laine Stump <laine at laine.org> wrote:

> On 02/10/2012 04:09 PM, Ansis Atteka wrote:
> > This patch allows libvirt to add interfaces to already
> > existing Open vSwitch bridges. The following syntax in
> > domain XML file can be used:
> >
> >     <interface type='bridge'>
> >       <mac address='52:54:00:d0:3f:f2'/>
> >       <source bridge='ovsbr'/>
> >       <virtualport type='openvswitch'>
> >         <parameters interfaceid='921a80cd-e6de-5a2e-db9c-ab27f15a6e1d'/>
> >       </virtualport>
> >       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
> >     </interface>
> >
> > or if libvirt should auto-generate the interfaceid use
> > following syntax:
> >
> >     <interface type='bridge'>
> >       <mac address='52:54:00:d0:3f:f2'/>
> >       <source bridge='ovsbr'/>
> >       <virtualport type='openvswitch'>
> >       </virtualport>
>
> Everything in this patch looks really good - it could be pushed with
> just a few nits fixed (detailed later on).
>
> This bit right above which demonstrates a case where interfaceid will be
> autogenerated is the only concern I have left - as I mentioned before,
> although you're doing the auto-generate of the interfaceid exactly the
> same as is done for instanceid (and also uuid and macaddr), this is
> going to cause a problem when we get to defining networks for
> openvswitch. In that case, to specify that a network uses openvswitch,
> we'll want to do this:
>
>   <network>
>     <name>ovs-network</name>
>     <forward mode='bridge'/>
>     <bridge name='ovbr0'/>
>     <virtualport type='openvswitch'>
>     </virtualport>
>   </network>
>
I haven't put a lot of thought into this, but I guess once we get to
networks, we might consider to use following syntax in the network
XML:

      <network>
        <name>ovs-network</name>
        <forward mode='bridge'/>
        <bridge name='ovbr0' type='[openvswitch|linux|default]' />
      </network>

This would ensure that switch from Linux to OVS bridge networks
would not require any changes to existing XML config.

Anyway this is something that would require more careful thinking.

>
> But because an interfaceid is auto-generated by the parser when one
> isn't given, and a common parser function for virtualport is used by the
> domain and network parsers, this will result in an interfaceid being
> filled in when the network is defined, which makes no sense for a
> <network> definition, since each interface that uses this network is
> supposed to have its own unique interfaceid. However, the code decides
> that the bridge is an openvswitch by looking for the presence of a
> virtualport of type='openvswitch'.
>
> I can see a couple ways out of this:
>
> 1) we could add an arg to virNetDevVPortProfileParse:
>
> virNetDevVPortProfilePtr
> virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags);
>
> with a possible flag being VIR_NETDEV_VPORT_AUTOGEN
>
> If that flag is given, interfaceid and instanceid will be auto-generated
> if necessary during parse, otherwise they will be left blank.
>
> 2) Rather than deciding which type of bridge we're dealing with by
> looking at the config, we could try to do it by direct examination of
> the system.
>
The first thing that comes into my mind would be to use SIOCETHTOOL
ioctl call with cmd==ETHTOOL_GDRVINFO. Then the bridge's driver name
would be either "linux" or "openvswitch".

>
> Something along the lines of (2) may be a good idea anyway, especially
> if we think about the idea of a guest that might migrate between a host
> that has openvswitch and one that doesn't, with the restriction that we
> want a persistent interfaceid for the times it's connected to an
> openvswitch. In that case, the interface definition would look like:
>
>    <interface type='network'>
>      <source network='bridged-network'>
>      <mac address='00:11:22:33:44:55'/>
>      <virtualport type='openvswitch'/>
>         <parameters interfaceid='921a80cd-e6de-5a2e-db9c-ab27f15a6e1d'
> profileid='test-profile'/>
>      </virtualport>
>   </interface>
>
> (you would need to specify the virtualport in the <interface> so that it
> would always be the same). Of course if you migrated to a machine where
> bridged-network happened to describe an openvswitch, this would
> coincidentally work correctly. But if bridged-network described a
> standard linux bridge, we would be trying to do openvswitch commands on
> a device that wasn't an openvswitch!
>
> So, I'm wondering if there's a simple way to confirm that a device
> *isn't* an openvswitch without relying on calling any utilities that are
> part of the ovs package.
>
> I'm thinking it may be okay to push this code more or less as-is though,
> since it works properly for the case of straight <interface
> type='bridge'/>, and the changes I'm talking about are refinements that
> are backward compatible, and are only necessary when we add support for
> <network>s that use openvswitch. Any other libvirt regulars have an
> opinion on this?
>
>
> >       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
> >     </interface>
> >
> > It is also possible to pass an optional profileid. To do that
> > use following syntax:
> >
> >    <interface type='bridge'>
> >      <source bridge='ovsbr'/>
> >      <mac address='00:55:1a:65:a2:8d'/>
> >      <virtualport type='openvswitch'>
> >        <parameters interfaceid='921a80cd-e6de-5a2e-db9c-ab27f15a6e1d'
> profileid='test-profile'/>
> >      </virtualport>
> >    </interface>
> >
> > To create Open vSwitch bridge install Open vSwitch and
> > run the following command:
> >
> >     ovs-vsctl add-br ovsbr
> > ---
> >  configure.ac                         |    5 +
> >  src/Makefile.am                      |    1 +
> >  src/conf/domain_conf.c               |   51 +++++++++++--
> >  src/conf/domain_conf.h               |    5 +-
> >  src/conf/netdev_vport_profile_conf.c |   47 +++++++++++-
> >  src/libvirt_private.syms             |    7 ++-
> >  src/lxc/lxc_driver.c                 |   37 +++++++--
> >  src/network/bridge_driver.c          |    3 +-
> >  src/qemu/qemu_command.c              |    5 +-
> >  src/qemu/qemu_hotplug.c              |   17 ++++-
> >  src/qemu/qemu_migration.c            |    4 +-
> >  src/qemu/qemu_process.c              |   10 ++-
> >  src/uml/uml_conf.c                   |    3 +-
> >  src/util/virnetdevopenvswitch.c      |  135
> ++++++++++++++++++++++++++++++++++
> >  src/util/virnetdevopenvswitch.h      |   42 +++++++++++
> >  src/util/virnetdevtap.c              |   14 +++-
> >  src/util/virnetdevtap.h              |    5 +-
> >  src/util/virnetdevvportprofile.c     |    2 +
> >  src/util/virnetdevvportprofile.h     |    7 ++-
> >  19 files changed, 365 insertions(+), 35 deletions(-)
> >  create mode 100644 src/util/virnetdevopenvswitch.c
> >  create mode 100644 src/util/virnetdevopenvswitch.h
> >
> > diff --git a/configure.ac b/configure.ac
> > index 9fb7bfc..dca178f 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -213,6 +213,8 @@ AC_PATH_PROG([UDEVSETTLE], [udevsettle], [],
> >       [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
> >  AC_PATH_PROG([MODPROBE], [modprobe], [],
> >       [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
> > +AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl],
> > +     [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
> >
> >  AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"],
> >          [Location or name of the dnsmasq program])
> > @@ -220,6 +222,9 @@ AC_DEFINE_UNQUOTED([RADVD],["$RADVD"],
> >          [Location or name of the radvd program])
> >  AC_DEFINE_UNQUOTED([TC],["$TC"],
> >          [Location or name of the tc profram (see iproute2)])
> > +AC_DEFINE_UNQUOTED([OVSVSCTL],["$OVSVSCTL"],
> > +        [Location or name of the ovs-vsctl program])
> > +
> >  if test -n "$UDEVADM"; then
> >    AC_DEFINE_UNQUOTED([UDEVADM],["$UDEVADM"],
> >          [Location or name of the udevadm program])
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index a3dd847..d5f52a0 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -97,6 +97,7 @@ UTIL_SOURCES =
>              \
> >               util/virnetdevbandwidth.h util/virnetdevbandwidth.c \
> >               util/virnetdevbridge.h util/virnetdevbridge.c   \
> >               util/virnetdevmacvlan.c util/virnetdevmacvlan.h \
> > +             util/virnetdevopenvswitch.h util/virnetdevopenvswitch.c \
> >               util/virnetdevtap.h util/virnetdevtap.c         \
> >               util/virnetdevveth.h util/virnetdevveth.c \
> >               util/virnetdevvportprofile.h util/virnetdevvportprofile.c \
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index a09a506..5b5db43 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -31,6 +31,7 @@
> >  #include <sys/time.h>
> >  #include <strings.h>
> >
> > +#include "internal.h"
> >  #include "virterror_internal.h"
> >  #include "datatypes.h"
> >  #include "domain_conf.h"
> > @@ -951,6 +952,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr
> def)
> >      switch (def->type) {
> >      case VIR_DOMAIN_NET_TYPE_BRIDGE:
> >          VIR_FREE(def->data.bridge.brname);
> > +        VIR_FREE(def->data.bridge.ovsPort);
>
> We should change all occurrences of ovsPort to virtPortProfile for
> consistency, and to allow for future use of the portprofile here for
> other non-ovs scenarios.
>
> >          break;
> >      case VIR_DOMAIN_NET_TYPE_DIRECT:
> >          VIR_FREE(def->data.direct.linkdev);
> > @@ -994,6 +996,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
> >      case VIR_DOMAIN_NET_TYPE_BRIDGE:
> >          VIR_FREE(def->data.bridge.brname);
> >          VIR_FREE(def->data.bridge.ipaddr);
> > +        VIR_FREE(def->data.bridge.ovsPort);
> >          break;
> >
> >      case VIR_DOMAIN_NET_TYPE_INTERNAL:
> > @@ -3732,7 +3735,11 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> >      }
> >
> >      if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> > -        actual->data.bridge.brname =
> virXPathString("string(./source[1]/@bridge)", ctxt);
> > +        xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt);
> > +        if (virtPortNode &&
> > +            (!(actual->data.bridge.ovsPort =
> > +               virNetDevVPortProfileParse(virtPortNode))))
> > +            goto error;
> >      } else if (actual->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> >          xmlNodePtr virtPortNode;
> >
> > @@ -3861,7 +3868,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
> >                  mode = virXMLPropString(cur, "mode");
> >              } else if ((virtPort == NULL) &&
> >                         ((def->type == VIR_DOMAIN_NET_TYPE_DIRECT) ||
> > -                        (def->type == VIR_DOMAIN_NET_TYPE_NETWORK)) &&
> > +                        (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) ||
> > +                        (def->type == VIR_DOMAIN_NET_TYPE_BRIDGE)) &&
> >                         xmlStrEqual(cur->name, BAD_CAST "virtualport")) {
> >                  if (!(virtPort = virNetDevVPortProfileParse(cur)))
> >                      goto error;
> > @@ -4000,6 +4008,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
> >              def->data.bridge.ipaddr = address;
> >              address = NULL;
> >          }
> > +        def->data.bridge.ovsPort = virtPort;
> > +        virtPort = NULL;
> >          break;
> >
> >      case VIR_DOMAIN_NET_TYPE_CLIENT:
> > @@ -10429,6 +10439,12 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> >      case VIR_DOMAIN_NET_TYPE_BRIDGE:
> >          virBufferEscapeString(buf, "        <source bridge='%s'/>\n",
> >                                def->data.bridge.brname);
> > +        if (def->data.bridge.ovsPort) {
> > +            virBufferAdjustIndent(buf, 6);
> > +            if (virNetDevVPortProfileFormat(def->data.bridge.ovsPort,
> buf) < 0)
> > +                return -1;
> > +            virBufferAdjustIndent(buf, -6);
> > +        }
> >          break;
> >
> >      case VIR_DOMAIN_NET_TYPE_DIRECT:
> > @@ -10516,6 +10532,12 @@ virDomainNetDefFormat(virBufferPtr buf,
> >          if (def->data.bridge.ipaddr)
> >              virBufferAsprintf(buf, "      <ip address='%s'/>\n",
> >                                def->data.bridge.ipaddr);
> > +        if (def->data.bridge.ovsPort) {
> > +            virBufferAdjustIndent(buf, 6);
> > +            if (virNetDevVPortProfileFormat(def->data.bridge.ovsPort,
> buf) < 0)
> > +                return -1;
> > +            virBufferAdjustIndent(buf, -6);
> > +        }
> >          break;
> >
> >      case VIR_DOMAIN_NET_TYPE_SERVER:
> > @@ -13832,15 +13854,27 @@
> virDomainNetGetActualDirectMode(virDomainNetDefPtr iface)
> >  }
> >
> >  virNetDevVPortProfilePtr
> > -virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface)
> > +virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface)
>
>
> In hindsight, renaming this function would have been better done in a
> separate pre-requisite patch, just to avoid clutter in this patch.
>
>
> >  {
> > -    if (iface->type == VIR_DOMAIN_NET_TYPE_DIRECT)
> > +    switch (iface->type) {
> > +    case VIR_DOMAIN_NET_TYPE_DIRECT:
> >          return iface->data.direct.virtPortProfile;
> > -    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> > -        return NULL;
> > -    if (!iface->data.network.actual)
> > +    case VIR_DOMAIN_NET_TYPE_BRIDGE:
> > +        return iface->data.bridge.ovsPort;
> > +    case VIR_DOMAIN_NET_TYPE_NETWORK:
> > +        if (!iface->data.network.actual)
> > +            return NULL;
> > +        switch (iface->data.network.actual->type) {
> > +        case VIR_DOMAIN_NET_TYPE_DIRECT:
> > +            return
> iface->data.network.actual->data.direct.virtPortProfile;
> > +        case VIR_DOMAIN_NET_TYPE_BRIDGE:
> > +            return iface->data.network.actual->data.bridge.ovsPort;
> > +        default:
> > +            return NULL;
> > +        }
> > +    default:
> >          return NULL;
> > -    return iface->data.network.actual->data.direct.virtPortProfile;
> > +    }
> >  }
> >
> >  virNetDevBandwidthPtr
> > @@ -13853,7 +13887,6 @@
> virDomainNetGetActualBandwidth(virDomainNetDefPtr iface)
> >      return iface->bandwidth;
> >  }
> >
> > -
>
> stray extra whitespace change...
>
> >  /* Return listens[ii] from the appropriate union for the graphics
> >   * type, or NULL if this is an unsuitable type, or the index is out of
> >   * bounds. If force0 is TRUE, ii == 0, and there is no listen array,
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 8bb21cf..c4c3551 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -41,6 +41,7 @@
> >  # include "virnetdevmacvlan.h"
> >  # include "sysinfo.h"
> >  # include "virnetdevvportprofile.h"
> > +# include "virnetdevopenvswitch.h"
> >  # include "virnetdevbandwidth.h"
> >
> >  /* Different types of hypervisor */
> > @@ -596,6 +597,7 @@ struct _virDomainActualNetDef {
> >      union {
> >          struct {
> >              char *brname;
> > +            virNetDevVPortProfilePtr ovsPort;
>
>
> These should be named the same as they are in the existing cases
> (virtPortProfile), since there may be other uses for a virtPortProfile
> on a bridge aside from openvswitch.
>
>
> >          } bridge;
> >          struct {
> >              char *linkdev;
> > @@ -647,6 +649,7 @@ struct _virDomainNetDef {
> >          struct {
> >              char *brname;
> >              char *ipaddr;
> > +            virNetDevVPortProfilePtr ovsPort;
>
> Likewise.
>
> >          } bridge;
> >          struct {
> >              char *name;
> > @@ -1875,7 +1878,7 @@ const char
> *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface);
> >  const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface);
> >  int virDomainNetGetActualDirectMode(virDomainNetDefPtr iface);
> >  virNetDevVPortProfilePtr
> > -virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface);
> > +virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface);
> >  virNetDevBandwidthPtr
> >  virDomainNetGetActualBandwidth(virDomainNetDefPtr iface);
> >
> > diff --git a/src/conf/netdev_vport_profile_conf.c
> b/src/conf/netdev_vport_profile_conf.c
> > index b48b2cb..205d674 100644
> > --- a/src/conf/netdev_vport_profile_conf.c
> > +++ b/src/conf/netdev_vport_profile_conf.c
> > @@ -35,7 +35,8 @@
> >  VIR_ENUM_IMPL(virNetDevVPort, VIR_NETDEV_VPORT_PROFILE_LAST,
> >                "none",
> >                "802.1Qbg",
> > -              "802.1Qbh")
> > +              "802.1Qbh",
> > +              "openvswitch")
> >
> >
> >  virNetDevVPortProfilePtr
> > @@ -47,6 +48,7 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> >      char *virtPortTypeIDVersion = NULL;
> >      char *virtPortInstanceID = NULL;
> >      char *virtPortProfileID = NULL;
> > +    char *virtPortInterfaceID = NULL;
> >      virNetDevVPortProfilePtr virtPort = NULL;
> >      xmlNodePtr cur = node->children;
> >
> > @@ -76,7 +78,7 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> >              virtPortTypeIDVersion = virXMLPropString(cur,
> "typeidversion");
> >              virtPortInstanceID = virXMLPropString(cur, "instanceid");
> >              virtPortProfileID = virXMLPropString(cur, "profileid");
> > -
> > +            virtPortInterfaceID = virXMLPropString(cur, "interfaceid");
> >              break;
> >          }
> >
> > @@ -171,6 +173,33 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> >              goto error;
> >          }
> >          break;
> > +    case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
> > +        if (virtPortInterfaceID != NULL) {
> > +            if (virUUIDParse(virtPortInterfaceID,
> > +                             virtPort->u.openvswitch.interfaceID)) {
> > +                virNetDevError(VIR_ERR_XML_ERROR, "%s",
> > +                                     _("cannot parse interfaceid
> parameter as a uuid"));
> > +                goto error;
> > +            }
> > +        } else {
> > +            if (virUUIDGenerate(virtPort->u.openvswitch.interfaceID)) {
> > +                virNetDevError(VIR_ERR_XML_ERROR, "%s",
> > +                                     _("cannot generate a random uuid
> for interfaceid"));
> > +                goto error;
> > +            }
> > +        }
> > +        /* profileid is not mandatory for Open vSwitch */
> > +        if (virtPortProfileID != NULL) {
> > +            if (virStrcpyStatic(virtPort->u.openvswitch.profileID,
> > +                                virtPortProfileID) == NULL) {
> > +                virNetDevError(VIR_ERR_XML_ERROR, "%s",
> > +                                     _("profileid parameter too long"));
>
>
> I'm not sure why the existing profileID is a fixed length. If that's
> something that is limiting to openvswitch, we should take care of that
> limitation. But that shouldn't hole up this patch going in (since you're
> only propagating an existing practice).
>
> > +                goto error;
> > +            }
> > +        } else {
> > +            virtPort->u.openvswitch.profileID[0] = '\0';
> > +        }
> > +        break;
> >
> >      default:
> >          virNetDevError(VIR_ERR_XML_ERROR,
> > @@ -225,6 +254,20 @@
> virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort,
> >                            virtPort->u.virtPort8021Qbh.profileID);
> >          break;
> >
> > +    case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
> > +        virUUIDFormat(virtPort->u.openvswitch.interfaceID,
> > +                      uuidstr);
> > +        if (virtPort->u.openvswitch.profileID[0] == '\0') {
> > +            virBufferAsprintf(buf, "  <parameters interfaceid='%s'/>\n",
> > +                              uuidstr);
> > +        } else {
> > +            virBufferAsprintf(buf, "  <parameters interfaceid='%s' "
> > +                              "profileid='%s'/>\n", uuidstr,
> > +                              virtPort->u.openvswitch.profileID);
> > +        }
> > +
> > +        break;
> > +
> >      default:
> >          virNetDevError(VIR_ERR_XML_ERROR,
> >                         _("unexpected virtualport type %d"),
> virtPort->virtPortType);
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 0c22dec..ad5c7d3 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -378,7 +378,7 @@ virDomainNetGetActualBandwidth;
> >  virDomainNetGetActualBridgeName;
> >  virDomainNetGetActualDirectDev;
> >  virDomainNetGetActualDirectMode;
> > -virDomainNetGetActualDirectVirtPortProfile;
> > +virDomainNetGetActualVirtPortProfile;
>
> We like to keep these in alphabetic order, so this one will have to move
> down.
>
>
> >  virDomainNetGetActualType;
> >  virDomainNetIndexByMac;
> >  virDomainNetInsert;
> > @@ -1237,6 +1237,11 @@ virNetDevMacVLanCreateWithVPortProfile;
> >  virNetDevMacVLanDeleteWithVPortProfile;
> >
> >
> > +# virnetdevopenvswitch.h
> > +virNetDevOpenvswitchAddPort;
> > +virNetDevOpenvswitchRemovePort;
> > +
> > +
> >  # virnetdevtap.h
> >  virNetDevTapCreate;
> >  virNetDevTapCreateInBridgePort;
> > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> > index b712da4..17e8312 100644
> > --- a/src/lxc/lxc_driver.c
> > +++ b/src/lxc/lxc_driver.c
> > @@ -56,6 +56,7 @@
> >  #include "domain_nwfilter.h"
> >  #include "network/bridge_driver.h"
> >  #include "virnetdev.h"
> > +#include "virnetdevtap.h"
> >  #include "virnodesuspend.h"
> >  #include "virtime.h"
> >  #include "virtypedparam.h"
> > @@ -1105,6 +1106,7 @@ static void lxcVmCleanup(lxc_driver_t *driver,
> >      virCgroupPtr cgroup;
> >      int i;
> >      lxcDomainObjPrivatePtr priv = vm->privateData;
> > +    virNetDevVPortProfilePtr vport = NULL;
> >
> >      /* now that we know it's stopped call the hook if present */
> >      if (virHookPresent(VIR_HOOK_DRIVER_LXC)) {
> > @@ -1132,10 +1134,15 @@ static void lxcVmCleanup(lxc_driver_t *driver,
> >      priv->monitorWatch = -1;
> >
> >      for (i = 0 ; i < vm->def->nnets ; i++) {
> > -        ignore_value(virNetDevSetOnline(vm->def->nets[i]->ifname,
> false));
> > -        ignore_value(virNetDevVethDelete(vm->def->nets[i]->ifname));
> > -
> > -        networkReleaseActualDevice(vm->def->nets[i]);
> > +        virDomainNetDefPtr iface = vm->def->nets[i];
> > +        vport = virDomainNetGetActualVirtPortProfile(iface);
> > +        ignore_value(virNetDevSetOnline(iface->ifname, false));
> > +        if (vport && vport->virtPortType ==
> VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> > +            ignore_value(virNetDevOpenvswitchRemovePort(
> > +
> virDomainNetGetActualBridgeName(iface),
> > +                                       iface->ifname));
> > +        ignore_value(virNetDevVethDelete(iface->ifname));
> > +        networkReleaseActualDevice(iface);
> >      }
> >
> >      virDomainConfVMNWFilterTeardown(vm);
> > @@ -1165,6 +1172,7 @@ static int lxcSetupInterfaceBridged(virConnectPtr
> conn,
> >      int ret = -1;
> >      char *parentVeth;
> >      char *containerVeth = NULL;
> > +    const virNetDevVPortProfilePtr vport =
> virDomainNetGetActualVirtPortProfile(net);
> >
> >      VIR_DEBUG("calling vethCreate()");
> >      parentVeth = net->ifname;
> > @@ -1186,7 +1194,11 @@ static int lxcSetupInterfaceBridged(virConnectPtr
> conn,
> >      if (virNetDevSetMAC(containerVeth, net->mac) < 0)
> >          goto cleanup;
> >
> > -    if (virNetDevBridgeAddPort(brname, parentVeth) < 0)
> > +    if (vport && vport->virtPortType ==
> VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> > +        ret = virNetDevOpenvswitchAddPort(brname, parentVeth, net->mac,
> vport);
> > +    else
> > +        ret = virNetDevBridgeAddPort(brname, parentVeth);
> > +    if (ret < 0)
> >          goto cleanup;
> >
> >      if (virNetDevSetOnline(parentVeth, true) < 0)
> > @@ -1242,7 +1254,7 @@ static int lxcSetupInterfaceDirect(virConnectPtr
> conn,
> >       * and automagically dies when the container dies. So
> >       * we have no dev to perform disassociation with.
> >       */
> > -    prof = virDomainNetGetActualDirectVirtPortProfile(net);
> > +    prof = virDomainNetGetActualVirtPortProfile(net);
> >      if (prof) {
> >          lxcError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >                   _("Unable to set port profile on direct interfaces"));
> > @@ -1260,7 +1272,7 @@ static int lxcSetupInterfaceDirect(virConnectPtr
> conn,
> >              virDomainNetGetActualDirectDev(net),
> >              virDomainNetGetActualDirectMode(net),
> >              false, false, def->uuid,
> > -            virDomainNetGetActualDirectVirtPortProfile(net),
> > +            virDomainNetGetActualVirtPortProfile(net),
> >              &res_ifname,
> >              VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
> >              driver->stateDir,
> > @@ -1377,8 +1389,15 @@ static int lxcSetupInterfaces(virConnectPtr conn,
> >
> >  cleanup:
> >      if (ret != 0) {
> > -        for (i = 0 ; i < def->nnets ; i++)
> > -            networkReleaseActualDevice(def->nets[i]);
> > +        for (i = 0 ; i < def->nnets ; i++) {
> > +            virDomainNetDefPtr iface = def->nets[i];
> > +            virNetDevVPortProfilePtr vport =
> virDomainNetGetActualVirtPortProfile(iface);
> > +            if (vport && vport->virtPortType ==
> VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> > +                ignore_value(virNetDevOpenvswitchRemovePort(
> > +
>  virDomainNetGetActualBridgeName(iface),
> > +                                        iface->ifname));
> > +            networkReleaseActualDevice(iface);
> > +        }
>
> While going through this, I noticed what seemed to be some
> inconsistencies in the way lxc cleans up network interfaces (prior to
> your changes), but some of the code also led me to believe I don't
> understand everything that's going on, so I'm going to say your patches
> here are okay, as they don't change behavior for non-ovs bridge devices.
>
> >      }
> >      return ret;
> >  }
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index 57ebb9f..1423d3f 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -1765,7 +1765,8 @@ networkStartNetworkVirtual(struct network_driver
> *driver,
> >              goto err0;
> >          }
> >          if (virNetDevTapCreateInBridgePort(network->def->bridge,
> > -                                           &macTapIfName,
> network->def->mac, 0, false, NULL) < 0) {
> > +                           &macTapIfName, network->def->mac, 0,
> > +                           false, NULL, NULL) < 0) {
>
> There's an indentation problem here.
>
>
> >              VIR_FREE(macTapIfName);
> >              goto err0;
> >          }
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 1b92aa4..591e371 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -154,7 +154,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
> >          virDomainNetGetActualDirectDev(net),
> >          virDomainNetGetActualDirectMode(net),
> >          true, vnet_hdr, def->uuid,
> > -        virDomainNetGetActualDirectVirtPortProfile(net),
> > +        virDomainNetGetActualVirtPortProfile(net),
> >          &res_ifname,
> >          vmop, driver->stateDir,
> >          virDomainNetGetActualBandwidth(net));
> > @@ -247,7 +247,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> >      memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
> >      tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */
> >      err = virNetDevTapCreateInBridgePort(brname, &net->ifname, tapmac,
> > -                                         vnet_hdr, true, &tapfd);
> > +                             vnet_hdr, true, &tapfd,
> > +                             virDomainNetGetActualVirtPortProfile(net));
> >      virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);
> >      if (err < 0) {
> >          if (template_ifname)
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index b4870be..3b1dbbf 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -40,6 +40,7 @@
> >  #include "qemu_cgroup.h"
> >  #include "locking/domain_lock.h"
> >  #include "network/bridge_driver.h"
> > +#include "virnetdevtap.h"
> >
> >  #define VIR_FROM_THIS VIR_FROM_QEMU
> >
> > @@ -652,6 +653,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> >      int vhostfd = -1;
> >      char *nicstr = NULL;
> >      char *netstr = NULL;
> > +    virNetDevVPortProfilePtr vport = NULL;
> >      int ret = -1;
> >      virDomainDevicePCIAddress guestAddr;
> >      int vlan;
> > @@ -838,6 +840,12 @@ cleanup:
> >          if (iface_connected)
> >              virDomainConfNWFilterTeardown(net);
> >
> > +        vport = virDomainNetGetActualVirtPortProfile(net);
> > +        if (vport && vport->virtPortType ==
> VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> > +            ignore_value(virNetDevOpenvswitchRemovePort(
> > +
> virDomainNetGetActualBridgeName(net),
> > +                                       net->ifname));
>
> Again a small indentation problem.
>
> > +
> >          networkReleaseActualDevice(net);
> >      }
> >
> > @@ -1833,6 +1841,7 @@ int qemuDomainDetachNetDevice(struct qemud_driver
> *driver,
> >      qemuDomainObjPrivatePtr priv = vm->privateData;
> >      int vlan;
> >      char *hostnet_name = NULL;
> > +    virNetDevVPortProfilePtr vport = NULL;
> >
> >      for (i = 0 ; i < vm->def->nnets ; i++) {
> >          virDomainNetDefPtr net = vm->def->nets[i];
> > @@ -1923,7 +1932,7 @@ int qemuDomainDetachNetDevice(struct qemud_driver
> *driver,
> >                           detach->ifname, detach->mac,
> >                           virDomainNetGetActualDirectDev(detach),
> >                           virDomainNetGetActualDirectMode(detach),
> > -
> virDomainNetGetActualDirectVirtPortProfile(detach),
> > +                         virDomainNetGetActualVirtPortProfile(detach),
> >                           driver->stateDir));
> >          VIR_FREE(detach->ifname);
> >      }
> > @@ -1938,6 +1947,12 @@ int qemuDomainDetachNetDevice(struct qemud_driver
> *driver,
> >          }
> >      }
> >
> > +    vport = virDomainNetGetActualVirtPortProfile(detach);
> > +    if (vport && vport->virtPortType ==
> VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> > +        ignore_value(virNetDevOpenvswitchRemovePort(
> > +
> virDomainNetGetActualBridgeName(detach),
> > +                                       detach->ifname));
> > +
> >      networkReleaseActualDevice(detach);
> >      if (vm->def->nnets > 1) {
> >          memmove(vm->def->nets + i,
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 12cfbde..a513b3c 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -2563,7 +2563,7 @@
> qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) {
> >          net = def->nets[i];
> >          if (virDomainNetGetActualType(net) ==
> VIR_DOMAIN_NET_TYPE_DIRECT) {
> >              if (virNetDevVPortProfileAssociate(net->ifname,
> > -
> virDomainNetGetActualDirectVirtPortProfile(net),
> > +
> virDomainNetGetActualVirtPortProfile(net),
> >                                                 net->mac,
> >
> virDomainNetGetActualDirectDev(net),
> >                                                 def->uuid,
> > @@ -2580,7 +2580,7 @@ err_exit:
> >          net = def->nets[i];
> >          if (virDomainNetGetActualType(net) ==
> VIR_DOMAIN_NET_TYPE_DIRECT) {
> >              ignore_value(virNetDevVPortProfileDisassociate(net->ifname,
> > -
> virDomainNetGetActualDirectVirtPortProfile(net),
> > +
> virDomainNetGetActualVirtPortProfile(net),
> >                                                             net->mac,
> >
> virDomainNetGetActualDirectDev(net),
> >
> VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH));
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 2d92d66..9bc5436 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -62,6 +62,7 @@
> >  #include "network/bridge_driver.h"
> >  #include "uuid.h"
> >  #include "virtime.h"
> > +#include "virnetdevtap.h"
> >
> >  #define VIR_FROM_THIS VIR_FROM_QEMU
> >
> > @@ -3604,6 +3605,7 @@ void qemuProcessStop(struct qemud_driver *driver,
> >      qemuDomainObjPrivatePtr priv = vm->privateData;
> >      virErrorPtr orig_err;
> >      virDomainDefPtr def;
> > +    virNetDevVPortProfilePtr vport = NULL;
> >      int i;
> >      int logfile = -1;
> >      char *timestamp;
> > @@ -3724,13 +3726,19 @@ void qemuProcessStop(struct qemud_driver *driver,
> >                               net->ifname, net->mac,
> >                               virDomainNetGetActualDirectDev(net),
> >                               virDomainNetGetActualDirectMode(net),
> > -
> virDomainNetGetActualDirectVirtPortProfile(net),
> > +                             virDomainNetGetActualVirtPortProfile(net),
> >                               driver->stateDir));
> >              VIR_FREE(net->ifname);
> >          }
> >          /* release the physical device (or any other resources used by
> >           * this interface in the network driver
> >           */
> > +        vport = virDomainNetGetActualVirtPortProfile(net);
> > +        if (vport && vport->virtPortType ==
> VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> > +            ignore_value(virNetDevOpenvswitchRemovePort(
> > +
> virDomainNetGetActualBridgeName(net),
> > +                                       net->ifname));
> > +
> >          networkReleaseActualDevice(net);
> >      }
> >
> > diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> > index 316d558..8c17578 100644
> > --- a/src/uml/uml_conf.c
> > +++ b/src/uml/uml_conf.c
> > @@ -142,7 +142,8 @@ umlConnectTapDevice(virConnectPtr conn,
> >      memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
> >      tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */
> >      if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, tapmac,
> > -                                       0, true, NULL) < 0) {
> > +                       0, true, NULL,
> > +                       virDomainNetGetActualVirtPortProfile(net)) < 0) {
> >          if (template_ifname)
> >              VIR_FREE(net->ifname);
> >          goto error;
> > diff --git a/src/util/virnetdevopenvswitch.c
> b/src/util/virnetdevopenvswitch.c
> > new file mode 100644
> > index 0000000..24a335a
> > --- /dev/null
> > +++ b/src/util/virnetdevopenvswitch.c
> > @@ -0,0 +1,135 @@
> > +/*
> > + * Copyright (C) 2012 Nicira, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
>  USA
> > + *
> > + * Authors:
> > + *     Dan Wendlandt <dan at nicira.com>
> > + *     Kyle Mestery <kmestery at cisco.com>
> > + *     Ansis Atteka <aatteka at nicira.com>
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include "virnetdevopenvswitch.h"
> > +#include "command.h"
> > +#include "memory.h"
> > +#include "virterror_internal.h"
> > +#include "ignore-value.h"
> > +#include "virmacaddr.h"
> > +
> > +#define VIR_FROM_THIS VIR_FROM_NONE
> > +
> > +/**
> > + * virNetDevOpenvswitchAddPort:
> > + * @brname: the bridge name
> > + * @ifname: the network interface name
> > + * @macaddr: the mac address of the virtual interface
> > + * @ovsport: the ovs specific fields
> > + *
> > + * Add an interface to the OVS bridge
> > + *
> > + * Returns 0 in case of success or -1 in case of failure.
> > + */
> > +int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
> > +                                   const unsigned char *macaddr,
> > +                                   virNetDevVPortProfilePtr ovsport)
> > +{
> > +    int ret = -1;
> > +    virCommandPtr cmd = NULL;
> > +    char macaddrstr[VIR_MAC_STRING_BUFLEN];
> > +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> > +    char *attachedmac_ex_id = NULL;
> > +    char *ifaceid_ex_id = NULL;
> > +    char *profile_ex_id = NULL;
> > +
> > +    virMacAddrFormat(macaddr, macaddrstr);
> > +    virUUIDFormat(ovsport->u.openvswitch.interfaceID, uuidstr);
> > +
> > +    if (virAsprintf(&attachedmac_ex_id,
> "external-ids:attached-mac=\"%s\"",
> > +                    macaddrstr) < 0)
>
> If the quotes aren't necessary, there is a "virCommandAddArgPair" that
> will do this for you once you've created the command:
>
>        virCommandAddArgPair(cmd, "external-ids:attached-mac", macaddrstr);
>
> (I wonder if it would be worthwhile adding a "quote the arg" (and/or
> "quote the arg if necessary") flag to that function (or maybe creating a
> new function virCommandAddArgPairQuoted).
>
> > +        goto cleanup;
> > +    if (virAsprintf(&ifaceid_ex_id, "external-ids:iface-id=\"%s\"",
> > +                    uuidstr) < 0)
> > +        goto cleanup;
> > +    if (ovsport->u.openvswitch.profileID[0] != '\0') {
> > +        if (virAsprintf(&profile_ex_id,
> "external-ids:port-profile=\"%s\"",
> > +                        ovsport->u.openvswitch.profileID) < 0)
> > +            goto cleanup;
> > +    }
> > +
> > +    cmd = virCommandNew(OVSVSCTL);
> > +    if (ovsport->u.openvswitch.profileID[0] == '\0') {
> > +        virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
> > +                        brname, ifname,
> > +                        "--", "set", "Interface", ifname,
> attachedmac_ex_id,
> > +                        "--", "set", "Interface", ifname, ifaceid_ex_id,
> > +                        "--", "set", "Interface", ifname,
> > +                        "external-ids:iface-status=active",
> > +                        NULL);
> > +    } else {
> > +        virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
> > +                        brname, ifname,
> > +                        "--", "set", "Interface", ifname,
> attachedmac_ex_id,
> > +                        "--", "set", "Interface", ifname, ifaceid_ex_id,
> > +                        "--", "set", "Interface", ifname, profile_ex_id,
> > +                        "--", "set", "Interface", ifname,
> > +                        "external-ids:iface-status=active",
> > +                        NULL);
> > +    }
> > +
> > +    if (virCommandRun(cmd, NULL) < 0) {
> > +        virReportSystemError(VIR_ERR_INTERNAL_ERROR,
> > +                             _("Unable to add port %s to OVS bridge
> %s"),
> > +                             ifname, brname);
> > +        goto cleanup;
> > +    }
> > +    ret = 0;
> > +
> > +    cleanup:
> > +        VIR_FREE(attachedmac_ex_id);
> > +        VIR_FREE(ifaceid_ex_id);
> > +        VIR_FREE(profile_ex_id);
> > +        virCommandFree(cmd);
> > +        return ret;
> > +}
> > +
> > +/**
> > + * virNetDevOpenvswitchRemovePort:
> > + * @ifname: the network interface name
> > + *
> > + * Deletes an interface from a OVS bridge
> > + *
> > + * Returns 0 in case of success or -1 in case of failure.
> > + */
> > +int virNetDevOpenvswitchRemovePort(const char *brname ATTRIBUTE_UNUSED,
> const char *ifname)
> > +{
> > +    int ret = -1;
> > +    virCommandPtr cmd = NULL;
> > +
> > +    cmd = virCommandNew(OVSVSCTL);
> > +    virCommandAddArgList(cmd, "--", "--if-exists", "del-port", ifname,
> NULL);
> > +
> > +    if (virCommandRun(cmd, NULL) < 0) {
> > +        virReportSystemError(VIR_ERR_INTERNAL_ERROR,
> > +                             _("Unable to delete port %s from OVS"),
> ifname);
> > +        goto cleanup;
> > +    }
> > +    ret = 0;
> > +
> > +    cleanup:
> > +        virCommandFree(cmd);
> > +        return ret;
> > +}
> > diff --git a/src/util/virnetdevopenvswitch.h
> b/src/util/virnetdevopenvswitch.h
> > new file mode 100644
> > index 0000000..bca4c3e
> > --- /dev/null
> > +++ b/src/util/virnetdevopenvswitch.h
> > @@ -0,0 +1,42 @@
> > +/*
> > + * Copyright (C) 2012 Nicira, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
>  USA
> > + *
> > + * Authors:
> > + *     Dan Wendlandt <dan at nicira.com>
> > + *     Kyle Mestery <kmestery at cisco.com>
> > + *     Ansis Atteka <aatteka at nicira.com>
> > + */
> > +
> > +#ifndef __VIR_NETDEV_OPENVSWITCH_H__
> > +# define __VIR_NETDEV_OPENVSWITCH_H__
> > +
> > +# include "internal.h"
> > +# include "util.h"
> > +# include "virnetdevvportprofile.h"
> > +
> > +
> > +int virNetDevOpenvswitchAddPort(const char *brname,
> > +                                const char *ifname,
> > +                                const unsigned char *macaddr,
> > +                                virNetDevVPortProfilePtr ovsport)
> > +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> > +    ATTRIBUTE_RETURN_CHECK;
> > +
> > +int virNetDevOpenvswitchRemovePort(const char *brname, const char
> *ifname)
> > +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> > +
> > +#endif /* __VIR_NETDEV_OPENVSWITCH_H__ */
> > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> > index 2ed53c6..cd1d4c1 100644
> > --- a/src/util/virnetdevtap.c
> > +++ b/src/util/virnetdevtap.c
> > @@ -25,6 +25,7 @@
> >  #include "virnetdevtap.h"
> >  #include "virnetdev.h"
> >  #include "virnetdevbridge.h"
> > +#include "virnetdevopenvswitch.h"
> >  #include "virterror_internal.h"
> >  #include "virfile.h"
> >  #include "virterror_internal.h"
> > @@ -249,6 +250,7 @@ int virNetDevTapDelete(const char *ifname
> ATTRIBUTE_UNUSED)
> >   * @macaddr: desired MAC address (VIR_MAC_BUFLEN long)
> >   * @vnet_hdr: whether to try enabling IFF_VNET_HDR
> >   * @tapfd: file descriptor return value for the new tap device
> > + * @ovsport: Open vSwitch specific configuration
> >   *
> >   * This function creates a new tap device on a bridge. @ifname can be
> either
> >   * a fixed name or a name template with '%d' for dynamic name
> allocation.
> > @@ -265,7 +267,8 @@ int virNetDevTapCreateInBridgePort(const char
> *brname,
> >                                     const unsigned char *macaddr,
> >                                     int vnet_hdr,
> >                                     bool up,
> > -                                   int *tapfd)
> > +                                   int *tapfd,
> > +                                   virNetDevVPortProfilePtr ovsport)
>
> This arg should be named virtPortProfile for consistency (although I
> think that name is too long, maybe a global replace is in order
> sometime...)
>
> >  {
> >      if (virNetDevTapCreate(ifname, vnet_hdr, tapfd) < 0)
> >          return -1;
> > @@ -286,8 +289,13 @@ int virNetDevTapCreateInBridgePort(const char
> *brname,
> >      if (virNetDevSetMTUFromDevice(*ifname, brname) < 0)
> >          goto error;
> >
> > -    if (virNetDevBridgeAddPort(brname, *ifname) < 0)
> > -        goto error;
> > +    if (ovsport) {
> > +        if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr,
> ovsport) < 0)
> > +            goto error;
> > +    } else {
> > +        if (virNetDevBridgeAddPort(brname, *ifname) < 0)
> > +            goto error;
> > +    }
> >
> >      if (virNetDevSetOnline(*ifname, up) < 0)
> >          goto error;
> > diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
> > index fb35ac5..ee159f7 100644
> > --- a/src/util/virnetdevtap.h
> > +++ b/src/util/virnetdevtap.h
> > @@ -24,6 +24,7 @@
> >  # define __VIR_NETDEV_TAP_H__
> >
> >  # include "internal.h"
> > +# include "virnetdevvportprofile.h"
> >
> >  int virNetDevTapCreate(char **ifname,
> >                         int vnet_hdr,
> > @@ -38,8 +39,10 @@ int virNetDevTapCreateInBridgePort(const char *brname,
> >                                     const unsigned char *macaddr,
> >                                     int vnet_hdr,
> >                                     bool up,
> > -                                   int *tapfd)
> > +                                   int *tapfd,
> > +                                   virNetDevVPortProfilePtr ovsport)
> >      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> >      ATTRIBUTE_RETURN_CHECK;
> >
> > +
> >  #endif /* __VIR_NETDEV_TAP_H__ */
> > diff --git a/src/util/virnetdevvportprofile.c
> b/src/util/virnetdevvportprofile.c
> > index faadc3a..67fd7bc 100644
> > --- a/src/util/virnetdevvportprofile.c
> > +++ b/src/util/virnetdevvportprofile.c
> > @@ -968,6 +968,7 @@ virNetDevVPortProfileAssociate(const char
> *macvtap_ifname,
> >
> >      switch (virtPort->virtPortType) {
> >      case VIR_NETDEV_VPORT_PROFILE_NONE:
> > +    case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
> >      case VIR_NETDEV_VPORT_PROFILE_LAST:
> >          break;
> >
> > @@ -1027,6 +1028,7 @@ virNetDevVPortProfileDisassociate(const char
> *macvtap_ifname,
> >
> >      switch (virtPort->virtPortType) {
> >      case VIR_NETDEV_VPORT_PROFILE_NONE:
> > +    case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
> >      case VIR_NETDEV_VPORT_PROFILE_LAST:
> >          break;
> >
> > diff --git a/src/util/virnetdevvportprofile.h
> b/src/util/virnetdevvportprofile.h
> > index 7a8d81f..ec2d06d 100644
> > --- a/src/util/virnetdevvportprofile.h
> > +++ b/src/util/virnetdevvportprofile.h
> > @@ -35,6 +35,7 @@ enum virNetDevVPortProfile {
> >      VIR_NETDEV_VPORT_PROFILE_NONE,
> >      VIR_NETDEV_VPORT_PROFILE_8021QBG,
> >      VIR_NETDEV_VPORT_PROFILE_8021QBH,
> > +    VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH,
> >
> >      VIR_NETDEV_VPORT_PROFILE_LAST,
> >  };
> > @@ -54,7 +55,7 @@ enum virNetDevVPortProfileOp {
> >  };
> >  VIR_ENUM_DECL(virNetDevVPortProfileOp)
> >
> > -/* profile data for macvtap (VEPA) */
> > +/* profile data for macvtap (VEPA) and openvswitch */
> >  typedef struct _virNetDevVPortProfile virNetDevVPortProfile;
> >  typedef virNetDevVPortProfile *virNetDevVPortProfilePtr;
> >  struct _virNetDevVPortProfile {
> > @@ -69,6 +70,10 @@ struct _virNetDevVPortProfile {
> >          struct {
> >              char          profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
> >          } virtPort8021Qbh;
> > +        struct {
> > +            unsigned char interfaceID[VIR_UUID_BUFLEN];
> > +            char          profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
> > +        } openvswitch;
> >      } u;
> >  };
> >
>
> I didn't see any major problems in this version, so I'm inclined to ACK
> it conditioned on the nits being fixed (they're simple enough I could
> just take care of them while pushing it). The one thing I would like to
> get other opinions of before pushing is problem with auto-generating the
> interfaceid (just in case there's a better way to fix it that would
> required making a change now).
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120215/7d82bb15/attachment-0001.htm>


More information about the libvir-list mailing list