[libvirt] [PATCH v2] libvirt: xen: do not use ioemu type for any emulated NIC
Stefan Bader
stefan.bader at canonical.com
Mon Apr 16 18:06:51 UTC 2012
On 16.04.2012 17:21, Cole Robinson wrote:
> On 04/16/2012 10:11 AM, Stefan Bader wrote:
>> 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 at 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 at 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)...
>>
>
> Might be something simple like missing a build dep. Try getting some debug
> output from the tests and it might be clear. If it isn't something simple, I
> can give your patch a spin.
>
> - Cole
So the initial failure turned out to be a missing libsasl2-dev. The other two
are new after the rebase:
TEST: xml2vmxtest
...
31) VMware XML-2-VMX esx-in-the-wild-2 -> esx-in-the-wild-2
@@ -7,7 +7,6 @@
memsize = "1000"
sched.mem.max = "118"
numvcpus = "4"
-sched.cpu.affinity = "0,1,2,5,6,7"
scsi0.present = "true"
scsi0.virtualDev = "lsilogic"
scsi1.present = "true"
32) VMware XML-2-VMX esx-in-the-wild-3 -> esx-in-the-wild-3
@@ -6,7 +6,6 @@
displayName = "virtDebian2"
memsize = "1024"
numvcpus = "2"
-sched.cpu.affinity = "0,3,4,5"
scsi0.present = "true"
scsi0.virtualDev = "lsilogic"
scsi0:0.present = "true"
...
PASS: test_conf.sh
--- exp 2012-04-16 17:27:09.672832070 +0000
+++ out 2012-04-16 17:27:09.672832070 +0000
@@ -1,3 +1,3 @@
error: Failed to define domain from xml-invalid
-error: internal error topology cpuset syntax error
+error: operation failed: domain 'test' already exists with uuid
96b2bf5e-ea49-17b7-ad2c-14308ca3ff59
FAIL: cpuset
All three go away when I revert the following patch:
commit 8fb2164cfff35ce0b87f1d513a0f3ca5111d7880
Author: Osier Yang <jyang at redhat.com>
Date: Wed Apr 11 22:40:33 2012 +0800
numad: Ignore cpuset if placement is auto
So I would think I can call my patch tested now. ;)
-Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120416/d2d03f32/attachment-0001.sig>
More information about the libvir-list
mailing list