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

On 16.04.2012 14:54, Stefan Bader wrote: > 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. > Rebasing to HEAD made things rather worse... now 3 checks are failing (without my patch)...