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

[Laine Stump]

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


> but I agree with your decision to make the rest
> of this patch Fedora-specific, and hope that by F18 we will have
> automatically phased out enough of the offenders to not have to carry
> the patch around any longer.
>
>> +++ b/src/conf/domain_conf.c
>> @@ -8061,7 +8061,25 @@ virDomainDefPtr virDomainDefParseString(virCapsPtr caps,
>>                                           unsigned int expectedVirtTypes,
>>                                           unsigned int flags)
>>   {
>> -    return virDomainDefParse(xmlStr, NULL, caps, expectedVirtTypes, flags);
>> +    virDomainDefPtr def
>> +        = virDomainDefParse(xmlStr, NULL, caps, expectedVirtTypes, flags);
>> +
>> +    /* Fedora-specific HACK - treat fedora-13 and pc-0.13 as equivalent.
>> +     * This handles the case of domains that had been saved to an image file
>> +     * prior to upgrade (save or snapshot), then restarted/reverted.
>> +     */
>> +    if (def&&  STREQ_NULLABLE(def->os.machine, "fedora-13")) {
> We want the hack to only apply to qemu guests.  At first, I wondered if
> maybe we should move this into files in src/qemu/qemu_*.c; but on
> thinking about it more, that would make the problem get bigger, as there
> are multiple code paths that call this function.  For the purposes of a
> hack that we don't plan to support in F18, I can live with doing it
> here.  But doing it here means we have to worry about this code being
> shared by other drivers.  Therefore, I think you need one additional
> piece of logic gating this hack:
>
>   if (def->virtType == VIR_DOMAIN_VIRT_QEMU ||
>       def->virtType == VIR_DOMAIN_VIRT_KQEMU ||
>       def->virtType == VIR_DOMAIN_VIRT_KVM)


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



>> @@ -11343,8 +11361,20 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>       virBufferAddLit(buf, "<type");
>>       if (def->os.arch)
>>           virBufferAsprintf(buf, " arch='%s'", def->os.arch);
>> -    if (def->os.machine)
>> -        virBufferAsprintf(buf, " machine='%s'", def->os.machine);
>> +    if (def->os.machine) {
>> +        /* Fedora-specific HACK - replace "fedora-13" with "pc-0.13".
>> +         * This will catch XML being written to save/migration images
>> +         * of domains that were running when libvirtd was restarted at
>> +         * the time of upgrade.
>> +         */
>> +        if (STREQ_NULLABLE(def->os.machine, "fedora-13")) {
>> +            virBufferAddLit(buf, " machine='pc-0.13'");
>> +            VIR_WARN("substituting machine type 'fedora-13' with 'pc-0.13' "
>> +                     "in domain %s", def->name);
>> +        } else {
>> +            virBufferAsprintf(buf, " machine='%s'", def->os.machine);
> 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!)


>> \@@ -11779,6 +11809,22 @@ static virDomainObjPtr virDomainLoadConfig(virCapsPtr caps,
>>                                         VIR_DOMAIN_XML_INACTIVE)))
>>           goto error;
>>
>> +    /* Fedora-specific HACK - replace "fedora-13" with "pc-0.13".
>> +     * This updates all config files at the first restart of libvirt
> s/libvirt/libvirtd/
>
>> +     * after upgrade.
>> +     */
>> +    if (STREQ_NULLABLE(def->os.machine, "fedora-13")) {
> Again, this should be gated to just qemu.


Same comment as above. Does anyone else have an opinion on whether or 
not this is necessary/desirable?

If there's one other vote in your suggested direction (or even if 
there's no other votes against it), I'll do it.


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