The formatter for the backup job data didn't pass the virDomainXMLOption struct to the disk formatter which meant that the private data of the disk source were not formatted. This didn't pose a problem for now as the blockjob list remembered the nodenames for the jobs, but the backup source lost them. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/backup_conf.c | 10 ++++++---- src/conf/backup_conf.h | 3 ++- src/qemu/qemu_backup.c | 4 +++- src/qemu/qemu_domain.c | 2 +- tests/genericxml2xmltest.c | 2 +- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index 694553a544..2a7fa95e0c 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -318,7 +318,8 @@ static int virDomainBackupDiskDefFormat(virBuffer *buf, virDomainBackupDiskDef *disk, bool push, - bool internal) + bool internal, + virDomainXMLOption *xmlopt) { g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -358,7 +359,7 @@ virDomainBackupDiskDefFormat(virBuffer *buf, if (virDomainDiskSourceFormat(&childBuf, disk->store, sourcename, 0, false, storageSourceFormatFlags, - false, false, NULL) < 0) + false, false, xmlopt) < 0) return -1; } @@ -390,7 +391,8 @@ virDomainBackupDefFormatPrivate(virBuffer *buf, int virDomainBackupDefFormat(virBuffer *buf, virDomainBackupDef *def, - bool internal) + bool internal, + virDomainXMLOption *xmlopt) { g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -418,7 +420,7 @@ virDomainBackupDefFormat(virBuffer *buf, for (i = 0; i < def->ndisks; i++) { if (virDomainBackupDiskDefFormat(&disksChildBuf, &def->disks[i], def->type == VIR_DOMAIN_BACKUP_TYPE_PUSH, - internal) < 0) + internal, xmlopt) < 0) return -1; } diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h index b682da1c95..dc66b75892 100644 --- a/src/conf/backup_conf.h +++ b/src/conf/backup_conf.h @@ -123,7 +123,8 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainBackupDef, virDomainBackupDefFree); int virDomainBackupDefFormat(virBuffer *buf, virDomainBackupDef *def, - bool internal); + bool internal, + virDomainXMLOption *xmlopt); int virDomainBackupAlignDisks(virDomainBackupDef *backup, virDomainDef *dom, diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index abbfcf3682..9fa8d2f02e 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -954,6 +954,8 @@ char * qemuBackupGetXMLDesc(virDomainObj *vm, unsigned int flags) { + qemuDomainObjPrivate *priv = vm->privateData; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virDomainBackupDef *backup; @@ -962,7 +964,7 @@ qemuBackupGetXMLDesc(virDomainObj *vm, if (!(backup = qemuDomainGetBackup(vm))) return NULL; - if (virDomainBackupDefFormat(&buf, backup, false) < 0) + if (virDomainBackupDefFormat(&buf, backup, false, priv->driver->xmlopt) < 0) return NULL; return virBufferContentAndReset(&buf); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 209337404a..fb203bc830 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2152,7 +2152,7 @@ qemuDomainObjPrivateXMLFormatBackups(virBuffer *buf, g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); if (priv->backup && - virDomainBackupDefFormat(&childBuf, priv->backup, true) < 0) + virDomainBackupDefFormat(&childBuf, priv->backup, true, priv->driver->xmlopt) < 0) return -1; virXMLFormatElement(buf, "backups", &attrBuf, &childBuf); diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index a6f974e758..34ccaff615 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -114,7 +114,7 @@ testCompareBackupXML(const void *opaque) return -1; } - if (virDomainBackupDefFormat(&buf, backup, data->internal) < 0) { + if (virDomainBackupDefFormat(&buf, backup, data->internal, NULL) < 0) { VIR_TEST_VERBOSE("failed to format backup def '%s'", file_in); return -1; } -- 2.31.1

On Mon, Nov 1, 2021 at 4:48 PM Peter Krempa <pkrempa(a)redhat.com&gt; wrote: Shouldn't this be libvirt-123-*, matching the index=? Since we have the index entry, and libvirt knows that it has libvirt-N-storage and libvirt-N-format for every disk, why do we need to keep this info? Finally, is this exposed in backup-dumpxml?

Use the 'query-nodes' flag to return all stats. The flag was introduced prior to qemu-2.11 so we can always use it, but we invoke it only when querying stats. The other invocation is used for detecting the nodenames which is fragile code. The images without a frontend don't have the device field so the extraction code checks need to be relaxed. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_monitor_json.c | 20 +++++++++----------- src/qemu/qemu_monitor_json.h | 3 ++- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 908ee0d302..1fbf1eb5c7 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2038,14 +2038,14 @@ qemuMonitorGetBlockInfo(qemuMonitor *mon) * qemuMonitorQueryBlockstats: * @mon: monitor object * - * Returns data from a call to 'query-blockstats'. + * Returns data from a call to 'query-blockstats' without using 'query-nodes' */ virJSONValue * qemuMonitorQueryBlockstats(qemuMonitor *mon) { QEMU_CHECK_MONITOR_NULL(mon); - return qemuMonitorJSONQueryBlockstats(mon); + return qemuMonitorJSONQueryBlockstats(mon, false); } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e9be9bdabd..7f5d9f7cad 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2435,12 +2435,15 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValue *dev, virJSONValue * -qemuMonitorJSONQueryBlockstats(qemuMonitor *mon) +qemuMonitorJSONQueryBlockstats(qemuMonitor *mon, + bool queryNodes) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", NULL))) + if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", + "B:query-nodes", queryNodes, + NULL))) return NULL; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) @@ -2462,7 +2465,7 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitor *mon, size_t i; g_autoptr(virJSONValue) devices = NULL; - if (!(devices = qemuMonitorJSONQueryBlockstats(mon))) + if (!(devices = qemuMonitorJSONQueryBlockstats(mon, true))) return -1; for (i = 0; i < virJSONValueArraySize(devices); i++) { @@ -2476,16 +2479,11 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitor *mon, return -1; } - if (!(dev_name = virJSONValueObjectGetString(dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats device entry was not " - "in expected format")); - return -1; + if ((dev_name = virJSONValueObjectGetString(dev, "device"))) { + if (*dev_name == '\0') + dev_name = NULL; } - if (*dev_name == '\0') - dev_name = NULL; - rc = qemuMonitorJSONGetOneBlockStatsInfo(dev, dev_name, 0, hash); if (rc < 0) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index f9e01e5bf5..aa70d35bfb 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -75,7 +75,8 @@ int qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitor *mon, int qemuMonitorJSONGetBlockInfo(qemuMonitor *mon, GHashTable *table); -virJSONValue *qemuMonitorJSONQueryBlockstats(qemuMonitor *mon); +virJSONValue *qemuMonitorJSONQueryBlockstats(qemuMonitor *mon, + bool queryNodes); int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitor *mon, GHashTable *hash); int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitor *mon, -- 2.31.1

On Mon, Nov 1, 2021 at 4:48 PM Peter Krempa <pkrempa(a)redhat.com&gt; wrote: I wanted to ask about getting the allocation for mirror target disk and I see you already handle it, thanks!