[libvirt] [PATCH v8 20/21] backup: qemu: Wire up qemu full push backup commands over QMP

[Peter Krempa]

On Wed, Apr 17, 2019 at 09:09:20 -0500, Eric Blake wrote:
> Update the code to support push backups; for now, the destination file
> still has to be local, although the XML could be extended into
> supporting remote destinations (where we will have to use the full
> power of blockdev-add). This also touches up the event handling to
> inform the user when the job is complete. (However, there are probably
> bugs lurking in the event code; pull mode is more tested than push
> mode at the time I write this).
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 81 +++++++++++++++++++++++++++++-------------
>  1 file changed, 56 insertions(+), 25 deletions(-)

This patch splitting doesn't make that much sense.

> @@ -17749,16 +17748,17 @@ qemuDomainBackupDiskCleanup(virQEMUDriverPtr driver, virDomainObjPtr vm,
>      }
>      if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_READY &&
>          qemuMonitorBlockdevDel(priv->mon, disk->store->nodeformat) < 0) {
> -        VIR_WARN("Unable to remove temp disk %s after backup",
> -                 disk->name);
> +        VIR_WARN("Unable to remove %s disk %s after backup",
> +                 push ? "target" : "scratch", disk->name);
>          ret = -1;
>      }
>      if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_LABEL)
>          qemuDomainDiskChainElementRevoke(driver, vm, disk->store);
> -    if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_CREATED &&
> +    if ((!push || !completed) &&
> +        disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_CREATED &&
>          disk->store->detected && unlink(disk->store->path) < 0) {
> -        VIR_WARN("Unable to unlink temp disk %s after backup",
> -                 disk->store->path);
> +        VIR_WARN("Unable to unlink %s disk %s after backup",
> +                 push ? "failed target" : "scratch", disk->store->path);

We tend to stay away from warnings now. Almost nobody reads that, it
doesn't notify the user right away and they are not translated.

> @@ -18126,23 +18130,50 @@ static int qemuDomainBackupEnd(virDomainPtr domain, int id, unsigned int flags)
>          goto cleanup;
>      }
> 
> -    if (priv->backup->type != VIR_DOMAIN_BACKUP_TYPE_PUSH)
> -        want_abort = false;
>      def = priv->backup;
> +    if (def->type != VIR_DOMAIN_BACKUP_TYPE_PUSH) {
> +        want_abort = false;
> +        push = false;
> +    }
> 
>      /* We are going to modify the domain below. */
>      if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>          goto cleanup;
> 
>      qemuDomainObjEnterMonitor(driver, vm);
> -    if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL)
> +    if (push) {
> +        for (i = 0; i < def->ndisks; i++) {
> +            virDomainBackupDiskDef *disk = &def->disks[i];
> +
> +            if (!disk->store)
> +                continue;
> +            if (disk->state != VIR_DOMAIN_BACKUP_DISK_STATE_COMPLETE)

I probably missed something. Where is this updated?

> +                completed = false;
> +        }
> +    } else {
>          ret = qemuMonitorNBDServerStop(priv->mon);
> -    for (i = 0; i < def->ndisks; i++) {
> -        if (qemuMonitorBlockJobCancel(priv->mon,
> -                                      def->disks[i].name) < 0 ||
> -            qemuDomainBackupDiskCleanup(driver, vm, &def->disks[i],
> -                                        !!def->incremental) < 0)
> -            ret = -1;
> +    }
> +    if (!completed && !want_abort) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("backup job id '%d' not complete yet"), id);
> +    } else {
> +        for (i = 0; i < def->ndisks; i++) {
> +            virDomainBackupDiskDef *disk = &def->disks[i];
> +
> +            if (!disk->store)
> +                continue;
> +            if (!push || disk->state < VIR_DOMAIN_BACKUP_DISK_STATE_COMPLETE) {
> +                if (qemuMonitorBlockJobCancel(priv->mon,
> +                                              disk->name) < 0 &&

There were problems with other block jobs that Cancel is possibly long
running. Does it not happen with the backup job?

> +                    !want_abort) {
> +                    ret = -1;
> +                    continue;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190426/fdd3e27c/attachment-0001.sig>