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

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