On 02/23/2015 03:54 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@xxxxxxxxxxxx>
> ---
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_hotplug.c | 25 ++++++++++++++++++-------
> src/qemu/qemu_process.c | 13 +++++++++----
> src/util/virnetdevtap.c | 11 ++++++++---
> 4 files changed, 36 insertions(+), 14 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"
I'm not sure why virnetdevopenvswitch.h is included here, as nothing
from it is used in domain_conf.h. Maybe it was there because
virnetdevbandwidth was already there. At any rate, it shouldn't be there
(I'll send a patch to fix that in a bit), and neither should
virnetdevmidonet.h. Instead, virnetdevmidonet.h should be included in
qemu_process.c and qemu_hotplug.c, where the functions are actually used.
I changed that locally and will squash it in.
> # include "virnetdevbandwidth.h"
> # include "virnetdevvlan.h"
> 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));
> + }
> + }
> }
>
> 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));
> + }
> + }
>
> 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));
> + }
> + }
>
So much common code. This should really go in a utility function in the
new qemu_interface.c (along with lots of other code, so it's nothing to
worry about here; just thought I'd mention it)
> /* 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;
> }
As long as we're checking the virtporttype for both cases in all the
other places, we may as well do it here. It may prevent accidentally
doing the wrong thing if we ever add another virtporttype, and may also
help persuade someone to turn the if/else if/else if/... into a switch.
ACK with the #include changes and the above change to the if. I've
squashed in both and am pushing.
Thanks for the contribution!! (and the patience :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list