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@xxxxxxxxxx>
> ---
> 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;
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
