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

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(a)gmx.com&gt; > > --- ... > + 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