[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Aug 17, 2012, at 10:12 AM, Laine Stump wrote:
> 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 cisco com>
>> ---
>> .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!


Thanks Laine, those additions look good to me!

Kyle


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]