<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 19, 2015 at 4:18 PM, Laine Stump <span dir="ltr"><<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">On 02/17/2015 09:42 PM, Antoni Segura Puimedon wrote:<br>
> Use the utilities introduced in the previous patches so the qemu<br>
> driver is able to create tap devices that are bound (and unbound<br>
> on domain destroyal) to Midonet virtual ports.<br>
><br>
> Signed-off-by: Antoni Segura Puimedon <<a href="mailto:toni%2Blibvirt@midokura.com">toni+libvirt@midokura.com</a>><br>
> ---<br>
> src/conf/domain_conf.h | 1 +<br>
> src/conf/netdev_vport_profile_conf.c | 3 ++-<br>
<br>
</span>Changes to the parser/formatter should be in the same patch as the<br>
schema/docs changes.<br></blockquote><div><br>Agreed! Thanks <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="h5"><br>
<br>
> src/qemu/qemu_hotplug.c | 25 ++++++++++++++++++-------<br>
> src/qemu/qemu_process.c | 13 +++++++++----<br>
> src/util/virnetdevtap.c | 11 ++++++++---<br>
> 5 files changed, 38 insertions(+), 15 deletions(-)<br>
><br>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h<br>
> index 325afa8..cafe50f 100644<br>
> --- a/src/conf/domain_conf.h<br>
> +++ b/src/conf/domain_conf.h<br>
> @@ -42,6 +42,7 @@<br>
> # include "virnetdevmacvlan.h"<br>
> # include "virsysinfo.h"<br>
> # include "virnetdevvportprofile.h"<br>
> +# include "virnetdevmidonet.h"<br>
> # include "virnetdevopenvswitch.h"<br>
> # include "virnetdevbandwidth.h"<br>
> # include "virnetdevvlan.h"<br>
> diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c<br>
> index 8da0838..1641a3e 100644<br>
> --- a/src/conf/netdev_vport_profile_conf.c<br>
> +++ b/src/conf/netdev_vport_profile_conf.c<br>
> @@ -260,7 +260,8 @@ virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort,<br>
> virBufferAsprintf(buf, " instanceid='%s'", uuidstr);<br>
> }<br>
> if (virtPort->interfaceID_specified &&<br>
> - (type == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH ||<br>
> + (type == VIR_NETDEV_VPORT_PROFILE_MIDONET ||<br>
> + type == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH ||<br>
> type == VIR_NETDEV_VPORT_PROFILE_NONE)) {<br>
> char uuidstr[VIR_UUID_STRING_BUFLEN];<br>
><br>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c<br>
> index 8691c7e..34d7988 100644<br>
> --- a/src/qemu/qemu_hotplug.c<br>
> +++ b/src/qemu/qemu_hotplug.c<br>
> @@ -1141,9 +1141,15 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,<br>
> }<br>
><br>
> vport = virDomainNetGetActualVirtPortProfile(net);<br>
> - if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)<br>
> - ignore_value(virNetDevOpenvswitchRemovePort(<br>
> - virDomainNetGetActualBridgeName(net), net->ifname));<br>
> + if (vport) {<br>
> + if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {<br>
> + ignore_value(virNetDevMidonetUnbindPort(vport));<br>
> + } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {<br>
> + ignore_value(virNetDevOpenvswitchRemovePort(<br>
> + virDomainNetGetActualBridgeName(net),<br>
> + net->ifname));<br>
> + }<br>
> + }<br>
<br>
</div></div>To pre-emptively prevent bugs if we ever have to add code to<br>
specifically disconnect from standard bridges (unlikely but possible),<br>
I think these if's shouldn't be nested. Instead it should be:<br>
<br>
if (vpor && vport->virtPortType == OPENVSWITCH) {<br>
ovs stuff<br>
} else if (vport && vport->virtPortType == MIDONET) {<br>
midonet stuff<br>
}<br>
<br>
(that way we can just add an "else" clause if we have to without<br>
restructuring anything else).<br></blockquote><div><br>Will do!<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> }<br>
><br>
> virDomainNetRemoveHostdev(vm->def, net);<br>
> @@ -2934,10 +2940,15 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,<br>
> }<br>
><br>
> vport = virDomainNetGetActualVirtPortProfile(net);<br>
> - if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)<br>
> - ignore_value(virNetDevOpenvswitchRemovePort(<br>
> - virDomainNetGetActualBridgeName(net),<br>
> - net->ifname));<br>
> + if (vport) {<br>
> + if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {<br>
> + ignore_value(virNetDevMidonetUnbindPort(vport));<br>
> + } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {<br>
> + ignore_value(virNetDevOpenvswitchRemovePort(<br>
> + virDomainNetGetActualBridgeName(net),<br>
> + net->ifname));<br>
> + }<br>
> + }<br>
<br>
</span>See the comment above. Same applies here.<br></blockquote><div><br>Will do!<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
><br>
> networkReleaseActualDevice(vm->def, net);<br>
> virDomainNetDefFree(net);<br>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c<br>
> index 1d4e957..982f802 100644<br>
> --- a/src/qemu/qemu_process.c<br>
> +++ b/src/qemu/qemu_process.c<br>
> @@ -5291,10 +5291,15 @@ void qemuProcessStop(virQEMUDriverPtr driver,<br>
> /* release the physical device (or any other resources used by<br>
> * this interface in the network driver<br>
> */<br>
> - if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)<br>
> - ignore_value(virNetDevOpenvswitchRemovePort(<br>
> - virDomainNetGetActualBridgeName(net),<br>
> - net->ifname));<br>
> + if (vport) {<br>
> + if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {<br>
> + ignore_value(virNetDevMidonetUnbindPort(vport));<br>
> + } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {<br>
> + ignore_value(virNetDevOpenvswitchRemovePort(<br>
> + virDomainNetGetActualBridgeName(net),<br>
> + net->ifname));<br>
> + }<br>
> + }<br>
<br>
</span>and here.<br></blockquote><div><br>Will do!<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="h5"><br>
><br>
> /* kick the device out of the hostdev list too */<br>
> virDomainNetRemoveHostdev(def, net);<br>
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c<br>
> index 83b4131..f6152e9 100644<br>
> --- a/src/util/virnetdevtap.c<br>
> +++ b/src/util/virnetdevtap.c<br>
> @@ -26,6 +26,7 @@<br>
> #include "virnetdevtap.h"<br>
> #include "virnetdev.h"<br>
> #include "virnetdevbridge.h"<br>
> +#include "virnetdevmidonet.h"<br>
> #include "virnetdevopenvswitch.h"<br>
> #include "virerror.h"<br>
> #include "virfile.h"<br>
> @@ -580,9 +581,13 @@ int virNetDevTapCreateInBridgePort(const char *brname,<br>
> goto error;<br>
><br>
> if (virtPortProfile) {<br>
> - if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr, vmuuid,<br>
> - virtPortProfile, virtVlan) < 0) {<br>
> - goto error;<br>
> + if (virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {<br>
> + if (virNetDevMidonetBindPort(*ifname, virtPortProfile) < 0)<br>
> + goto error;<br>
> + } else {<br>
> + if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr, vmuuid,<br>
> + virtPortProfile, virtVlan) < 0)<br>
> + goto error;<br>
> }<br>
> } else {<br>
> if (virNetDevBridgeAddPort(brname, *ifname) < 0)<br>
<br>
</div></div>I know you didn't create the problem in this case, but I think here the<br>
logic should be the following:<br>
<br>
if (virtPortProfile && virtPortType == OPENVSWITCH) {<br>
virNetDevOpenvswitchAddPort(blah)<br>
} else if (virtPortProfile && virtPortType == MIDONET) {<br>
virNetDevMidonetBindPort(blah)<br>
} else {<br>
virNetDevBridgeAddPort(blah)<br>
}<br>
<br>
That way if someone specifies a <virtualport> as a placeholder, but no<br>
type, they will still get attached to a standard bridge (which is almost<br>
surely what they will expect).<br></blockquote><div><br>Agreed. Much better.<br><br>Thanks a lot for the review Laine. I'll send the updated v3 today. <br></div></div><br></div></div>