[libvirt] [PATCH] qemu: Don't lose VM runtime state on libvirt downgrade

[Cole Robinson]

On 05/16/2016 02:59 AM, Peter Krempa wrote:
> On Sun, May 15, 2016 at 18:35:24 -0400, Cole Robinson wrote:
>> Run libvirtd from git with latest qemu, start a VM, stop libvirtd.
>> Run an older libvirtd version an you may see an error like:
> 
> We explicitly don't support downgrades. It will be very hard to handle
> this correctly ... let me explain:
> 
> [...]
> 
>> ---
>>  src/qemu/qemu_domain.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index b0eb3b6..dbf8124 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1411,18 +1411,18 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
>>              goto error;
>>  
>>          for (i = 0; i < n; i++) {
>> +            int flag;
>>              char *str = virXMLPropString(nodes[i], "name");
>> -            if (str) {
>> -                int flag = virQEMUCapsTypeFromString(str);
>> -                if (flag < 0) {
>> -                    virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                                   _("Unknown qemu capabilities flag %s"), str);
>> -                    VIR_FREE(str);
>> -                    goto error;
>> -                }
>> -                VIR_FREE(str);
>> +            if (!str)
>> +                continue;
>> +
>> +            flag = virQEMUCapsTypeFromString(str);
>> +            if (flag < 0) {
>> +                VIR_WARN("Unknown qemu capabilities flag %s", str);
> 
> At this point the VM was created with a set of capabilities known by
> the newer libvirt version which may change the behavior in a way where
> only the new code can handle it.
> 
> One of recent examples would be QEMU_CAPS_NAME_DEBUG_THREADS which
> broke one of the APIs returning stats about the thread due to change
> in the naming. The API was fixed along with the addition of the
> capability. If any previous version would contain this code the API
> would start to fail after a downgrade.
> 
>> +            } else {
>>                  virQEMUCapsSet(qemuCaps, flag);
>>              }
>> +            VIR_FREE(str);
>>          }
> 
> NACK to this approach. If you want to fix the disk corruption issue
> which is legitimate you need to kill the running VM process with missing
> capabilities. Making silently ignore new caps is asking for trouble and
> complications in the long run since we'd need to start to be forward
> compatible.
> 
> One of the troublesome approaches could be introduced by
> QEMU_CAPS_OBJECT_SECRET which needs different handling in the hotplug
> code (not yet implemented).
> 
> Also this would create a false sense of that we actually do support
> downgrades which I don't think we ever want to do.
> 

Makes sense, thanks for explaining. I'll drop this patch

- Cole