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

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



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 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;
 }


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