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

Message ID 8a1b5962437bfa51e7c8f139673dda4497220527.1463351724.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson May 15, 2016, 10:35 p.m.
Run libvirtd from git with latest qemu, start a VM, stop libvirtd.
Run an older libvirtd version an you may see an error like:

qemuDomainObjPrivateXMLParse:857 : internal error: Unknown qemu capabilities flag device-tray-moved-event

Libvirt finds a cached capabilities flag it doesn't understand, and
fails to parse the VM runtime state. It now thinks the VM isn't
running, when it is. This is potentially serious since it could
lead to disk corruption if the VM is re-run.

For the common case of unknown qemu capabilities flags, treat an
unknown flag as non-fatal and continue on
---
 src/qemu/qemu_domain.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson May 16, 2016, 1 p.m. | #1
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

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);
+            } else {
                 virQEMUCapsSet(qemuCaps, flag);
             }
+            VIR_FREE(str);
         }
 
         priv->qemuCaps = qemuCaps;