[libvirt] [PATCH] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_*

Shi Lei posted 1 patch 5 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1532059253-21899-1-git-send-email-shilei.massclouds@gmx.com
Test syntax-check failed
src/conf/domain_conf.c       |  46 +++++-----
src/conf/network_conf.c      |  49 ++++++-----
src/conf/virnetworkobj.c     |  15 ++--
src/esx/esx_network_driver.c |   8 +-
src/network/bridge_driver.c  | 203 +++++++++++++++++++++++--------------------
5 files changed, 178 insertions(+), 143 deletions(-)
[libvirt] [PATCH] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_*
Posted by Shi Lei 5 years, 8 months ago
Hi, everyone!
For VIR_NETWORK_FORWARD_*, I try to replace 'if' type conditions
with typed 'switch()'.
It might be more clear.

Signed-off-by: Shi Lei <shilei.massclouds@gmx.com>

---
 src/conf/domain_conf.c       |  46 +++++-----
 src/conf/network_conf.c      |  49 ++++++-----
 src/conf/virnetworkobj.c     |  15 ++--
 src/esx/esx_network_driver.c |   8 +-
 src/network/bridge_driver.c  | 203 +++++++++++++++++++++++--------------------
 5 files changed, 178 insertions(+), 143 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 178c6d2..c02543f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29975,40 +29975,44 @@ virDomainNetResolveActualType(virDomainNetDefPtr iface)
     if (!(def = virNetworkDefParseString(xml)))
         goto cleanup;
 
-    if ((def->forward.type == VIR_NETWORK_FORWARD_NONE) ||
-        (def->forward.type == VIR_NETWORK_FORWARD_NAT) ||
-        (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) ||
-        (def->forward.type == VIR_NETWORK_FORWARD_OPEN)) {
+    switch (def->forward.type) {
+    case VIR_NETWORK_FORWARD_NONE:
+    case VIR_NETWORK_FORWARD_NAT:
+    case VIR_NETWORK_FORWARD_ROUTE:
+    case VIR_NETWORK_FORWARD_OPEN:
         /* for these forward types, the actual net type really *is*
          * NETWORK; we just keep the info from the portgroup in
          * iface->data.network.actual
          */
         ret = VIR_DOMAIN_NET_TYPE_NETWORK;
+        break;
 
-    } else if ((def->forward.type == VIR_NETWORK_FORWARD_BRIDGE) &&
-               def->bridge) {
-
-        /* <forward type='bridge'/> <bridge name='xxx'/>
-         * is VIR_DOMAIN_NET_TYPE_BRIDGE
-         */
-
-        ret = VIR_DOMAIN_NET_TYPE_BRIDGE;
-
-    } else if (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) {
-
+    case VIR_NETWORK_FORWARD_HOSTDEV:
         ret = VIR_DOMAIN_NET_TYPE_HOSTDEV;
+        break;
+
+    case VIR_NETWORK_FORWARD_BRIDGE:
+        if (def->bridge) {
+            /* <forward type='bridge'/> <bridge name='xxx'/>
+             * is VIR_DOMAIN_NET_TYPE_BRIDGE
+             */
+            ret = VIR_DOMAIN_NET_TYPE_BRIDGE;
+            break;
+        }
 
-    } else if ((def->forward.type == VIR_NETWORK_FORWARD_BRIDGE) ||
-               (def->forward.type == VIR_NETWORK_FORWARD_PRIVATE) ||
-               (def->forward.type == VIR_NETWORK_FORWARD_VEPA) ||
-               (def->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH)) {
+        /* intentionally fall through to the direct case for
+         * VIR_NETWORK_FORWARD_BRIDGE with no bridge device defined
+         */
+        ATTRIBUTE_FALLTHROUGH;
 
+    case VIR_NETWORK_FORWARD_PRIVATE:
+    case VIR_NETWORK_FORWARD_VEPA:
+    case VIR_NETWORK_FORWARD_PASSTHROUGH:
         /* <forward type='bridge|private|vepa|passthrough'> are all
          * VIR_DOMAIN_NET_TYPE_DIRECT.
          */
-
         ret = VIR_DOMAIN_NET_TYPE_DIRECT;
-
+        break;
     }
 
  cleanup:
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 630a87f..bafc1d3 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1959,17 +1959,22 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 
     VIR_FREE(stp);
 
-    if (def->mtu &&
-        (def->forward.type != VIR_NETWORK_FORWARD_NONE &&
-         def->forward.type != VIR_NETWORK_FORWARD_NAT &&
-         def->forward.type != VIR_NETWORK_FORWARD_ROUTE &&
-         def->forward.type != VIR_NETWORK_FORWARD_OPEN)) {
-        virReportError(VIR_ERR_XML_ERROR,
-                       _("mtu size only allowed in open, route, nat, "
-                         "and isolated mode, not in %s (network '%s')"),
-                       virNetworkForwardTypeToString(def->forward.type),
-                       def->name);
-        goto error;
+    if (def->mtu) {
+        switch (def->forward.type) {
+        case VIR_NETWORK_FORWARD_NONE:
+        case VIR_NETWORK_FORWARD_NAT:
+        case VIR_NETWORK_FORWARD_ROUTE:
+        case VIR_NETWORK_FORWARD_OPEN:
+            break;
+
+        default:
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("mtu size only allowed in open, route, nat, "
+                             "and isolated mode, not in %s (network '%s')"),
+                           virNetworkForwardTypeToString(def->forward.type),
+                           def->name);
+            goto error;
+        }
     }
 
     /* Extract custom metadata */
@@ -2349,6 +2354,7 @@ virNetworkDefFormatBuf(virBufferPtr buf,
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     size_t i;
     bool shortforward;
+    bool hasbridge = false;
 
     virBufferAddLit(buf, "<network");
     if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0))
@@ -2469,22 +2475,21 @@ virNetworkDefFormatBuf(virBufferPtr buf,
             virBufferAddLit(buf, "</forward>\n");
     }
 
+    switch (def->forward.type) {
+    case VIR_NETWORK_FORWARD_NONE:
+    case VIR_NETWORK_FORWARD_NAT:
+    case VIR_NETWORK_FORWARD_ROUTE:
+    case VIR_NETWORK_FORWARD_OPEN:
+        hasbridge = true;
+        break;
+    }
 
-    if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
-        def->forward.type == VIR_NETWORK_FORWARD_NAT ||
-        def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
-        def->forward.type == VIR_NETWORK_FORWARD_OPEN ||
-        def->bridge || def->macTableManager) {
-
+    if (hasbridge || def->bridge || def->macTableManager) {
         virBufferAddLit(buf, "<bridge");
         virBufferEscapeString(buf, " name='%s'", def->bridge);
-        if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
-            def->forward.type == VIR_NETWORK_FORWARD_NAT ||
-            def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
-            def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
+        if (hasbridge)
             virBufferAsprintf(buf, " stp='%s' delay='%ld'",
                               def->stp ? "on" : "off", def->delay);
-        }
         if (def->macTableManager) {
             virBufferAsprintf(buf, " macTableManager='%s'",
                              virNetworkBridgeMACTableManagerTypeToString(def->macTableManager));
diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index e00c8a7..7706817 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -1009,19 +1009,22 @@ virNetworkLoadConfig(virNetworkObjListPtr nets,
         goto error;
     }
 
-    if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
-        def->forward.type == VIR_NETWORK_FORWARD_NAT ||
-        def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
-        def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
-
+    switch (def->forward.type) {
+    case VIR_NETWORK_FORWARD_NONE:
+    case VIR_NETWORK_FORWARD_NAT:
+    case VIR_NETWORK_FORWARD_ROUTE:
+    case VIR_NETWORK_FORWARD_OPEN:
         if (!def->mac_specified) {
             virNetworkSetBridgeMacAddr(def);
             virNetworkSaveConfig(configDir, def);
         }
-    } else {
+        break;
+
+    default:
         /* Throw away MAC address for other forward types,
          * which could have been generated by older libvirt RPMs */
         def->mac_specified = false;
+        break;
     }
 
     if (!(obj = virNetworkObjAssignDef(nets, def, 0)))
diff --git a/src/esx/esx_network_driver.c b/src/esx/esx_network_driver.c
index 04118b4..8ea1079 100644
--- a/src/esx/esx_network_driver.c
+++ b/src/esx/esx_network_driver.c
@@ -326,8 +326,12 @@ esxNetworkDefineXML(virConnectPtr conn, const char *xml)
     }
 
     /* FIXME: Add support for NAT */
-    if (def->forward.type != VIR_NETWORK_FORWARD_NONE &&
-        def->forward.type != VIR_NETWORK_FORWARD_BRIDGE) {
+    switch (def->forward.type) {
+    case VIR_NETWORK_FORWARD_NONE:
+    case VIR_NETWORK_FORWARD_BRIDGE:
+        break;
+
+    default:
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("Unsupported forward mode '%s'"),
                        virNetworkForwardTypeToString(def->forward.type));
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index da3c32e..5e885b5 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2088,19 +2088,22 @@ networkRefreshDaemonsHelper(virNetworkObjPtr obj,
 
     virObjectLock(obj);
     def = virNetworkObjGetDef(obj);
-    if (virNetworkObjIsActive(obj) &&
-        ((def->forward.type == VIR_NETWORK_FORWARD_NONE) ||
-         (def->forward.type == VIR_NETWORK_FORWARD_NAT) ||
-         (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) ||
-         (def->forward.type == VIR_NETWORK_FORWARD_OPEN))) {
-        /* Only the three L3 network types that are configured by
-         * libvirt will have a dnsmasq or radvd daemon associated
-         * with them.  Here we send a SIGHUP to an existing
-         * dnsmasq and/or radvd, or restart them if they've
-         * disappeared.
-         */
-        networkRefreshDhcpDaemon(driver, obj);
-        networkRefreshRadvd(driver, obj);
+    if (virNetworkObjIsActive(obj)) {
+        switch (def->forward.type) {
+        case VIR_NETWORK_FORWARD_NONE:
+        case VIR_NETWORK_FORWARD_NAT:
+        case VIR_NETWORK_FORWARD_ROUTE:
+        case VIR_NETWORK_FORWARD_OPEN:
+            /* Only the three L3 network types that are configured by
+             * libvirt will have a dnsmasq or radvd daemon associated
+             * with them.  Here we send a SIGHUP to an existing
+             * dnsmasq and/or radvd, or restart them if they've
+             * disappeared.
+             */
+            networkRefreshDhcpDaemon(driver, obj);
+            networkRefreshRadvd(driver, obj);
+            break;
+        }
     }
     virObjectUnlock(obj);
     return 0;
@@ -2128,18 +2131,21 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr obj,
 
     virObjectLock(obj);
     def = virNetworkObjGetDef(obj);
-    if (virNetworkObjIsActive(obj) &&
-        ((def->forward.type == VIR_NETWORK_FORWARD_NONE) ||
-         (def->forward.type == VIR_NETWORK_FORWARD_NAT) ||
-         (def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) {
-        /* Only three of the L3 network types that are configured by
-         * libvirt need to have iptables rules reloaded. The 4th L3
-         * network type, forward='open', doesn't need this because it
-         * has no iptables rules.
-         */
-        networkRemoveFirewallRules(def);
-        if (networkAddFirewallRules(def) < 0) {
-            /* failed to add but already logged */
+    if (virNetworkObjIsActive(obj)) {
+        switch (def->forward.type) {
+        case VIR_NETWORK_FORWARD_NONE:
+        case VIR_NETWORK_FORWARD_NAT:
+        case VIR_NETWORK_FORWARD_ROUTE:
+            /* Only three of the L3 network types that are configured by
+             * libvirt need to have iptables rules reloaded. The 4th L3
+             * network type, forward='open', doesn't need this because it
+             * has no iptables rules.
+             */
+            networkRemoveFirewallRules(def);
+            if (networkAddFirewallRules(def) < 0) {
+                /* failed to add but already logged */
+            }
+            break;
         }
     }
     virObjectUnlock(obj);
@@ -3273,11 +3279,11 @@ networkValidate(virNetworkDriverStatePtr driver,
     /* Only the three L3 network types that are configured by libvirt
      * need to have a bridge device name / mac address provided
      */
-    if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
-        def->forward.type == VIR_NETWORK_FORWARD_NAT ||
-        def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
-        def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
-
+    switch (def->forward.type) {
+    case VIR_NETWORK_FORWARD_NONE:
+    case VIR_NETWORK_FORWARD_NAT:
+    case VIR_NETWORK_FORWARD_ROUTE:
+    case VIR_NETWORK_FORWARD_OPEN:
         /* if no bridge name was given in the config, find a name
          * unused by any other libvirt networks and assign it.
          */
@@ -3285,7 +3291,9 @@ networkValidate(virNetworkDriverStatePtr driver,
             return -1;
 
         virNetworkSetBridgeMacAddr(def);
-    } else {
+        break;
+
+    default:
         /* They are also the only types that currently support setting
          * a MAC or IP address for the host-side device (bridge), DNS
          * configuration, or network-wide bandwidth limits.
@@ -3331,6 +3339,7 @@ networkValidate(virNetworkDriverStatePtr driver,
             return -1;
         }
         bandwidthAllowed = false;
+        break;
     }
 
     /* we support configs with a single PF defined:
@@ -3755,9 +3764,10 @@ networkUpdate(virNetworkPtr net,
         /* Take care of anything that must be done before updating the
          * live NetworkDef.
          */
-        if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
-            def->forward.type == VIR_NETWORK_FORWARD_NAT ||
-            def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
+        switch (def->forward.type) {
+        case VIR_NETWORK_FORWARD_NONE:
+        case VIR_NETWORK_FORWARD_NAT:
+        case VIR_NETWORK_FORWARD_ROUTE:
             switch (section) {
             case VIR_NETWORK_SECTION_FORWARD:
             case VIR_NETWORK_SECTION_FORWARD_INTERFACE:
@@ -3768,14 +3778,13 @@ networkUpdate(virNetworkPtr net,
                  * old rules (and remember to load new ones after the
                  * update).
                  */
-                if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) {
-                    networkRemoveFirewallRules(def);
-                    needFirewallRefresh = true;
-                }
+                networkRemoveFirewallRules(def);
+                needFirewallRefresh = true;
                 break;
             default:
                 break;
             }
+            break;
         }
     }
 
@@ -4440,10 +4449,11 @@ networkAllocateActualDevice(virDomainDefPtr dom,
        iface->data.network.actual->trustGuestRxFilters
           = netdef->trustGuestRxFilters;
 
-    if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) ||
-        (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) ||
-        (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE) ||
-        (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN)) {
+    switch (netdef->forward.type) {
+    case VIR_NETWORK_FORWARD_NONE:
+    case VIR_NETWORK_FORWARD_NAT:
+    case VIR_NETWORK_FORWARD_ROUTE:
+    case VIR_NETWORK_FORWARD_OPEN:
         /* for these forward types, the actual net type really *is*
          * NETWORK; we just keep the info from the portgroup in
          * iface->data.network.actual
@@ -4463,46 +4473,9 @@ networkAllocateActualDevice(virDomainDefPtr dom,
 
         if (networkPlugBandwidth(obj, iface) < 0)
             goto error;
+        break;
 
-    } else if ((netdef->forward.type == VIR_NETWORK_FORWARD_BRIDGE) &&
-               netdef->bridge) {
-
-        /* <forward type='bridge'/> <bridge name='xxx'/>
-         * is VIR_DOMAIN_NET_TYPE_BRIDGE
-         */
-
-        iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
-        if (VIR_STRDUP(iface->data.network.actual->data.bridge.brname,
-                       netdef->bridge) < 0)
-            goto error;
-        iface->data.network.actual->data.bridge.macTableManager
-           = netdef->macTableManager;
-
-        /* merge virtualports from interface, network, and portgroup to
-         * arrive at actual virtualport to use
-         */
-        if (virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile,
-                                        iface->virtPortProfile,
-                                        netdef->virtPortProfile,
-                                        portgroup
-                                        ? portgroup->virtPortProfile : NULL) < 0) {
-            goto error;
-        }
-        virtport = iface->data.network.actual->virtPortProfile;
-        if (virtport) {
-            /* only type='openvswitch' is allowed for bridges */
-            if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("<virtualport type='%s'> not supported for network "
-                                 "'%s' which uses a bridge device"),
-                               virNetDevVPortTypeToString(virtport->virtPortType),
-                               netdef->name);
-                goto error;
-            }
-        }
-
-    } else if (netdef->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) {
-
+    case VIR_NETWORK_FORWARD_HOSTDEV: {
         virDomainHostdevSubsysPCIBackendType backend;
 
         iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_HOSTDEV;
@@ -4575,12 +4548,55 @@ networkAllocateActualDevice(virDomainDefPtr dom,
                 goto error;
             }
         }
+        break;
+    }
+
+    case VIR_NETWORK_FORWARD_BRIDGE:
+        if (netdef->bridge) {
+            /* <forward type='bridge'/> <bridge name='xxx'/>
+             * is VIR_DOMAIN_NET_TYPE_BRIDGE
+             */
+
+            iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
+            if (VIR_STRDUP(iface->data.network.actual->data.bridge.brname,
+                           netdef->bridge) < 0)
+                goto error;
+            iface->data.network.actual->data.bridge.macTableManager
+               = netdef->macTableManager;
+
+            /* merge virtualports from interface, network, and portgroup to
+             * arrive at actual virtualport to use
+             */
+            if (virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile,
+                                            iface->virtPortProfile,
+                                            netdef->virtPortProfile,
+                                            portgroup
+                                            ? portgroup->virtPortProfile : NULL) < 0) {
+                goto error;
+            }
+            virtport = iface->data.network.actual->virtPortProfile;
+            if (virtport) {
+                /* only type='openvswitch' is allowed for bridges */
+                if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                                   _("<virtualport type='%s'> not supported for network "
+                                     "'%s' which uses a bridge device"),
+                                   virNetDevVPortTypeToString(virtport->virtPortType),
+                                   netdef->name);
+                    goto error;
+                }
+            }
+            break;
+        }
 
-    } else if ((netdef->forward.type == VIR_NETWORK_FORWARD_BRIDGE) ||
-               (netdef->forward.type == VIR_NETWORK_FORWARD_PRIVATE) ||
-               (netdef->forward.type == VIR_NETWORK_FORWARD_VEPA) ||
-               (netdef->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH)) {
+        /* intentionally fall through to the direct case for
+         * VIR_NETWORK_FORWARD_BRIDGE with no bridge device defined
+         */
+        ATTRIBUTE_FALLTHROUGH;
 
+    case VIR_NETWORK_FORWARD_PRIVATE:
+    case VIR_NETWORK_FORWARD_VEPA:
+    case VIR_NETWORK_FORWARD_PASSTHROUGH:
         /* <forward type='bridge|private|vepa|passthrough'> are all
          * VIR_DOMAIN_NET_TYPE_DIRECT.
          */
@@ -4680,6 +4696,7 @@ networkAllocateActualDevice(virDomainDefPtr dom,
                            dev->device.dev) < 0)
                 goto error;
         }
+        break;
     }
 
     if (virNetworkObjMacMgrAdd(obj, driver->dnsmasqStateDir,
@@ -5037,13 +5054,15 @@ networkReleaseActualDevice(virDomainDefPtr dom,
     }
     netdef = virNetworkObjGetDef(obj);
 
-    if (iface->data.network.actual &&
-        (netdef->forward.type == VIR_NETWORK_FORWARD_NONE ||
-         netdef->forward.type == VIR_NETWORK_FORWARD_NAT ||
-         netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
-         netdef->forward.type == VIR_NETWORK_FORWARD_OPEN) &&
-        networkUnplugBandwidth(obj, iface) < 0)
-        goto error;
+    switch (netdef->forward.type) {
+    case VIR_NETWORK_FORWARD_NONE:
+    case VIR_NETWORK_FORWARD_NAT:
+    case VIR_NETWORK_FORWARD_ROUTE:
+    case VIR_NETWORK_FORWARD_OPEN:
+        if (iface->data.network.actual && networkUnplugBandwidth(obj, iface) < 0)
+            goto error;
+        break;
+    }
 
     if ((!iface->data.network.actual) ||
         ((actualType != VIR_DOMAIN_NET_TYPE_DIRECT) &&
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_*
Posted by Erik Skultety 5 years, 8 months ago
On Fri, Jul 20, 2018 at 12:00:53PM +0800, Shi Lei wrote:
> Hi, everyone!
> For VIR_NETWORK_FORWARD_*, I try to replace 'if' type conditions
> with typed 'switch()'.
> It might be more clear.
>
> Signed-off-by: Shi Lei <shilei.massclouds@gmx.com>
>
> ---

...

> +            networkRemoveFirewallRules(def);
> +            if (networkAddFirewallRules(def) < 0) {
> +                /* failed to add but already logged */
> +            }

^This if should go away, just call networkAddFirewallRules(def);

...

Conceptually, I agree with your changes, however, since you're already doing
the convert, recently we've changed our practice on how we approach the
switches. First, you should typecast def->forward.type to virNetworkForwardType
explicitly, so that we can utilize compile time checks. Second, we use the
default case only to report an out of range error with virReportEnumRangeError
(mainly because the values you care about are all already enumerated), you can
find plenty of examples throughout the code base if you look for the function I
just mentioned. No other objections from my side.

Regards,
Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_*
Posted by Shi Lei 5 years, 8 months ago

On Fri, July 20, 2018 at 4:52 PM, Erik wrote:
> On Fri, Jul 20, 2018 at 12:00:53PM +0800, Shi Lei wrote:
> > Hi, everyone!
> > For VIR_NETWORK_FORWARD_*, I try to replace 'if' type conditions
> > with typed 'switch()'.
> > It might be more clear.
> >
> > Signed-off-by: Shi Lei <shilei.massclouds@gmx.com>
> >
> > ---
> 
> ...
> 
> > +            networkRemoveFirewallRules(def);
> > +            if (networkAddFirewallRules(def) < 0) {
> > +                /* failed to add but already logged */
> > +            }
> 
> ^This if should go away, just call networkAddFirewallRules(def);

OK. 

> 
> ...
> 
> Conceptually, I agree with your changes, however, since you're already doing
> the convert, recently we've changed our practice on how we approach the
> switches. First, you should typecast def->forward.type to virNetworkForwardType
> explicitly, so that we can utilize compile time checks. Second, we use the
> default case only to report an out of range error with virReportEnumRangeError
> (mainly because the values you care about are all already enumerated), you can
> find plenty of examples throughout the code base if you look for the function I
> just mentioned. No other objections from my side.
> 
> Regards,
> Erik
> 

Thanks for your reviewing and directions, Erik!
And I have found examples you mentioned.
Have a nice day!

Shi Lei

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list