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@xxxxxxx>
> >
> > ---
>
> ...
>
> > + 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
