[PATCH 0/8] qemu: Improve nodename lookup and format index for backup disks
Peter Krempa (8): qemuDomainDiskLookupByNodename: Simplify node name lookup qemuDomainGetStorageSourceByDevstr: Use virDomainDiskByTarget qemuDomainGetStorageSourceByDevstr: Avoid logged errors backup: Move file format check from parser to qemu driver virDomainBackupDiskDefParseXML: Use virDomainStorageSourceParseBase qemuDomainDiskLookupByNodename: Lookup also backup 'store' nodenames qemuDomainGetStorageSourceByDevstr: Lookup also backup 'store' nodenames conf: backup: Format index of 'store' docs/formatbackup.rst | 4 + src/conf/backup_conf.c | 46 ++++------- src/qemu/qemu_backup.c | 10 ++- src/qemu/qemu_domain.c | 78 ++++++++++++------- src/qemu/qemu_domain.h | 4 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 7 +- .../qemustatusxml2xmldata/backup-pull-in.xml | 2 +- 8 files changed, 89 insertions(+), 64 deletions(-) -- 2.28.0
Use dummy variable to fill 'src' so that access to it doesn't need to be conditionalized and use temporary variable for 'disk' rather than dereferencing the array multiple times. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2275c83aa8..f5d4e468ce 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9911,24 +9911,18 @@ qemuDomainDiskLookupByNodename(virDomainDefPtr def, size_t i; virStorageSourcePtr tmp = NULL; - if (src) - *src = NULL; + if (!src) + src = &tmp; for (i = 0; i < def->ndisks; i++) { - if ((tmp = virStorageSourceFindByNodeName(def->disks[i]->src, nodename))) { - if (src) - *src = tmp; + virDomainDiskDefPtr domdisk = def->disks[i]; - return def->disks[i]; - } - - if (def->disks[i]->mirror && - (tmp = virStorageSourceFindByNodeName(def->disks[i]->mirror, nodename))) { - if (src) - *src = tmp; + if ((*src = virStorageSourceFindByNodeName(domdisk->src, nodename))) + return domdisk; - return def->disks[i]; - } + if (domdisk->mirror && + (*src = virStorageSourceFindByNodeName(domdisk->mirror, nodename))) + return domdisk; } return NULL; -- 2.28.0
The function replaces the open-coded block. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f5d4e468ce..e56351333c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9954,7 +9954,6 @@ qemuDomainGetStorageSourceByDevstr(const char *devstr, virStorageSourcePtr src = NULL; g_autofree char *target = NULL; unsigned int idx; - size_t i; if (virStorageFileParseBackingStoreStr(devstr, &target, &idx) < 0) { virReportError(VIR_ERR_INVALID_ARG, @@ -9962,14 +9961,7 @@ qemuDomainGetStorageSourceByDevstr(const char *devstr, return NULL; } - for (i = 0; i < def->ndisks; i++) { - if (STREQ(target, def->disks[i]->dst)) { - disk = def->disks[i]; - break; - } - } - - if (!disk) { + if (!(disk = virDomainDiskByTarget(def, target))) { virReportError(VIR_ERR_INVALID_ARG, _("failed to find disk '%s'"), target); return NULL; -- 2.28.0
'virStorageFileChainLookup' reports an error when the lookup of the backing chain entry is unsuccessful. Since we possibly use it multiple times when looking up backing for 'disk->mirror' the function can report error which won't be actually reported. Replace the call to virStorageFileChainLookup by lookup in the chain by index. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e56351333c..44abe0ce93 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9951,7 +9951,7 @@ qemuDomainGetStorageSourceByDevstr(const char *devstr, virDomainDefPtr def) { virDomainDiskDefPtr disk = NULL; - virStorageSourcePtr src = NULL; + virStorageSourcePtr n; g_autofree char *target = NULL; unsigned int idx; @@ -9970,13 +9970,20 @@ qemuDomainGetStorageSourceByDevstr(const char *devstr, if (idx == 0) return disk->src; - if ((src = virStorageFileChainLookup(disk->src, NULL, NULL, idx, NULL))) - return src; + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (n->id == idx) + return n; + } - if (disk->mirror && - (src = virStorageFileChainLookup(disk->mirror, NULL, NULL, idx, NULL))) - return src; + if (disk->mirror) { + for (n = disk->mirror; virStorageSourceIsBacking(n); n = n->backingStore) { + if (n->id == idx) + return n; + } + } + virReportError(VIR_ERR_INVALID_ARG, + _("failed to find disk '%s'"), devstr); return NULL; } -- 2.28.0
It's a technical detail in qemu that QCOW2 is needed for a pull-mode backup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/backup_conf.c | 5 ----- src/qemu/qemu_backup.c | 10 +++++++++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index ea812cc432..47e3bc1d60 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -204,11 +204,6 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk backup driver '%s'"), driver); return -1; - } else if (!push && def->store->format != VIR_STORAGE_FILE_QCOW2) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("pull mode requires qcow2 driver, not '%s'"), - driver); - return -1; } } diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 09f7921ea7..b2340eb1cf 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -266,8 +266,16 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, if (!qemuDomainDiskBlockJobIsSupported(vm, dd->domdisk)) return -1; - if (!dd->store->format) + if (dd->store->format == VIR_STORAGE_FILE_NONE) { dd->store->format = VIR_STORAGE_FILE_QCOW2; + } else if (dd->store->format != VIR_STORAGE_FILE_QCOW2) { + if (pull) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("pull mode backup for disk '%s' requires qcow2 driver"), + dd->backupdisk->name); + return -1; + } + } /* calculate backing store to use: * push mode: -- 2.28.0
Don't duplicate code to parse the virStorageSource basics. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/backup_conf.c | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index 47e3bc1d60..11d419ce2b 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -104,7 +104,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, { VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autofree char *type = NULL; - g_autofree char *driver = NULL; + g_autofree char *format = NULL; g_autofree char *backup = NULL; g_autofree char *state = NULL; g_autofree char *backupmode = NULL; @@ -169,23 +169,17 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, def->state = tmp; } - def->store = virStorageSourceNew(); + type = virXMLPropString(node, "type"); + format = virXPathString("string(./driver/@type)", ctxt); - if ((type = virXMLPropString(node, "type"))) { - if ((def->store->type = virStorageTypeFromString(type)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown disk backup type '%s'"), type); - return -1; - } + if (!(def->store = virDomainStorageSourceParseBase(type, format, NULL))) + return -1; - if (def->store->type != VIR_STORAGE_TYPE_FILE && - def->store->type != VIR_STORAGE_TYPE_BLOCK) { - virReportError(VIR_ERR_XML_ERROR, - _("unsupported disk backup type '%s'"), type); - return -1; - } - } else { - def->store->type = VIR_STORAGE_TYPE_FILE; + if (def->store->type != VIR_STORAGE_TYPE_FILE && + def->store->type != VIR_STORAGE_TYPE_BLOCK) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported disk backup type '%s'"), type); + return -1; } if (push) @@ -198,15 +192,6 @@ 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) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk backup driver '%s'"), driver); - return -1; - } - } - return 0; } -- 2.28.0
Nodename may be asociated to a disk backup job, add support to looking up in that chain too. This is specifically useful for the BLOCK_WRITE_THRESHOLD event which can be registered for any nodename. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++++++++++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_process.c | 7 +++++-- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 44abe0ce93..7232b6131f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9896,6 +9896,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, /** * qemuDomainDiskLookupByNodename: * @def: domain definition to look for the disk + * @backupdef: definition of the backup job of the domain (optional) * @nodename: block backend node name to find * @src: filled with the specific backing store element if provided * @@ -9905,6 +9906,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, */ virDomainDiskDefPtr qemuDomainDiskLookupByNodename(virDomainDefPtr def, + virDomainBackupDefPtr backupdef, const char *nodename, virStorageSourcePtr *src) { @@ -9925,6 +9927,16 @@ qemuDomainDiskLookupByNodename(virDomainDefPtr def, return domdisk; } + if (backupdef) { + for (i = 0; i < backupdef->ndisks; i++) { + virDomainBackupDiskDefPtr backupdisk = backupdef->disks + i; + + if (backupdisk->store && + (*src = virStorageSourceFindByNodeName(backupdisk->store, nodename))) + return virDomainDiskByTarget(def, backupdisk->name); + } + } + return NULL; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9e870ff1e2..32f3586882 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -912,6 +912,7 @@ int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, int *perms); virDomainDiskDefPtr qemuDomainDiskLookupByNodename(virDomainDefPtr def, + virDomainBackupDefPtr backupdef, const char *nodename, virStorageSourcePtr *src); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9d83825190..db77d8f1de 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -879,7 +879,7 @@ qemuProcessHandleIOError(qemuMonitorPtr mon G_GNUC_UNUSED, if (diskAlias) disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, diskAlias, NULL); else if (nodename) - disk = qemuDomainDiskLookupByNodename(vm->def, nodename, NULL); + disk = qemuDomainDiskLookupByNodename(vm->def, NULL, nodename, NULL); else disk = NULL; @@ -1483,6 +1483,7 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, unsigned long long excess, void *opaque) { + qemuDomainObjPrivatePtr priv; virQEMUDriverPtr driver = opaque; virObjectEventPtr eventSource = NULL; virObjectEventPtr eventDevice = NULL; @@ -1492,11 +1493,13 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, virObjectLock(vm); + priv = vm->privateData; + VIR_DEBUG("BLOCK_WRITE_THRESHOLD event for block node '%s' in domain %p %s:" "threshold '%llu' exceeded by '%llu'", nodename, vm, vm->def->name, threshold, excess); - if ((disk = qemuDomainDiskLookupByNodename(vm->def, nodename, &src))) { + if ((disk = qemuDomainDiskLookupByNodename(vm->def, priv->backup, nodename, &src))) { if (virStorageSourceIsLocalStorage(src)) path = src->path; -- 2.28.0
Nodename may be asociated to a disk backup job, add support to looking up in that chain too. This is specifically useful for the BLOCK_WRITE_THRESHOLD event which can be registered for any nodename. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 19 ++++++++++++++++++- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_driver.c | 2 +- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7232b6131f..58119bee1a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9960,7 +9960,8 @@ qemuDomainDiskBackingStoreGetName(virDomainDiskDefPtr disk, virStorageSourcePtr qemuDomainGetStorageSourceByDevstr(const char *devstr, - virDomainDefPtr def) + virDomainDefPtr def, + virDomainBackupDefPtr backupdef) { virDomainDiskDefPtr disk = NULL; virStorageSourcePtr n; @@ -9994,6 +9995,22 @@ qemuDomainGetStorageSourceByDevstr(const char *devstr, } } + if (backupdef) { + size_t i; + + for (i = 0; i < backupdef->ndisks; i++) { + virDomainBackupDiskDefPtr backupdisk = backupdef->disks + i; + + if (STRNEQ(target, backupdisk->name)) + continue; + + for (n = backupdisk->store; virStorageSourceIsBacking(n); n = n->backingStore) { + if (n->id == idx) + return n; + } + } + } + virReportError(VIR_ERR_INVALID_ARG, _("failed to find disk '%s'"), devstr); return NULL; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 32f3586882..154339ef8f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -920,7 +920,8 @@ char *qemuDomainDiskBackingStoreGetName(virDomainDiskDefPtr disk, unsigned int idx); virStorageSourcePtr qemuDomainGetStorageSourceByDevstr(const char *devstr, - virDomainDefPtr def); + virDomainDefPtr def, + virDomainBackupDefPtr backupdef); int qemuDomainUpdateCPU(virDomainObjPtr vm, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4fd70ed300..f8eb575628 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19575,7 +19575,7 @@ qemuDomainSetBlockThreshold(virDomainPtr dom, goto endjob; } - if (!(src = qemuDomainGetStorageSourceByDevstr(dev, vm->def))) + if (!(src = qemuDomainGetStorageSourceByDevstr(dev, vm->def, priv->backup))) goto endjob; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV) && -- 2.28.0
Similarly to other disk-related stuff, the index is useful when you want to refer to the image in APIs such as virDomainSetBlockThreshold. For internal use we also need to parse it inside of the status XML. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatbackup.rst | 4 ++++ src/conf/backup_conf.c | 8 +++++++- tests/qemustatusxml2xmldata/backup-pull-in.xml | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst index 1b9e6ebb22..c378ad9d9a 100644 --- a/docs/formatbackup.rst +++ b/docs/formatbackup.rst @@ -95,6 +95,10 @@ were supplied). The following child elements and attributes are supported: Similar to a disk declaration for a domain, the choice of type controls what additional sub-elements are needed to describe the destination. + ``index`` + Output only. The value can be used to refer to the scratch or output + file of the backup in APIs such as ``virDomainSetBlockThreshold``. + ``target`` Valid only for push mode backups, this is the primary sub-element that describes the file name of the backup destination, similar to the diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index 11d419ce2b..90ffcc51d1 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -105,6 +105,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autofree char *type = NULL; g_autofree char *format = NULL; + g_autofree char *idx = NULL; g_autofree char *backup = NULL; g_autofree char *state = NULL; g_autofree char *backupmode = NULL; @@ -171,8 +172,10 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node, type = virXMLPropString(node, "type"); format = virXPathString("string(./driver/@type)", ctxt); + if (internal) + idx = virXMLPropString(node, "index"); - if (!(def->store = virDomainStorageSourceParseBase(type, format, NULL))) + if (!(def->store = virDomainStorageSourceParseBase(type, format, idx))) return -1; if (def->store->type != VIR_STORAGE_TYPE_FILE && @@ -386,6 +389,9 @@ virDomainBackupDiskDefFormat(virBufferPtr buf, virBufferEscapeString(&attrBuf, " exportname='%s'", disk->exportname); virBufferEscapeString(&attrBuf, " exportbitmap='%s'", disk->exportbitmap); + if (disk->store->id != 0) + virBufferAsprintf(&attrBuf, " index='%u'", disk->store->id); + if (disk->store->format > 0) virBufferEscapeString(&childBuf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->store->format)); diff --git a/tests/qemustatusxml2xmldata/backup-pull-in.xml b/tests/qemustatusxml2xmldata/backup-pull-in.xml index faaed67e38..bb322f94d3 100644 --- a/tests/qemustatusxml2xmldata/backup-pull-in.xml +++ b/tests/qemustatusxml2xmldata/backup-pull-in.xml @@ -256,7 +256,7 @@ <incremental>12345</incremental> <server transport='tcp' tls='yes' name='localhost' port='10809'/> <disks> - <disk name='vda' backup='yes' state='running' type='file'> + <disk name='vda' backup='yes' state='running' type='file' index='123'> <scratch file='/path/to/file/'/> </disk> </disks> -- 2.28.0
On a Monday in 2020, Peter Krempa wrote:
Peter Krempa (8): qemuDomainDiskLookupByNodename: Simplify node name lookup qemuDomainGetStorageSourceByDevstr: Use virDomainDiskByTarget qemuDomainGetStorageSourceByDevstr: Avoid logged errors backup: Move file format check from parser to qemu driver virDomainBackupDiskDefParseXML: Use virDomainStorageSourceParseBase qemuDomainDiskLookupByNodename: Lookup also backup 'store' nodenames qemuDomainGetStorageSourceByDevstr: Lookup also backup 'store' nodenames conf: backup: Format index of 'store'
docs/formatbackup.rst | 4 + src/conf/backup_conf.c | 46 ++++------- src/qemu/qemu_backup.c | 10 ++- src/qemu/qemu_domain.c | 78 ++++++++++++------- src/qemu/qemu_domain.h | 4 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 7 +- .../qemustatusxml2xmldata/backup-pull-in.xml | 2 +- 8 files changed, 89 insertions(+), 64 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa