[fedora-virt] [fedora-specific libvirt PATCH] qemu: replace deprecated fedora-13 machine type with pc-0.13

[Eric Blake]

On 12/08/2011 06:56 PM, Laine Stump wrote:
> On 12/08/2011 05:43 PM, Eric Blake wrote:
>> On 12/08/2011 03:08 PM, Laine Stump wrote:
>>> (I'm thinking of appying this patch to libvirt in the F15 and F16
>>> branches, as well as Rawhide. The intent is to prepare everyone for
>>> qemu's removal of support for the "fedora-13" machine type, which will
>>> happen in F17 (already done in Rawhide)).
>>>
>>> This addresses https://bugzilla.redhat.com/show_bug.cgi?id=754772 .
>>> It should only be applied to Fedora builds of libvirt (it seems
>>> appropriate to apply it to all versions currently in support).
>>>
>>> ---
>>>   src/conf/domain_conf.c                             |   52 ++++++++++++++++++-
>>>   .../qemuxml2argv-encrypted-disk.args               |    2 +-
>>>   .../qemuxml2argv-encrypted-disk.xml                |    2 +-
>> The 4 line change to the testsuite is probably worth proposing upstream
>> (upstream should try to favor upstream qemu names, rather than a
>> Fedora-specific name),
> 
> 
> I thought of that, but then realized that it's irrelevant, since that 
> config / commandline is never passed to an actual qemu, and the tests 
> still pass.
> 
> I can still send it separately upstream, but it needs to stay here, 
> since make check will otherwise fail once the rest of the patch is 
> applied. (I suppose I can put it in a separate patch, so that the main 
> patch can be the same for rawhide and F16.)

Yes, the tests fix needs to be present in Fedora, whether bundled in
this Fedora-specific patch or as a backport of the upstream tweak.

> This looks really ugly to me. Is it really necessary, seeing that none 
> of the other hypervisor types uses the machine name "fedora-13"? As a 
> matter of fact it looks like os.machine is only used in 2 places outside 
> of domain_conf.c and the qemu directory:
> 
> *** src/xenxs/xen_xm.c:
> xenParseXM[271]                if (!(def->os.machine = 
> strdup(defaultMachine)))
> 
> *** src/vbox/vbox_tmpl.c:
> <global>[3746]                 VIR_DEBUG("def->os.machine  %s", 
> def->os.machine);

Okay, you're starting to convince me that it probably won't hurt xen or
vbox, and no one else cared.  If no one else speaks up in the next day
or so, then I won't mind the simpler patch that skips the gating.

>> Same comment about gating this to just qemu.  Also, do we want to print
>> the VIR_WARN on every dumpxml, or do we want to modify def->os.machine
>> in-place so that the remaining uses of the def are already at pc-0.13?
> 
> 
> I had thought about that, but the idea of modifying an object that the 
> caller is expecting to not change wasn't very appealing to me. I could 
> be persuaded otherwise, though.
> 
> (Actually I thought I'd just removed that VIR_WARN(), because I'd 
> discovered that during the managedsave of a single 1GB domain, 
> virDomainFormatDef was called 234 times!)

I'm okay with not doing the in-place modification, but then you are
correct that we shouldn't VIR_WARN in that case.

>> Except for the missing gating, it looks reasonable; I'll take some time
>> to test a domain snapshot taken before the patch and loaded after and
>> report back with that (but at first glance, it should work).
>>
> 
> 
> It may also be useful to start a domain running prior to the upgrade, 
> then do a snapshot after the upgrade is finished and see if it gets the 
> right machine type in the xml that's stored in the snapshot file.

Yep, I'll include that in my testing results.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
Url : http://lists.fedoraproject.org/pipermail/virt/attachments/20111209/88a20db5/attachment.bin