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

[adding direct cc to Osier] On 04/16/2012 01:58 PM, Cole Robinson wrote: >>>> 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(a)redhat.com> >> Date: Wed Apr 11 22:40:33 2012 +0800 >> >> numad: Ignore cpuset if placement is auto Yes, I independently hit the same problem and conclusion this morning. >> >> So I would think I can call my patch tested now. ;) >> > > Cool, thanks for checking. ACK to the original patch. > > Osier, looks like that patch caused some test fallout?

Daniel and I were discussing it on IRC, although I haven't yet tried patching anything: [09:30] danpb1 eblake: (04:03:19 PM) eblake: +error: operation failed: domain 'test' already exists with uuid 88cb9633-8015-4624-a258-2a3cb9700ad0 [09:31] danpb1 eblake: the fact that we see that error message suggest a pre-existing bug in the test suite - the XML being used to define the test domain is missing a UUID [09:31] danpb1 eblake: so the 2 repeated defines get different auto-generated UUIDs& thus violate the uniqueness check [09:31] danpb1 eblake: i expect if you fixed that problem, then you'd now see the 'define' operation actually suceed

[09:32] eblake but since the test expects a topology syntax error, what changed to make it no longer an error? [09:33] eblake and thus exposing the latent bug in the uuid usage [09:33] danpb1 oh sure, it sounds like we still have a bug in changed semantics [09:34] danpb1 eblake: + if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC) { [09:34] danpb1 that line means we now don't ever parse the placement XML and so won't see the bogus data [09:35] danpb1 since the default value is PLACEMENT_MODE_DEFAULT instead of PLACEMENT_MODE_STATIC [09:35] danpb1 we need to treat DEFAULT + STATIC in the same way in the parser [09:36] eblake DEFAULT is primarily useful for where there is a difference in the XML between omitting something and explicitly requesting STATIC [09:36] eblake but if we want to always default to STATIC, maybe we could just trim the enum to get rid of DEFAULT and make STATIC == 0

[09:37] danpb1 i guess the intent was that we don't suddenly fill all existing XML with a new attribute

[09:38] eblake in which case, default (for omitted) vs. static (explicit, don't lose what the user had) makes sense [09:38] eblake but then I agree that we need to handle both modes the same when deciding what else to parse