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

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(a)midokura.com&gt; > --- > 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).