[libvirt] [PATCH v2 3/3] Add midonet virtual port type support to qemu
Antoni Segura Puimedon
toni at midokura.com
Thu Feb 19 15:21:51 UTC 2015
On Thu, Feb 19, 2015 at 4:18 PM, Laine Stump <laine at laine.org> wrote:
> 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.
>
Agreed! Thanks
>
>
> > 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).
>
Will do!
>
> > }
> >
> > 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.
>
Will do!
>
> >
> > 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.
>
Will do!
>
> >
> > /* 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).
>
Agreed. Much better.
Thanks a lot for the review Laine. I'll send the updated v3 today.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150219/a10b9993/attachment-0001.htm>
More information about the libvir-list
mailing list