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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
