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

2018-07-23 Thread Shi Lei
Signed-off-by: Shi Lei 
---

v2 here: https://www.redhat.com/archives/libvir-list/2018-July/msg01423.html
since v2:
 - typecast def->forward.type to virNetworkForwardType explicitly
  in all the switches rather than change its type to enum in
  the struct definition

v1 here: https://www.redhat.com/archives/libvir-list/2018-July/msg01314.html
since v1:
 - Change the type declaration of _virNetworkForwardDef.type
  from int to virNetworkForwardType
 - use the default case to report out of range error with
  virReportEnumRangeError

 src/conf/domain_conf.c   |  49 ---
 src/conf/network_conf.c  |  75 +++---
 src/conf/virnetworkobj.c |  24 +++-
 src/esx/esx_network_driver.c |  19 ++-
 src/libvirt_private.syms |   1 +
 src/network/bridge_driver.c  | 317 ---
 src/qemu/qemu_process.c  |  10 +-
 7 files changed, 334 insertions(+), 161 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 178c6d2..33b0b4a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29975,40 +29975,49 @@ 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 ((virNetworkForwardType) 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) {
-
-/*  
- * 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;
 
-} 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)) {
+case VIR_NETWORK_FORWARD_BRIDGE:
+if (def->bridge) {
+/*  
+ * is VIR_DOMAIN_NET_TYPE_BRIDGE
+ */
+ret = VIR_DOMAIN_NET_TYPE_BRIDGE;
+break;
+}
+
+/* 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:
 /*  are all
  * VIR_DOMAIN_NET_TYPE_DIRECT.
  */
-
 ret = VIR_DOMAIN_NET_TYPE_DIRECT;
+break;
 
+case VIR_NETWORK_FORWARD_LAST:
+default:
+virReportEnumRangeError(virNetworkForwardType, def->forward.type);
+goto cleanup;
 }
 
  cleanup:
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 630a87f..c08456b 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1874,7 +1874,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 /* Validate some items in the main NetworkDef that need to align
  * with the chosen forward mode.
  */
-switch (def->forward.type) {
+switch ((virNetworkForwardType) def->forward.type) {
 case VIR_NETWORK_FORWARD_NONE:
 break;
 
@@ -1955,21 +1955,40 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 goto error;
 }
 break;
+
+case VIR_NETWORK_FORWARD_LAST:
+default:
+virReportEnumRangeError(virNetworkForwardType, def->forward.type);
+goto error;
 }
 
 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 ((virNetworkForwardType) def->forward.type) {
+case VIR_NETWORK_FORWARD_NONE:
+case VIR_NETWORK_FORWARD_NAT:
+case VIR_NETWORK_FORWARD_ROUTE:
+case V

Re: [libvirt] [PATCHv3] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_

2018-07-25 Thread Erik Skultety
On Tue, Jul 24, 2018 at 11:49:48AM +0800, Shi Lei wrote:
> Signed-off-by: Shi Lei 
> ---
>
> v2 here: https://www.redhat.com/archives/libvir-list/2018-July/msg01423.html
> since v2:
>  - typecast def->forward.type to virNetworkForwardType explicitly
>   in all the switches rather than change its type to enum in
>   the struct definition
>
> v1 here: https://www.redhat.com/archives/libvir-list/2018-July/msg01314.html
> since v1:
>  - Change the type declaration of _virNetworkForwardDef.type
>   from int to virNetworkForwardType
>  - use the default case to report out of range error with
>   virReportEnumRangeError
>
...

> @@ -2128,20 +2150,37 @@ 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 ((virNetworkForwardType) 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);
> +/* No need to check return value since already logged internally 
> */

I dropped ^this commentary, adjusted the commit message and pushed the patch.

Congratulations on your first libvirt patch.
Erik

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


Re: [libvirt] [PATCHv3] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_

2018-07-25 Thread Shi Lei
On Wednesday, July 25, 2018 at 8:37 PM, Erik wrote:
> On Tue, Jul 24, 2018 at 11:49:48AM +0800, Shi Lei wrote:
> > Signed-off-by: Shi Lei 
> > ---
> >
> > v2 here: https://www.redhat.com/archives/libvir-list/2018-July/msg01423.html
> > since v2:
> >  - typecast def->forward.type to virNetworkForwardType explicitly
> >   in all the switches rather than change its type to enum in
> >   the struct definition
> >
> > v1 here: https://www.redhat.com/archives/libvir-list/2018-July/msg01314.html
> > since v1:
> >  - Change the type declaration of _virNetworkForwardDef.type
> >   from int to virNetworkForwardType
> >  - use the default case to report out of range error with
> >   virReportEnumRangeError
> >
> ...
> 
> > +if (virNetworkObjIsActive(obj)) {
> > +switch ((virNetworkForwardType) 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);
> > +/* No need to check return value since already logged 
> > internally */
> 
> I dropped ^this commentary, adjusted the commit message and pushed the patch.

Sorry, I forgot to remove this commentary.

> 
> Congratulations on your first libvirt patch.
> Erik
>
 
Thanks!
Shi Lei

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