[libvirt] [PATCH 2/3] xenconfig: add support for openvswitch configuration

[Jim Fehlig]

On 12/6/18 12:44 AM, Michal Privoznik wrote:
> On 11/16/18 11:26 PM, Jim Fehlig wrote:
>> Add support for converting openvswitch interface configuration
>> to/from libvirt domXML and xl.cfg(5). The xl config syntax for
>> virtual interfaces is described in detail in the
>> xl-network-configuration(5) man page. The Xen Networking wiki
>> also contains information and examples for using openvswitch
>> in xl.cfg config format
>>
>> https://wiki.xenproject.org/wiki/Xen_Networking#Open_vSwitch
>>
>> Tests are added to check conversions of openvswitch tagged and
>> trunked VLAN configuration.
>>
>> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
>> ---
>>   src/xenconfig/xen_common.c                    | 113 +++++++++++++++++-
>>   .../test-fullvirt-ovswitch-tagged.cfg         |  25 ++++
>>   .../test-fullvirt-ovswitch-tagged.xml         |  50 ++++++++
>>   .../test-fullvirt-ovswitch-trunked.cfg        |  25 ++++
>>   .../test-fullvirt-ovswitch-trunked.xml        |  51 ++++++++
>>   tests/xlconfigtest.c                          |   2 +
>>   6 files changed, 262 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
>> index 0a9958711f..5390b933e0 100644
>> --- a/src/xenconfig/xen_common.c
>> +++ b/src/xenconfig/xen_common.c
>> @@ -856,6 +856,84 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
>>   }
>>   
>>   
>> +static int
>> +xenParseVifBridge(virDomainNetDefPtr net, char *bridge)
>> +{
>> +    char *vlanstr;
>> +    unsigned int tag;
>> +
>> +    /* 'bridge' string contains a bridge name and single vlan tag */
>> +    vlanstr = strchr(bridge, '.');
>> +    if (vlanstr) {
>> +        if (VIR_STRNDUP(net->data.bridge.brname, bridge, vlanstr - bridge) < 0)
>> +            return -1;
>> +
>> +        vlanstr++;
>> +        if (virStrToLong_ui(vlanstr, NULL, 10, &tag) < 0)
>> +            return -1;
>> +
>> +        if (VIR_ALLOC_N(net->vlan.tag, 1) < 0)
>> +            return -1;
>> +
>> +        net->vlan.tag[0] = tag;
>> +        net->vlan.nTags = 1;
>> +
>> +        if (VIR_ALLOC(net->virtPortProfile) < 0)
>> +            return -1;
>> +
>> +        net->virtPortProfile->virtPortType = VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
>> +        return 0;
>> +    }
>> +
>> +    /* 'bridge' string contains a bridge name and one or more vlan trunks */
>> +    vlanstr = strchr(bridge, ':');
>> +    if (vlanstr) {
>> +        size_t i;
>> +        size_t nvlans = 0;
>> +        char **vlanstr_list = virStringSplit(bridge, ":", 0);
>> +
>> +        if (!vlanstr_list)
>> +            return -1;
>> +
>> +        if (VIR_STRDUP(net->data.bridge.brname, vlanstr_list[0]) < 0) {
>> +            virStringListFree(vlanstr_list);
>> +            return -1;
>> +        }
>> +
>> +        for (i = 1; vlanstr_list[i]; i++)
>> +            nvlans++;
>> +
>> +        if (VIR_ALLOC_N(net->vlan.tag, nvlans) < 0) {
>> +            virStringListFree(vlanstr_list);
>> +            return -1;
>> +        }
>> +
>> +        for (i = 1; i <= nvlans; i++) {
>> +            if (virStrToLong_ui(vlanstr_list[i], NULL, 10, &tag) < 0) {
>> +                virStringListFree(vlanstr_list);
>> +                return -1;
>> +            }
>> +            net->vlan.tag[i - 1] = tag;
>> +        }
>> +        net->vlan.nTags = nvlans;
>> +        net->vlan.trunk = true;
>> +        virStringListFree(vlanstr_list);
>> +
>> +        if (VIR_ALLOC(net->virtPortProfile) < 0)
>> +            return -1;
>> +
>> +        net->virtPortProfile->virtPortType = VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
>> +        return 0;
>> +    }
>> +
>> +    /* 'bridge' string only contains the bridge name */
>> +    if (VIR_STRDUP(net->data.bridge.brname, bridge) < 0)
>> +        return -1;
>> +
>> +    return 0;
> 
> Not a show stopper, I just wonder if we should perhaps use:
> 
> if ((vlanstr = strchr(bridge, '.'))) {
> ...
> } else if ((vlanstr = strchr(bridge, ':'))) {
> ...
> } else {
> ...
> }
> 
> return 0;
> 
> To me that looks a bit cleaner, but I'll leave it up to you. Don't want
> to be picky.

It definitely looks cleaner. I'll squash in the below diff before pushing. 
Thanks a lot for reviewing the patches!

Regards,
Jim


diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 83eb28cdc9..60c8d7edc8 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -862,9 +862,8 @@ xenParseVifBridge(virDomainNetDefPtr net, char *bridge)
      char *vlanstr;
      unsigned int tag;

-    /* 'bridge' string contains a bridge name and single vlan tag */
-    vlanstr = strchr(bridge, '.');
-    if (vlanstr) {
+    if ((vlanstr = strchr(bridge, '.'))) {
+        /* 'bridge' string contains a bridge name and single vlan tag */
          if (VIR_STRNDUP(net->data.bridge.brname, bridge, vlanstr - bridge) < 0)
              return -1;

@@ -883,11 +882,8 @@ xenParseVifBridge(virDomainNetDefPtr net, char *bridge)

          net->virtPortProfile->virtPortType = VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
          return 0;
-    }
-
-    /* 'bridge' string contains a bridge name and one or more vlan trunks */
-    vlanstr = strchr(bridge, ':');
-    if (vlanstr) {
+    } else if ((vlanstr = strchr(bridge, ':'))) {
+        /* 'bridge' string contains a bridge name and one or more vlan trunks */
          size_t i;
          size_t nvlans = 0;
          char **vlanstr_list = virStringSplit(bridge, ":", 0);
@@ -924,12 +920,12 @@ xenParseVifBridge(virDomainNetDefPtr net, char *bridge)

          net->virtPortProfile->virtPortType = VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
          return 0;
+    } else {
+        /* 'bridge' string only contains the bridge name */
+        if (VIR_STRDUP(net->data.bridge.brname, bridge) < 0)
+            return -1;
      }

-    /* 'bridge' string only contains the bridge name */
-    if (VIR_STRDUP(net->data.bridge.brname, bridge) < 0)
-        return -1;
-
      return 0;
  }