Re: [PATCH v8 15/21] backup: Add new qemu monitor interactions

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

 



On Wed, Apr 17, 2019 at 09:09:15 -0500, Eric Blake wrote:
> Add some monitor commands to be used during backup/checkpoint
> operations:
> - another facet to query-block for learning bitmap size
> - new export and bitmap parameters to nbd-server-add
> - new block-dirty-bitmap-{add,enable,disable,merge} functions
> 
> Also add two capabilities for testing that they are supported;
> using block-dirty-bitmap-merge as the generic witness of checkpoint
> support (since all of the functionalities were added in the same
> qemu 4.0 release), and the bitmap parameter to nbd-server-add for
> pull-mode backup support.  Even though both capabilities are
> likely to be present or absent together (that is, it is unlikely
> to encounter a qemu that backports only one of the two), it still
> makes sense to keep two capabilities as the two uses are
> orthogonal (full backups don't require checkpoints, push mode
> backups don't require NBD bitmap support, and checkpoints can
> be used for more than just incremental backups).

I'd suggest you split the capabilities, new QMP APIs, changes to
existing QMP apis and block job handling changes each into a separate
commit.

> 
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>  src/qemu/qemu_capabilities.h                  |   2 +
>  src/qemu/qemu_monitor.h                       |  21 +-
>  src/qemu/qemu_monitor_json.h                  |  19 +-
>  src/qemu/qemu_capabilities.c                  |   4 +
>  src/qemu/qemu_migration.c                     |   2 +-
>  src/qemu/qemu_monitor.c                       |  65 +++++-
>  src/qemu/qemu_monitor_json.c                  | 199 +++++++++++++++++-
>  .../caps_4.0.0.riscv32.xml                    |   2 +
>  .../caps_4.0.0.riscv64.xml                    |   2 +
>  .../caps_4.0.0.x86_64.xml                     |   2 +
>  tests/qemumonitorjsontest.c                   |   2 +-
>  11 files changed, 312 insertions(+), 8 deletions(-)

[...]

> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 1237c14a84..aaf8f1df26 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h

[...]

> @@ -646,6 +650,19 @@ int qemuMonitorSetBalloon(qemuMonitorPtr mon,
>                            unsigned long long newmem);
>  int qemuMonitorSetCPU(qemuMonitorPtr mon, int cpu, bool online);
> 
> +int qemuMonitorAddBitmap(qemuMonitorPtr mon, const char *node,
> +                         const char *bitmap, bool persistent)

Usually it's one argument per line.

> +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +int qemuMonitorEnableBitmap(qemuMonitorPtr mon, const char *node,
> +                            const char *bitmap)
> +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +int qemuMonitorMergeBitmaps(qemuMonitorPtr mon, const char *node,
> +                            const char *dst, virJSONValuePtr *src)

[...]

> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 694ed90622..9b4a5c01ab 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c

[...]

> @@ -4471,3 +4486,47 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon,
>      virHashFree(info);
>      return ret;
>  }
> +
> +int
> +qemuMonitorAddBitmap(qemuMonitorPtr mon, const char *node,
> +                     const char *bitmap, bool persistent)

One argument per line.

> +{
> +    VIR_DEBUG("node=%s bitmap=%s persistent=%d", node, bitmap, persistent);

Should 'node' be declared as NONNULL? it's used without  NULLSTR here.

> +
> +    QEMU_CHECK_MONITOR(mon);
> +
> +    return qemuMonitorJSONAddBitmap(mon, node, bitmap, persistent);
> +}
> +
> +int
> +qemuMonitorEnableBitmap(qemuMonitorPtr mon, const char *node,
> +                        const char *bitmap)
> +{
> +    VIR_DEBUG("node=%s bitmap=%s", node, bitmap);
> +
> +    QEMU_CHECK_MONITOR(mon);
> +
> +    return qemuMonitorJSONEnableBitmap(mon, node, bitmap);
> +}
> +
> +int
> +qemuMonitorMergeBitmaps(qemuMonitorPtr mon, const char *node,
> +                        const char *dst, virJSONValuePtr *src)
> +{
> +    VIR_DEBUG("node=%s dst=%s src=%p", node, dst, *src);

Logging 'src' as pointer does not have much value. I'd just drop it as
it does not have a simple way to access it.

> +
> +    QEMU_CHECK_MONITOR(mon);
> +
> +    return qemuMonitorJSONMergeBitmaps(mon, node, dst, src);
> +}
> +
> +int
> +qemuMonitorDeleteBitmap(qemuMonitorPtr mon, const char *node,
> +                        const char *bitmap)
> +{
> +    VIR_DEBUG("node=%s bitmap=%s", node, bitmap);
> +
> +    QEMU_CHECK_MONITOR(mon);
> +
> +    return qemuMonitorJSONDeleteBitmap(mon, node, bitmap);
> +}
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index fc2d193ae2..801433826f 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1011,6 +1011,8 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
>          type = VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT;
>      else if (STREQ(type_str, "mirror"))
>          type = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
> +    else if (STREQ(type_str, "backup"))
> +        type = VIR_DOMAIN_BLOCK_JOB_TYPE_BACKUP;

