Re: [PATCHv2 1/4] virDomain: interface: add virDomainNetDefIsOvsport

On 7/1/21 10:42 AM, zhangjl02 wrote: Couple of notes: 1) We like to separate variable declaration block and code with an empty line. 2) The opening curly brace should go onto a new line. I wonder why 'ninja check' did not catch this. Or did you not try? 3) I think that actualType argument is redundant. Why hot just query it inside the function, e.g. like this: virDomainNetType actualType = virDomainNetGetActualType(net); So this is what I think should be squashed in: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 20c60659cd..5a27cd9d7d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29128,9 +29128,12 @@ virDomainNetGetActualVirtPortProfile(const virDomainNetDef *iface) /* Check whether the port is an ovs managed port */ bool -virDomainNetDefIsOvsport(virDomainNetDef *net, int actualType) { +virDomainNetDefIsOvsport(const virDomainNetDef *net) +{ const virNetDevVPortProfile *vport = virDomainNetGetActualVirtPortProfile(net); - return (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) && vport && + virDomainNetType actualType = virDomainNetGetActualType(net); + + return (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) && vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 12ad98ec8c..2a36c5acf1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3611,7 +3611,7 @@ virDomainHostdevDef *virDomainNetGetActualHostdev(virDomainNetDef *iface); const virNetDevVPortProfile * virDomainNetGetActualVirtPortProfile(const virDomainNetDef *iface); bool -virDomainNetDefIsOvsport(virDomainNetDef *net, int actualType); +virDomainNetDefIsOvsport(const virDomainNetDef *net); const virNetDevBandwidth * virDomainNetGetActualBandwidth(const virDomainNetDef *iface); const virNetDevVlan *virDomainNetGetActualVlan(const virDomainNetDef *iface); Michal