On Thu, Feb 19, 2015 at 4:18 PM, Laine Stump <[email protected]> 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 <[email protected]> > > --- > > 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.
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
