Re: [PATCH 14/19] qemu: domain: Store list of temporary bitmaps for migration in status XML

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux