On Thu, Feb 11, 2021 at 16:37:53 +0100, Peter Krempa wrote:
Add status XML infrastructure for storing a list of block dirty bitmaps which are temporarily used when migrating a VM with VIR_MIGRATE_NON_SHARED_DISK for cleanup after a libvirtd restart during migration.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 90 ++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_domain.h | 15 +++++++ 2 files changed, 102 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 53b4fb5f4f..ed37917670 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -86,6 +86,18 @@ qemuJobAllocPrivate(void) }
+void +qemuDomainJobPrivateMigrateTempBitmapFree(qemuDomainJobPrivateMigrateTempBitmapPtr bmp) +{ + if (!bmp) + return; + + g_free(bmp->nodename); + g_free(bmp->bitmapname); + g_free(bmp); +} + + static void qemuJobFreePrivate(void *opaque) { @@ -95,6 +107,9 @@ qemuJobFreePrivate(void *opaque) return;
qemuMigrationParamsFree(priv->migParams); + if (priv->migTempBitmaps) + g_slist_free_full(priv->migTempBitmaps, + (GDestroyNotify) qemuDomainJobPrivateMigrateTempBitmapFree);
I just realized this now although the same pattern is used in previous patches... Shouldn't g_slist_free_full be able to cope with NULL making the if () check before it redundant?
VIR_FREE(priv); }
@@ -165,6 +180,28 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf, return 0; }
+ +static void +qemuDomainObjPrivateXMLFormatMigrationBlockDirtyBitmapsTemp(virBufferPtr buf, + GSList *bitmaps)
The naming is pretty inconsistent here, how about qemuDomainObjPrivateXMLFormatMigrateTempBitmap(...)?
+{ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + GSList *next; + + for (next = bitmaps; next; next = next->next) { + qemuDomainJobPrivateMigrateTempBitmapPtr t = next->data; + g_auto(virBuffer) bitmapBuf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&bitmapBuf, " name='%s'", t->bitmapname); + virBufferAsprintf(&bitmapBuf, " nodename='%s'", t->nodename); + + virXMLFormatElement(&childBuf, "bitmap", &bitmapBuf, NULL); + } + + virXMLFormatElement(buf, "tempBlockDirtyBitmaps", NULL, &childBuf); +} + + static int qemuDomainFormatJobPrivate(virBufferPtr buf, qemuDomainJobObjPtr job, @@ -172,9 +209,14 @@ qemuDomainFormatJobPrivate(virBufferPtr buf, { qemuDomainJobPrivatePtr priv = job->privateData;
- if (job->asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT && - qemuDomainObjPrivateXMLFormatNBDMigration(buf, vm) < 0) - return -1; + if (job->asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) { + if (qemuDomainObjPrivateXMLFormatNBDMigration(buf, vm) < 0) + return -1; + + if (priv->migTempBitmaps) + qemuDomainObjPrivateXMLFormatMigrationBlockDirtyBitmapsTemp(buf, + priv->migTempBitmaps);
You could just directly call the formatting function as it won't format anything when priv->migTempBitmaps is an empty list.
+ }
if (priv->migParams) qemuMigrationParamsFormat(buf, priv->migParams); @@ -267,6 +309,45 @@ qemuDomainObjPrivateXMLParseJobNBD(virDomainObjPtr vm, return 0; }
+ +static int +qemuDomainParseJobPrivateXMLMigrationBlockDirtyBitmapsTemp(qemuDomainJobPrivatePtr jobPriv, + xmlXPathContextPtr ctxt)
qemuDomainObjPrivateXMLParseMigrateTempBitmap would make the naming a bit more consistent with other formatting and parsing functions.
+{ + g_autoslist(qemuDomainJobPrivateMigrateTempBitmap) bitmaps = NULL; + g_autofree xmlNodePtr *nodes = NULL; + size_t i; + int n; + + if ((n = virXPathNodeSet("./tempBlockDirtyBitmaps/bitmap", ctxt, &nodes)) < 0) + return -1; + + if (n == 0) + return 0; + + for (i = 0; i < n; i++) { + g_autofree char *bitmapname = virXMLPropString(nodes[i], "name"); + g_autofree char *nodename = virXMLPropString(nodes[i], "nodename"); + qemuDomainJobPrivateMigrateTempBitmapPtr bmp; + + if (!bitmapname || !nodename) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed <tempBlockDirtyBitmaps>")); + return -1; + }
Right, something similar is needed in patch 12/19. Although you could mention extend the error message here and mention the error happened in a status XML. And you could even get away without the temp variables by marking bmp for autoclean and stealing its content when adding to the list.
+ + bmp = g_new0(qemuDomainJobPrivateMigrateTempBitmap, 1); + bmp->nodename = g_steal_pointer(&nodename); + bmp->bitmapname = g_steal_pointer(&bitmapname); + + bitmaps = g_slist_prepend(bitmaps, bmp); + } + + jobPriv->migTempBitmaps = g_slist_reverse(g_steal_pointer(&bitmaps)); + return 0; +} + + static int qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt, qemuDomainJobObjPtr job, @@ -277,6 +358,9 @@ qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt, if (qemuDomainObjPrivateXMLParseJobNBD(vm, ctxt) < 0) return -1;
+ if (qemuDomainParseJobPrivateXMLMigrationBlockDirtyBitmapsTemp(priv, ctxt) < 0) + return -1; + if (qemuMigrationParamsParse(ctxt, &priv->migParams) < 0) return -1;
... No matter whether you decide to change the functions names... Reviewed-by: Jiri Denemark <jdenemar@redhat.com>