[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 canonical com>
>>>>> 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 canonical com>
>>>>> ---
>>>>>  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)...

-Stefan

> -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
>>
> 
> 


Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]