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

On 16.04.2012 14:45, Cole Robinson wrote: > On 04/16/2012 08:36 AM, Stefan Bader wrote: >> 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... >> > > Make sure it's actually your patch causing those issues. I wouldn't expect > your change to affect that particular test. > Right, I just ran the checks with my patch reverted and it fails exactly the same way. So I'll rebase to todays HEAD to see whether that is better. But basically you are right, my patch does not change anything.

-Stefan > The first section of HACKING has some info about things to run before > submitting a patch, and ways to debug test failures. > > Thanks, > Cole >