While qemu is running both locations are identical in semantics, but the move will allow us to fix the scenario when the VM is destroyed or crashes where we'd leak the images. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_backup.c | 24 ++++++++++++++++++++++++ src/qemu/qemu_blockjob.c | 15 +-------------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index de4730441b..4ab9a2b17e 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -614,6 +614,7 @@ qemuBackupJobTerminate(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; + size_t i; qemuDomainJobInfoUpdateTime(priv->job.current); @@ -630,6 +631,29 @@ qemuBackupJobTerminate(virDomainObjPtr vm, qemuDomainEventEmitJobCompleted(priv->driver, vm); + if (!(priv->job.apiFlags & VIR_DOMAIN_BACKUP_BEGIN_REUSE_EXTERNAL) && + (priv->backup->type == VIR_DOMAIN_BACKUP_TYPE_PULL || + (priv->backup->type == VIR_DOMAIN_BACKUP_TYPE_PUSH && + jobstatus != QEMU_DOMAIN_JOB_STATUS_COMPLETED))) { + + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + + for (i = 0; i < priv->backup->ndisks; i++) { + virDomainBackupDiskDefPtr backupdisk = priv->backup->disks + i; + uid_t uid; + gid_t gid; + + if (!backupdisk->store || + backupdisk->store->type != VIR_STORAGE_TYPE_FILE) + continue; + + qemuDomainGetImageIds(cfg, vm, backupdisk->store, NULL, &uid, &gid); + if (virFileRemove(backupdisk->store->path, uid, gid) < 0) + VIR_WARN("failed to remove scratch file '%s'", + backupdisk->store->path); + } + } + virDomainBackupDefFree(priv->backup); priv->backup = NULL; qemuDomainObjEndAsyncJob(priv->driver, vm); diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 498e2a716f..131b53d88d 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1335,9 +1335,6 @@ qemuBlockJobProcessEventConcludedBackup(virQEMUDriverPtr driver, unsigned long long progressCurrent, unsigned long long progressTotal) { - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - uid_t uid; - gid_t gid; g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL; g_autoptr(virJSONValue) actions = NULL; @@ -1369,18 +1366,8 @@ qemuBlockJobProcessEventConcludedBackup(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) return; - if (job->data.backup.store) { + if (job->data.backup.store) qemuDomainStorageSourceAccessRevoke(driver, vm, job->data.backup.store); - - if (job->data.backup.deleteStore && - job->data.backup.store->type == VIR_STORAGE_TYPE_FILE) { - qemuDomainGetImageIds(cfg, vm, job->data.backup.store, NULL, &uid, &gid); - - if (virFileRemove(job->data.backup.store->path, uid, gid) < 0) - VIR_WARN("failed to remove scratch file '%s'", - job->data.backup.store->path); - } - } } -- 2.23.0

Now that we delete the images elsewhere it's not required. Additionally it's safe to do as we never released an upstream version which required this being in place. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_backup.c | 12 ++++-------- src/qemu/qemu_blockjob.c | 2 -- src/qemu/qemu_blockjob.h | 2 -- src/qemu/qemu_domain.c | 7 ------- tests/qemustatusxml2xmldata/backup-pull-in.xml | 1 - 5 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 4ab9a2b17e..fa9588ef28 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -298,8 +298,7 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, virJSONValuePtr actions, virDomainMomentDefPtr *incremental, virHashTablePtr blockNamedNodeData, - virQEMUDriverConfigPtr cfg, - bool removeStore) + virQEMUDriverConfigPtr cfg) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -331,7 +330,6 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, } if (!(dd->blockjob = qemuBlockJobDiskNewBackup(vm, dd->domdisk, dd->store, - removeStore, dd->incrementalBitmap))) return -1; @@ -389,13 +387,11 @@ qemuBackupDiskPrepareData(virDomainObjPtr vm, virHashTablePtr blockNamedNodeData, virJSONValuePtr actions, virQEMUDriverConfigPtr cfg, - struct qemuBackupDiskData **rdd, - bool reuse_external) + struct qemuBackupDiskData **rdd) { struct qemuBackupDiskData *disks = NULL; ssize_t ndisks = 0; size_t i; - bool removeStore = !reuse_external && (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL); disks = g_new0(struct qemuBackupDiskData, def->ndisks); @@ -410,7 +406,7 @@ qemuBackupDiskPrepareData(virDomainObjPtr vm, if (qemuBackupDiskPrepareDataOne(vm, backupdisk, dd, actions, incremental, blockNamedNodeData, - cfg, removeStore) < 0) + cfg) < 0) goto error; if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) { @@ -827,7 +823,7 @@ qemuBackupBegin(virDomainObjPtr vm, goto endjob; if ((ndd = qemuBackupDiskPrepareData(vm, def, incremental, blockNamedNodeData, - actions, cfg, &dd, reuse)) <= 0) { + actions, cfg, &dd)) <= 0) { if (ndd == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("no disks selected for backup")); diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 131b53d88d..c536c19bb6 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -382,7 +382,6 @@ qemuBlockJobDataPtr qemuBlockJobDiskNewBackup(virDomainObjPtr vm, virDomainDiskDefPtr disk, virStorageSourcePtr store, - bool deleteStore, const char *bitmap) { g_autoptr(qemuBlockJobData) job = NULL; @@ -395,7 +394,6 @@ qemuBlockJobDiskNewBackup(virDomainObjPtr vm, job->data.backup.bitmap = g_strdup(bitmap); job->data.backup.store = virObjectRef(store); - job->data.backup.deleteStore = deleteStore; /* backup jobs are usually started in bulk by transaction so the caller * shall save the status XML */ diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 42b973fe96..7d584a2980 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -113,7 +113,6 @@ typedef qemuBlockJobBackupData *qemuBlockJobDataBackupPtr; struct _qemuBlockJobBackupData { virStorageSourcePtr store; - bool deleteStore; char *bitmap; }; @@ -201,7 +200,6 @@ qemuBlockJobDataPtr qemuBlockJobDiskNewBackup(virDomainObjPtr vm, virDomainDiskDefPtr disk, virStorageSourcePtr store, - bool deleteStore, const char *bitmap); qemuBlockJobDataPtr diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0e073c25e1..d76dfe3a87 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2619,9 +2619,6 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload, data->xmlopt, false) < 0) return -1; - - if (job->data.backup.deleteStore) - virBufferAddLit(&childBuf, "<deleteStore/>\n"); } break; @@ -3224,10 +3221,6 @@ qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job, if (!(tmp = virXPathNode("./store", ctxt)) || !(job->data.backup.store = qemuDomainObjPrivateXMLParseBlockjobChain(tmp, ctxt, xmlopt))) goto broken; - - if (virXPathNode("./deleteStore", ctxt)) - job->data.backup.deleteStore = true; - break; case QEMU_BLOCKJOB_TYPE_BROKEN: diff --git a/tests/qemustatusxml2xmldata/backup-pull-in.xml b/tests/qemustatusxml2xmldata/backup-pull-in.xml index 6ef4965bed..3c69c41840 100644 --- a/tests/qemustatusxml2xmldata/backup-pull-in.xml +++ b/tests/qemustatusxml2xmldata/backup-pull-in.xml @@ -248,7 +248,6 @@ </privateData> </source> </store> - <deleteStore/> </blockjob> </blockjobs> <agentTimeout>-2</agentTimeout> -- 2.23.0

When cancelling the blockjobs as part of failed backup job startup recover we didn't pass in the correct async job type. Luckily the block job handler and cancellation code paths use no block job at all currently so those were correct. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_backup.c | 17 +++++++++++------ src/qemu/qemu_backup.h | 7 +++++-- src/qemu/qemu_blockjob.c | 3 ++- src/qemu/qemu_driver.c | 2 +- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index fa9588ef28..1381505d9f 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -661,6 +661,7 @@ qemuBackupJobTerminate(virDomainObjPtr vm, * @vm: domain object * @backup: backup definition * @terminatebackup: flag whether to terminate and unregister the backup + * @asyncJob: currently used qemu asynchronous job type * * Sends all active blockjobs which are part of @backup of @vm a signal to * cancel. If @terminatebackup is true qemuBackupJobTerminate is also called @@ -669,7 +670,8 @@ qemuBackupJobTerminate(virDomainObjPtr vm, void qemuBackupJobCancelBlockjobs(virDomainObjPtr vm, virDomainBackupDefPtr backup, - bool terminatebackup) + bool terminatebackup, + int asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; size_t i; @@ -703,7 +705,8 @@ qemuBackupJobCancelBlockjobs(virDomainObjPtr vm, if (backupdisk->state != VIR_DOMAIN_BACKUP_DISK_STATE_RUNNING) continue; - qemuDomainObjEnterMonitor(priv->driver, vm); + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return; rc = qemuMonitorJobCancel(priv->mon, job->name, false); @@ -869,7 +872,7 @@ qemuBackupBegin(virDomainObjPtr vm, goto endjob; if (rc < 0) { - qemuBackupJobCancelBlockjobs(vm, priv->backup, false); + qemuBackupJobCancelBlockjobs(vm, priv->backup, false, QEMU_ASYNC_JOB_BACKUP); goto endjob; } } @@ -920,7 +923,8 @@ qemuBackupNotifyBlockjobEnd(virDomainObjPtr vm, virDomainDiskDefPtr disk, qemuBlockjobState state, unsigned long long cur, - unsigned long long end) + unsigned long long end, + int asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; bool has_running = false; @@ -938,7 +942,8 @@ qemuBackupNotifyBlockjobEnd(virDomainObjPtr vm, return; if (backup->type == VIR_DOMAIN_BACKUP_TYPE_PULL) { - qemuDomainObjEnterMonitor(priv->driver, vm); + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return; ignore_value(qemuMonitorNBDServerStop(priv->mon)); if (qemuDomainObjExitMonitor(priv->driver, vm) < 0) return; @@ -1011,7 +1016,7 @@ qemuBackupNotifyBlockjobEnd(virDomainObjPtr vm, if (has_running && (has_failed || has_cancelled)) { /* cancel the rest of the jobs */ - qemuBackupJobCancelBlockjobs(vm, backup, false); + qemuBackupJobCancelBlockjobs(vm, backup, false, asyncJob); } else if (!has_running && !has_cancelling) { /* all sub-jobs have stopped */ diff --git a/src/qemu/qemu_backup.h b/src/qemu/qemu_backup.h index df67b849be..1b8a03612c 100644 --- a/src/qemu/qemu_backup.h +++ b/src/qemu/qemu_backup.h @@ -31,14 +31,17 @@ qemuBackupGetXMLDesc(virDomainObjPtr vm, void qemuBackupJobCancelBlockjobs(virDomainObjPtr vm, virDomainBackupDefPtr backup, - bool terminatebackup); + bool terminatebackup, + int asyncJob); void qemuBackupNotifyBlockjobEnd(virDomainObjPtr vm, virDomainDiskDefPtr disk, qemuBlockjobState state, unsigned long long cur, - unsigned long long end); + unsigned long long end, + int asyncJob); + int qemuBackupGetJobInfoStats(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index c536c19bb6..e04fcf69d1 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1336,7 +1336,8 @@ qemuBlockJobProcessEventConcludedBackup(virQEMUDriverPtr driver, g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL; g_autoptr(virJSONValue) actions = NULL; - qemuBackupNotifyBlockjobEnd(vm, job->disk, newstate, progressCurrent, progressTotal); + qemuBackupNotifyBlockjobEnd(vm, job->disk, newstate, + progressCurrent, progressTotal, asyncJob); if (job->data.backup.store && !(backend = qemuBlockStorageSourceDetachPrepare(job->data.backup.store, NULL))) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ec8faf384c..e493755fb8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14124,7 +14124,7 @@ static int qemuDomainAbortJob(virDomainPtr dom) break; case QEMU_ASYNC_JOB_BACKUP: - qemuBackupJobCancelBlockjobs(vm, priv->backup, true); + qemuBackupJobCancelBlockjobs(vm, priv->backup, true, QEMU_ASYNC_JOB_NONE); ret = 0; break; -- 2.23.0

Commit d75f865fb989b3e6330c78c28e1c3bf7fa28e6a5 caused a job-deadlock if a VM is running the backup job and being destroyed as it removed the cleanup of the async job type and there was nothing to clean up the backup job. Add an explicit cleanup of the backup job when destroying a VM. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_backup.c | 2 +- src/qemu/qemu_backup.h | 3 +++ src/qemu/qemu_process.c | 5 +++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 1381505d9f..3bac6b353c 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -604,7 +604,7 @@ qemuBackupBeginCollectIncrementalCheckpoints(virDomainObjPtr vm, } -static void +void qemuBackupJobTerminate(virDomainObjPtr vm, qemuDomainJobStatus jobstatus) diff --git a/src/qemu/qemu_backup.h b/src/qemu/qemu_backup.h index 1b8a03612c..3321ba0b6f 100644 --- a/src/qemu/qemu_backup.h +++ b/src/qemu/qemu_backup.h @@ -42,6 +42,9 @@ qemuBackupNotifyBlockjobEnd(virDomainObjPtr vm, unsigned long long end, int asyncJob); +void +qemuBackupJobTerminate(virDomainObjPtr vm, + qemuDomainJobStatus jobstatus); int qemuBackupGetJobInfoStats(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e6c6c0bee2..3b036ac4bc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -57,6 +57,7 @@ #include "qemu_security.h" #include "qemu_extdevice.h" #include "qemu_firmware.h" +#include "qemu_backup.h" #include "cpu/cpu.h" #include "cpu/cpu_x86.h" @@ -7609,6 +7610,10 @@ void qemuProcessStop(virQEMUDriverPtr driver, virResctrlAllocRemove(vm->def->resctrls[i]->alloc); } + /* clean up a possible backup job */ + if (priv->backup) + qemuBackupJobTerminate(vm, QEMU_DOMAIN_JOB_STATUS_CANCELED); + qemuProcessRemoveDomainStatus(driver, vm); /* Remove VNC and Spice ports from port reservation bitmap, but only if -- 2.23.0

One of the first versions thought of using disk path as the second option but this was dropped as being a legacy interface. Remove the leftover pointless <choice> wrapper for the disk name as there's just one option now. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- docs/schemas/domainbackup.rng | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/docs/schemas/domainbackup.rng b/docs/schemas/domainbackup.rng index 7286acb18c..c1e4d80302 100644 --- a/docs/schemas/domainbackup.rng +++ b/docs/schemas/domainbackup.rng @@ -104,9 +104,7 @@ <oneOrMore> <element name='disk'> <attribute name='name'> - <choice> - <ref name='diskTarget'/> - </choice> + <ref name='diskTarget'/> </attribute> <choice> <group> @@ -165,9 +163,7 @@ <oneOrMore> <element name='disk'> <attribute name='name'> - <choice> - <ref name='diskTarget'/> - </choice> + <ref name='diskTarget'/> </attribute> <choice> <group> -- 2.23.0

If users wish to use different name for exported disks or bitmaps the new fields allow to do so. Additionally they also document the current settings. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- docs/formatbackup.html.in | 9 +++++++++ docs/schemas/domainbackup.rng | 8 ++++++++ src/conf/backup_conf.c | 11 +++++++++++ src/conf/backup_conf.h | 2 ++ tests/domainbackupxml2xmlin/backup-pull-seclabel.xml | 2 +- tests/domainbackupxml2xmlout/backup-pull-seclabel.xml | 2 +- 6 files changed, 32 insertions(+), 2 deletions(-) diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in index 1c690901c7..7d2c6f1599 100644 --- a/docs/formatbackup.html.in +++ b/docs/formatbackup.html.in @@ -85,6 +85,15 @@ <dd>Setting this attribute to <code>yes</code>(default) specifies that the disk should take part in the backup and using <code>no</code> excludes the disk from the backup.</dd> + <dt><code>exportname</code></dt> + <dd>Allows to modify the NBD export name for the given disk. + By default equal to disk target. + Valid only for pull mode backups. </dd> + <dt><code>exportbitmap</code></dt> + <dd>Allows to modify the name of the bitmap describing dirty + blocks for an incremental backup exported via NBD export name + for the given disk. + Valid only for pull mode backups. </dd> <dt><code>type</code></dt> <dd>A mandatory attribute to describe the type of the disk, except when <code>backup='no'</code> is diff --git a/docs/schemas/domainbackup.rng b/docs/schemas/domainbackup.rng index c1e4d80302..395ea841f9 100644 --- a/docs/schemas/domainbackup.rng +++ b/docs/schemas/domainbackup.rng @@ -165,6 +165,14 @@ <attribute name='name'> <ref name='diskTarget'/> </attribute> + <optional> + <attribute name='exportname'> + <text/> + </attribute> + <attribute name='exportbitmap'> + <text/> + </attribute> + </optional> <choice> <group> <attribute name='backup'> diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index aa11967d2a..a4b87baa55 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -71,6 +71,8 @@ virDomainBackupDefFree(virDomainBackupDefPtr def) virDomainBackupDiskDefPtr disk = def->disks + i; g_free(disk->name); + g_free(disk->exportname); + g_free(disk->exportbitmap); virObjectUnref(disk->store); } @@ -124,6 +126,11 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, if (def->backup == VIR_TRISTATE_BOOL_NO) return 0; + if (!push) { + def->exportname = virXMLPropString(node, "exportname"); + def->exportbitmap = virXMLPropString(node, "exportbitmap"); + } + if (internal) { if (!(state = virXMLPropString(node, "state")) || (tmp = virDomainBackupDiskStateTypeFromString(state)) < 0) { @@ -165,6 +172,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, storageSourceParseFlags, xmlopt) < 0) return -1; + if ((driver = virXPathString("string(./driver/@type)", ctxt))) { def->store->format = virStorageFileFormatTypeFromString(driver); if (def->store->format <= 0) { @@ -333,6 +341,9 @@ virDomainBackupDiskDefFormat(virBufferPtr buf, if (disk->backup == VIR_TRISTATE_BOOL_YES) { virBufferAsprintf(&attrBuf, " type='%s'", virStorageTypeToString(disk->store->type)); + virBufferEscapeString(&attrBuf, " exportname='%s'", disk->exportname); + virBufferEscapeString(&attrBuf, " exportbitmap='%s'", disk->exportbitmap); + if (disk->store->format > 0) virBufferEscapeString(&childBuf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->store->format)); diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h index 7cf44245d4..672fd52ee7 100644 --- a/src/conf/backup_conf.h +++ b/src/conf/backup_conf.h @@ -51,6 +51,8 @@ typedef virDomainBackupDiskDef *virDomainBackupDiskDefPtr; struct _virDomainBackupDiskDef { char *name; /* name matching the <target dev='...' of the domain */ virTristateBool backup; /* whether backup is requested */ + char *exportname; /* name of the NBD export for pull mode backup */ + char *exportbitmap; /* name of the bitmap exposed in NBD for pull mode backup */ /* details of target for push-mode, or of the scratch file for pull-mode */ virStorageSourcePtr store; diff --git a/tests/domainbackupxml2xmlin/backup-pull-seclabel.xml b/tests/domainbackupxml2xmlin/backup-pull-seclabel.xml index a00d8758bb..4e6e602c19 100644 --- a/tests/domainbackupxml2xmlin/backup-pull-seclabel.xml +++ b/tests/domainbackupxml2xmlin/backup-pull-seclabel.xml @@ -2,7 +2,7 @@ <incremental>1525889631</incremental> <server transport='tcp' name='localhost' port='10809'/> <disks> - <disk name='vda' type='file'> + <disk name='vda' type='file' exportname='test-vda' exportbitmap='blah'> <driver type='qcow2'/> <scratch file='/path/to/file'> <seclabel model='dac' relabel='no'/> diff --git a/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml b/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml index c631c9b979..450f007d3a 100644 --- a/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml +++ b/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml @@ -2,7 +2,7 @@ <incremental>1525889631</incremental> <server transport='tcp' name='localhost' port='10809'/> <disks> - <disk name='vda' backup='yes' type='file'> + <disk name='vda' backup='yes' type='file' exportname='test-vda' exportbitmap='blah'> <driver type='qcow2'/> <scratch file='/path/to/file'> <seclabel model='dac' relabel='no'/> -- 2.23.0

On Fri, Dec 20, 2019 at 3:27 PM Peter Krempa <pkrempa(a)redhat.com&gt; wrote: Makes sense. We assumed that the name is same as the drive name (e.g. "sda"), but being able to set this name or get it from the backup xml returned by backupGetXMLDesc() is more robust. Currently oVirt generates the NBD URL after starting a backup, assuming the drive name. I think with this we will would call backupGetXMLDesc(), parse it and extract the exportname. Currently we discover this name using NBD_OPT_LIST_META_CONTEXT but being able to set or get it from the xml is better. I think the text should mention that this name must be unique, so you cannot use the same name in case you backup multiple disks. We would probably use the drive name as well for this, so it will be available as "qemu:dirty-bitmap:sda". Or to ensure it can never clash with another bitmap, <backup-uuid>-<drive-name> (7ce9b65d-b832-4ff2-b100-5170c9d02e2a-sda). It would be nice to show these parameters in the examples at the bottom of the page, in particualr in an example output of backupGetXMLDesc(), where this value will always appear. Are there any limits on the content and size of these strings? Nir

On Fri, Dec 20, 2019 at 3:27 PM Peter Krempa <pkrempa(a)redhat.com&gt; wrote: It is a little strange that we set dd->backupdisk->exportname here, just before we call qemu. I think it would be cleaner if we configure dd-backupdisk-exportname after parsing the xml, or never, sending the value from dd->domdisk->dst. Otherwise looks fine.