Re: [libvirt] [PATCH v2] libvirt: xen: do not use ioemu type for any emulated NIC

On 16.04.2012 13:58, Cole Robinson wrote: > On 04/13/2012 09:14 AM, Stefan Bader wrote: >>> I think it would be better if we just centralized this logic, as in, >>> only set that (type ioemu) bit in conditional rather than 2. Should >>> be pretty straightforward. >> >> Did you have something like below in mind? >> >> -Stefan >> >> >From a0e214fa6dea9bd66616f37246067a2c631f4184 Mon Sep 17 00:00:00 2001 >> From: Stefan Bader <stefan.bader(a)canonical.com&gt; >> Date: Thu, 12 Apr 2012 15:32:41 +0200 >> Subject: [PATCH] libvirt: xen: do not use ioemu type for any emulated NIC >> >> When using the xm/xend stack to manage instances there is a bug >> that causes the emulated interfaces to be unusable when the vif >> config contains type=ioemu. >> >> The current code already has a special quirk to not use this >> keyword if no specific model is given for the emulated NIC >> (defaulting to rtl8139). >> Essentially it works because regardless of the type argument,i >> the Xen stack always creates emulated and paravirt interfaces and >> lets the guest decide which one to use. So neither xl nor xm stack >> actually require the type keyword for emulated NICs. >> >> [v2: move code around to have the exception in one case together] >> Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; >> --- >> src/xenxs/xen_sxpr.c | 28 +++++++++++++++------------- >> src/xenxs/xen_xm.c | 27 ++++++++++++++------------- >> 2 files changed, 29 insertions(+), 26 deletions(-) >> >> diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c >> index e1bbd76..3a56345 100644 >> --- a/src/xenxs/xen_sxpr.c >> +++ b/src/xenxs/xen_sxpr.c >> @@ -1999,20 +1999,22 @@ xenFormatSxprNet(virConnectPtr conn, >> if (def->model != NULL) >> virBufferEscapeSexpr(buf, "(model '%s')", def->model); >> } >> - else if (def->model == NULL) { >> - /* >> - * apparently (type ioemu) breaks paravirt drivers on HVM so skip >> - * this from XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU >> - */ >> - if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) >> - virBufferAddLit(buf, "(type ioemu)"); >> - } >> - else if (STREQ(def->model, "netfront")) { >> - virBufferAddLit(buf, "(type netfront)"); >> - } >> else { >> - virBufferEscapeSexpr(buf, "(model '%s')", def->model); >> - virBufferAddLit(buf, "(type ioemu)"); >> + if (def->model != NULL && STREQ(def->model, "netfront")) { >> + virBufferAddLit(buf, "(type netfront)"); >> + } >> + else { >> + if (def->model != NULL) { >> + virBufferEscapeSexpr(buf, "(model '%s')", def->model); >> + } >> + /* >> + * apparently (type ioemu) breaks paravirt drivers on HVM so skip >> + * this from XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU >> + */ >> + if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) { >> + virBufferAddLit(buf, "(type ioemu)"); >> + } >> + } >> } >> >> if (!isAttach) >> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c >> index d65e97a..93a26f9 100644 >> --- a/src/xenxs/xen_xm.c >> +++ b/src/xenxs/xen_xm.c >> @@ -1381,20 +1381,21 @@ static int xenFormatXMNet(virConnectPtr conn, >> if (net->model != NULL) >> virBufferAsprintf(&buf, ",model=%s", net->model); >> } >> - else if (net->model == NULL) { >> - /* >> - * apparently type ioemu breaks paravirt drivers on HVM so skip this >> - * from XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU >> - */ >> - if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) >> - virBufferAddLit(&buf, ",type=ioemu"); >> - } >> - else if (STREQ(net->model, "netfront")) { >> - virBufferAddLit(&buf, ",type=netfront"); >> - } >> else { >> - virBufferAsprintf(&buf, ",model=%s", net->model); >> - virBufferAddLit(&buf, ",type=ioemu"); >> + if (net->model != NULL && STREQ(net->model, "netfront")) { >> + virBufferAddLit(&buf, ",type=netfront"); >> + } >> + else { >> + if (net->model != NULL) >> + virBufferAsprintf(&buf, ",model=%s", net->model); >> + >> + /* >> + * apparently type ioemu breaks paravirt drivers on HVM so skip this >> + * from XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU >> + */ >> + if (xendConfigVersion <= XEND_CONFIG_MAX_VERS_NET_TYPE_IOEMU) >> + virBufferAddLit(&buf, ",type=ioemu"); >> + } >> } >> >> if (net->ifname) > > Looks good. Did you verify 'make check' passes as well? > > If so, ACK No :( And it fails. TEST: libvirtdconftest .....!!!!!!...!!!!!!!!!!!!!!!!!!!!!!! 37 FAIL FAIL: libvirtdconftest I guess this is expected as the change really does modify the generated configs. So I need to find out how to make it expect that...