As I've pointed out in previous responses, we need to clarify whether
the backup job is a blockjob, general job or we need a new job API in
libvirt.

> 
>      switch ((virConnectDomainEventBlockJobStatus) event) {
>      case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
> @@ -2755,6 +2757,82 @@ int qemuMonitorJSONBlockResize(qemuMonitorPtr mon,
>      return ret;
>  }
> 
> +int qemuMonitorJSONUpdateCheckpointSize(qemuMonitorPtr mon,
> +                                        virDomainCheckpointDefPtr chk)
> +{
> +    int ret = -1;
> +    size_t i, j;
> +    virJSONValuePtr devices;
> +
> +    if (!(devices = qemuMonitorJSONQueryBlock(mon)))
> +        return -1;
> +
> +    for (i = 0; i < virJSONValueArraySize(devices); i++) {
> +        virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
> +        virJSONValuePtr inserted;
> +        virJSONValuePtr bitmaps = NULL;
> +        const char *node;
> +        virDomainCheckpointDiskDefPtr disk;
> +
> +        if (!(dev = qemuMonitorJSONGetBlockDev(devices, i)))
> +            goto cleanup;
> +
> +        if (!(inserted = virJSONValueObjectGetObject(dev, "inserted")))
> +            continue;
> +        if (!(node = virJSONValueObjectGetString(inserted, "node-name"))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("query-block device entry was not in expected format"));
> +            goto cleanup;
> +        }
> +
> +        for (j = 0; j < chk->ndisks; j++) {
> +            disk = &chk->disks[j];
> +            if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
> +                continue;
> +            if (STREQ(chk->common.dom->disks[disk->idx]->src->nodeformat, node))

This will access stale data after you unplug a disk. Caching disk
indexes is a bad idea.

> +                break;
> +        }
> +        if (j == chk->ndisks) {
> +            VIR_DEBUG("query-block did not find node %s", node);
> +            continue;
> +        }
> +        if (!(bitmaps = virJSONValueObjectGetArray(dev, "dirty-bitmaps"))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("disk %s dirty bitmaps missing"), disk->name);
> +            goto cleanup;
> +        }
> +        for (j = 0; j < virJSONValueArraySize(bitmaps); j++) {
> +            virJSONValuePtr map = virJSONValueArrayGet(bitmaps, j);
> +            const char *name;
> +
> +            if (!(name = virJSONValueObjectGetString(map, "name"))) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("dirty bitmaps entry was not in expected format"));
> +                goto cleanup;
> +            }
> +            if (STRNEQ(name, disk->bitmap))
> +                continue;
> +            if (virJSONValueObjectGetNumberUlong(map, "count", &disk->size) < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("invalid bitmap count"));
> +                goto cleanup;
> +            }
> +            break;
> +        }
> +        if (j == virJSONValueArraySize(bitmaps)) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("disk %s dirty bitmap info missing"), disk->name);
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    virJSONValueFree(devices);
> +    return ret;
> +}
> +
> 
>  int qemuMonitorJSONSetPassword(qemuMonitorPtr mon,
>                                 const char *protocol,
> @@ -4706,6 +4784,8 @@ qemuMonitorJSONParseBlockJobInfo(virHashTablePtr blockJobs,
>          info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT;
>      else if (STREQ(type, "mirror"))
>          info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
> +    else if (STREQ(type, "backup"))
> +        info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_BACKUP;
>      else
>          info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
> 
[...]

Attachment: signature.asc
Description: PGP signature

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

[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