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

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

On 7/1/21 10:42 AM, zhangjl02 wrote:
> From: zhangjl02 <zhangjl02@xxxxxxxxxx>
> 
> Tell whether a port definition is an ovs managed virtual port
> ---
>  src/conf/domain_conf.c   | 8 ++++++++
>  src/conf/domain_conf.h   | 2 ++
>  src/libvirt_private.syms | 1 +
>  3 files changed, 11 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d78f846a52..7fe72fb3f2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -29126,6 +29126,14 @@ virDomainNetGetActualVirtPortProfile(const virDomainNetDef *iface)
>      }
>  }
>  
> +/* Check whether the port is an ovs managed port */
> +bool
> +virDomainNetDefIsOvsport(virDomainNetDef *net, int actualType) {
> +    const virNetDevVPortProfile *vport = virDomainNetGetActualVirtPortProfile(net);
> +    return (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) && vport  &&
> +        vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
> +}
> +

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