[libvirt] [PATCH v2 3/3] Add midonet virtual port type support to qemu

[Laine Stump]

On 02/17/2015 09:42 PM, Antoni Segura Puimedon wrote:
> Use the utilities introduced in the previous patches so the qemu
> driver is able to create tap devices that are bound (and unbound
> on domain destroyal) to Midonet virtual ports.
>
> Signed-off-by: Antoni Segura Puimedon <toni+libvirt at midokura.com>
> ---
>  src/conf/domain_conf.h               |  1 +
>  src/conf/netdev_vport_profile_conf.c |  3 ++-

Changes to the parser/formatter should be in the same patch as the
schema/docs changes.


>  src/qemu/qemu_hotplug.c              | 25 ++++++++++++++++++-------
>  src/qemu/qemu_process.c              | 13 +++++++++----
>  src/util/virnetdevtap.c              | 11 ++++++++---
>  5 files changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 325afa8..cafe50f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -42,6 +42,7 @@
>  # include "virnetdevmacvlan.h"
>  # include "virsysinfo.h"
>  # include "virnetdevvportprofile.h"
> +# include "virnetdevmidonet.h"
>  # include "virnetdevopenvswitch.h"
>  # include "virnetdevbandwidth.h"
>  # include "virnetdevvlan.h"
> diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c
> index 8da0838..1641a3e 100644
> --- a/src/conf/netdev_vport_profile_conf.c
> +++ b/src/conf/netdev_vport_profile_conf.c
> @@ -260,7 +260,8 @@ virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort,
>          virBufferAsprintf(buf, " instanceid='%s'", uuidstr);
>      }
>      if (virtPort->interfaceID_specified &&
> -        (type == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH ||
> +        (type == VIR_NETDEV_VPORT_PROFILE_MIDONET ||
> +         type == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH ||
>           type == VIR_NETDEV_VPORT_PROFILE_NONE)) {
>          char uuidstr[VIR_UUID_STRING_BUFLEN];
>  
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 8691c7e..34d7988 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1141,9 +1141,15 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>              }
>  
>              vport = virDomainNetGetActualVirtPortProfile(net);
> -            if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> -               ignore_value(virNetDevOpenvswitchRemovePort(
> -                               virDomainNetGetActualBridgeName(net), net->ifname));
> +            if (vport) {
> +                if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {
> +                    ignore_value(virNetDevMidonetUnbindPort(vport));
> +                } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
> +                    ignore_value(virNetDevOpenvswitchRemovePort(
> +                                     virDomainNetGetActualBridgeName(net),
> +                                     net->ifname));
> +                }
> +            }

To pre-emptively prevent bugs if we ever have to add code to
specifically disconnect  from standard bridges (unlikely but possible),
I think these if's shouldn't be nested. Instead it should be:

     if (vpor && vport->virtPortType == OPENVSWITCH) {
            ovs stuff
     } else if (vport && vport->virtPortType == MIDONET) {
            midonet stuff
     }

(that way we can just add an "else" clause if we have to without
restructuring anything else).

>          }
>  
>          virDomainNetRemoveHostdev(vm->def, net);
> @@ -2934,10 +2940,15 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>      }
>  
>      vport = virDomainNetGetActualVirtPortProfile(net);
> -    if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> -        ignore_value(virNetDevOpenvswitchRemovePort(
> -                        virDomainNetGetActualBridgeName(net),
> -                        net->ifname));
> +    if (vport) {
> +        if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {
> +            ignore_value(virNetDevMidonetUnbindPort(vport));
> +        } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
> +            ignore_value(virNetDevOpenvswitchRemovePort(
> +                             virDomainNetGetActualBridgeName(net),
> +                             net->ifname));
> +        }
> +    }

See the comment above. Same applies here.

>  
>      networkReleaseActualDevice(vm->def, net);
>      virDomainNetDefFree(net);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1d4e957..982f802 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5291,10 +5291,15 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>          /* release the physical device (or any other resources used by
>           * this interface in the network driver
>           */
> -        if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
> -            ignore_value(virNetDevOpenvswitchRemovePort(
> -                                       virDomainNetGetActualBridgeName(net),
> -                                       net->ifname));
> +        if (vport) {
> +            if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {
> +                ignore_value(virNetDevMidonetUnbindPort(vport));
> +            } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
> +                ignore_value(virNetDevOpenvswitchRemovePort(
> +                                 virDomainNetGetActualBridgeName(net),
> +                                 net->ifname));
> +            }
> +        }

and here.

>  
>          /* kick the device out of the hostdev list too */
>          virDomainNetRemoveHostdev(def, net);
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index 83b4131..f6152e9 100644
> --- a/src/util/virnetdevtap.c
> +++ b/src/util/virnetdevtap.c
> @@ -26,6 +26,7 @@
>  #include "virnetdevtap.h"
>  #include "virnetdev.h"
>  #include "virnetdevbridge.h"
> +#include "virnetdevmidonet.h"
>  #include "virnetdevopenvswitch.h"
>  #include "virerror.h"
>  #include "virfile.h"
> @@ -580,9 +581,13 @@ int virNetDevTapCreateInBridgePort(const char *brname,
>          goto error;
>  
>      if (virtPortProfile) {
> -        if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr, vmuuid,
> -                                        virtPortProfile, virtVlan) < 0) {
> -            goto error;
> +        if (virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {
> +            if (virNetDevMidonetBindPort(*ifname, virtPortProfile) < 0)
> +                goto error;
> +        } else {
> +            if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr, vmuuid,
> +                                            virtPortProfile, virtVlan) < 0)
> +                goto error;
>          }
>      } else {
>          if (virNetDevBridgeAddPort(brname, *ifname) < 0)

I know you didn't create the problem in this case, but I think here the
logic should be the following:

   if (virtPortProfile && virtPortType == OPENVSWITCH) {
        virNetDevOpenvswitchAddPort(blah)
   } else if (virtPortProfile && virtPortType == MIDONET) {
        virNetDevMidonetBindPort(blah)
   } else {
        virNetDevBridgeAddPort(blah)
   }

That way if someone specifies a <virtualport> as a placeholder, but no
type, they will still get attached to a standard bridge (which is almost
surely what they will expect).