Re: [libvirt] [PATCH] Add support for setting VLANs on Open vSwitch ports

On 08/17/2012 12:04 AM, Kyle Mestery wrote: > Add the ability to support VLAN tags for Open vSwitch > virtual port types. To accomplish this, modify > virNetDevOpenvswitchAddPort and > virNetDevTapCreateInBridgePort to take a virNetDevVlanPtr > argument. When adding the port to the OVS bridge, setup > either a single VLAN or a trunk port based on the > configuration from the virNetDevVlanPtr. > > Signed-off-by: Kyle Mestery <kmestery(a)cisco.com&gt; > --- > .gnulib | 2 +- > src/lxc/lxc_process.c | 2 +- > src/network/bridge_driver.c | 2 +- > src/qemu/qemu_command.c | 1 + > src/uml/uml_conf.c | 1 + > src/util/virnetdevopenvswitch.c | 34 +++++++++++++++++++++++++++++++--- > src/util/virnetdevopenvswitch.h | 4 +++- > src/util/virnetdevtap.c | 3 ++- > src/util/virnetdevtap.h | 2 ++ > 9 files changed, 43 insertions(+), 8 deletions(-) > > diff --git a/.gnulib b/.gnulib > index 271dd74..dbd9144 160000 > --- a/.gnulib > +++ b/.gnulib > @@ -1 +1 @@ > -Subproject commit 271dd74fdf54ec2a03e73a5173b0b5697f6088f1 > +Subproject commit dbd914496c99c52220e5f5ba4121d6cb55fb3beb > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 046ed86..dc34bef 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -325,7 +325,7 @@ static int virLXCProcessSetupInterfaceBridged(virConnectPtr conn, > > if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) > ret = virNetDevOpenvswitchAddPort(brname, parentVeth, &net->mac, > - vm->uuid, vport); > + vm->uuid, vport, &net->vlan); > else > ret = virNetDevBridgeAddPort(brname, parentVeth); > if (ret < 0) > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index 474bbfa..a78e3b6 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -1763,7 +1763,7 @@ networkStartNetworkVirtual(struct network_driver *driver, > } > if (virNetDevTapCreateInBridgePort(network->def->bridge, > &macTapIfName, &network->def->mac, > - NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) < 0) { > VIR_FREE(macTapIfName); > goto err0; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 9383530..e0062a1 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -255,6 +255,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, > err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, > def->uuid, &tapfd, > virDomainNetGetActualVirtPortProfile(net), > + &net->vlan, > tap_create_flags); > virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0); > if (err < 0) { > diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c > index 4c299d8..5461b42 100644 > --- a/src/uml/uml_conf.c > +++ b/src/uml/uml_conf.c > @@ -141,6 +141,7 @@ umlConnectTapDevice(virConnectPtr conn, > if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, &net->mac, > vm->uuid, NULL, > virDomainNetGetActualVirtPortProfile(net), > + &net->vlan, > VIR_NETDEV_TAP_CREATE_IFUP) < 0) { > if (template_ifname) > VIR_FREE(net->ifname); > diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c > index b57532b..8a31e77 100644 > --- a/src/util/virnetdevopenvswitch.c > +++ b/src/util/virnetdevopenvswitch.c > @@ -46,9 +46,11 @@ > int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, > const virMacAddrPtr macaddr, > const unsigned char *vmuuid, > - virNetDevVPortProfilePtr ovsport) > + virNetDevVPortProfilePtr ovsport, > + virNetDevVlanPtr virtVlan) > { > int ret = -1; > + int i = 0; > virCommandPtr cmd = NULL; > char macaddrstr[VIR_MAC_STRING_BUFLEN]; > char ifuuidstr[VIR_UUID_STRING_BUFLEN]; > @@ -57,6 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, > char *ifaceid_ex_id = NULL; > char *profile_ex_id = NULL; > char *vmid_ex_id = NULL; > + virBufferPtr buf; > > virMacAddrFormat(macaddr, macaddrstr); > virUUIDFormat(ovsport->interfaceID, ifuuidstr); > @@ -76,11 +79,35 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, > ovsport->profileID) < 0) > goto out_of_memory; > } > + if (virtVlan) { > + if (VIR_ALLOC(buf) < 0) > + goto out_of_memory; > + > + /* Trunk port first */ > + if (virtVlan->trunk) { > + virBufferAdd(buf, "trunk=", -1); For literal strings, you should instead use virBufferAddLit(buf, "trunk="). I changed both occurrences. > + > + /* > + * Trunk ports have at least one VLAN. Do the first one > + * outside the "for" loop so we can put a "," at the > + * start of the for loop if there are more than one VLANs > + * on this trunk port. > + */ > + virBufferAsprintf(buf, "%d", virtVlan->tag[i]); > + > + for (i = 1; i < virtVlan->nTags; i++) { > + virBufferAdd(buf, ",", -1); > + virBufferAsprintf(buf, "%d", virtVlan->tag[i]); > + } > + } else { > + virBufferAsprintf(buf, "tag=%d", virtVlan->tag[0]); > + } > + } > > cmd = virCommandNew(OVSVSCTL); > if (ovsport->profileID[0] == '\0') { > virCommandAddArgList(cmd, "--", "--may-exist", "add-port", > - brname, ifname, > + brname, ifname, virBufferCurrentContent(buf), Since you're done with the buffer, you need to call virBufferContentAndReset(buf) instead, otherwise you'll leak the memory pointed to by the buffer. I changed both occurrences of this as well. Aside from those two items, ACK. I squashed in the change in buffer functions and pushed the result. Thanks for the contribution!