Re: [libvirt] [PATCHv2] replace 'if' type conditions with 'switch' for VIR_NETWORK_FORWARD_
On Mon, Jul 23, 2018 at 4:47 PM, Erik wrote:
On Mon, Jul 23, 2018 at 12:03:57PM +0800, Shi Lei wrote:
> v1 patch here: https://www.redhat.com/archives/libvir-list/2018-July/msg01314.html
>
> since v1:
> 1. Change the type declaration of _virNetworkForwardDef.type
> from int to virNetworkForwardType
Maybe I wasn't 100% clear in my response to v1, you have to do the typecast
explicitly in all the switches rather than change the type to an enum in the
struct definition, since the compiler can decide the enum to be unsigned which
means that the return value from the virNetworkForwardTypeFromString call in
virNetworkForwardDefParseXML might get usigned-wrapped, ergo the check for a
non-existent type would be a contradiction.
OK.
> 2. use the default case to report out of range error with
> virReportEnumRangeError
Another thing, it's cool that you incorporated the diff summary. However, this
shouldn't ever be part of the commit message, rather it should be mentioned as
a note in the note section below the "dashed line"
OK.
>
> Signed-off-by: Shi Lei <shilei.massclouds(a)gmx.com>
> ---
> src/conf/domain_conf.c | 49 ++++---
> src/conf/network_conf.c | 73 +++++++---
> src/conf/network_conf.h | 2 +-
> src/conf/virnetworkobj.c | 24 +++-
> src/esx/esx_network_driver.c | 19 ++-
> src/libvirt_private.syms | 1 +
> src/network/bridge_driver.c | 309 ++++++++++++++++++++++++++++---------------
> src/qemu/qemu_process.c | 8 ++
> 8 files changed, 329 insertions(+), 156 deletions(-)
>
...
> + 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);
> + /* No need to check return value since already logged internally */
You can drop ^this comment, the fact that we're purposefully ignoring the return
value should tell the reader something ;).
OK. I'll drop it.
> + ignore_value(networkAddFirewallRules(def));
> + break;
> +
Thanks,
Erik
Thanks!
Shi Lei