[libvirt] [PATCH 0/9] qemu: stats/misc refactors (blockdev-add saga)
Last lot of refactors which could be split of prior to starting it for real. Peter Krempa (9): qemu: driver: Reuse qemuDomainBlocksStatsGather in qemuDomainGetBlockInfo qemu: domain: Move out clearing of backing chain in qemuDomainDetermineDiskChain qemu: domain: Add helper for getting the disk backend alias qemu: command: use qemuDomainDiskGetBackendAlias in commandline building qemu: monitor: Add the 'query-nodes' argument for query-blockstats qemu: json: Extract gathering of block statistics qemu: monitor: Split out code to gather data from 'query-block' utils: storage: Add helper for checking if storage source is the same qemu: Replace qemuDomainDiskSourceDiffers by virStorageSourceIsSameLocation src/libvirt_private.syms | 1 + src/qemu/qemu_block.c | 2 +- src/qemu/qemu_blockjob.c | 4 +- src/qemu/qemu_command.c | 24 ++++++---- src/qemu/qemu_domain.c | 66 ++++++++++++--------------- src/qemu/qemu_domain.h | 10 +++-- src/qemu/qemu_driver.c | 47 +++++++------------- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_monitor.c | 8 +++- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 103 ++++++++++++++++++++++++++++--------------- src/qemu/qemu_monitor_json.h | 3 +- src/qemu/qemu_process.c | 7 ++- src/util/virstoragefile.c | 43 ++++++++++++++++++ src/util/virstoragefile.h | 3 ++ 15 files changed, 198 insertions(+), 128 deletions(-) -- 2.16.2
Allow updating capacity for the block devices returned by qemuDomainBlocksStatsGather and replace the open-coded call to qemuMonitorGetAllBlockStatsInfo by the helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 41 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 847dab2edc..4f2855a26b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11063,6 +11063,7 @@ qemuDomainBlockStatsGatherTotals(void *payload, * @driver: driver object * @vm: domain object * @path: to gather the statistics for + * @capacity: refresh capacity of the backing image * @retstats: returns pointer to structure holding the stats * * Gathers the block statistics for use in qemuDomainBlockStats* APIs. @@ -11073,6 +11074,7 @@ static int qemuDomainBlocksStatsGather(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *path, + bool capacity, qemuBlockStatsPtr *retstats) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -11101,6 +11103,11 @@ qemuDomainBlocksStatsGather(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); nstats = qemuMonitorGetAllBlockStatsInfo(priv->mon, &blockstats, false); + + if (capacity && nstats >= 0 && + qemuMonitorBlockStatsUpdateCapacity(priv->mon, blockstats, false) < 0) + nstats = -1; + if (qemuDomainObjExitMonitor(driver, vm) < 0 || nstats < 0) goto cleanup; @@ -11154,7 +11161,7 @@ qemuDomainBlockStats(virDomainPtr dom, if (virDomainObjCheckActive(vm) < 0) goto endjob; - if (qemuDomainBlocksStatsGather(driver, vm, path, &blockstats) < 0) + if (qemuDomainBlocksStatsGather(driver, vm, path, false, &blockstats) < 0) goto endjob; stats->rd_req = blockstats->rd_req; @@ -11208,7 +11215,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, if (virDomainObjCheckActive(vm) < 0) goto endjob; - if ((nstats = qemuDomainBlocksStatsGather(driver, vm, path, + if ((nstats = qemuDomainBlocksStatsGather(driver, vm, path, false, &blockstats)) < 0) goto endjob; @@ -12021,10 +12028,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, int ret = -1; virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = NULL; - int rc; - virHashTablePtr stats = NULL; - qemuBlockStats *entry; - char *alias = NULL; + qemuBlockStatsPtr entry = NULL; virCheckFlags(0, -1); @@ -12065,28 +12069,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto endjob; } - if (!disk->info.alias || - !(alias = qemuDomainStorageAlias(disk->info.alias, 0))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing disk device alias name for %s"), disk->dst); + if (qemuDomainBlocksStatsGather(driver, vm, path, true, &entry) < 0) goto endjob; - } - - qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorGetAllBlockStatsInfo(qemuDomainGetMonitor(vm), - &stats, false); - if (rc >= 0) - rc = qemuMonitorBlockStatsUpdateCapacity(qemuDomainGetMonitor(vm), - stats, false); - - if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - goto endjob; - - if (!(entry = virHashLookup(stats, alias))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to gather stats for disk '%s'"), disk->dst); - goto endjob; - } if (!entry->wr_highest_offset_valid) { info->allocation = entry->physical; @@ -12131,8 +12115,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, endjob: qemuDomainObjEndJob(driver, vm); cleanup: - VIR_FREE(alias); - virHashFree(stats); + VIR_FREE(entry); virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:02PM +0200, Peter Krempa wrote:
Allow updating capacity for the block devices returned by qemuDomainBlocksStatsGather and replace the open-coded call to qemuMonitorGetAllBlockStatsInfo by the helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 41 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 29 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
In some cases backing chain needs to be cleared prior to re-detection. Move this step out of qemuDomainDetermineDiskChain as only certain places need it and the function itself is able to skip to the end of the chain to perform detection. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 4 ++-- src/qemu/qemu_domain.c | 4 ---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_process.c | 7 +++++-- 6 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index b08e047490..0f52996ade 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -175,8 +175,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; disk->src->id = 0; - ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk, - true, true)); + virStorageSourceBackingStoreClear(disk->src); + ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk, true)); ignore_value(qemuBlockNodeNamesDetect(driver, vm, asyncJob)); diskPriv->blockjob = false; break; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed76495309..c92ac8c926 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8352,7 +8352,6 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - bool force_probe, bool report_broken) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -8368,9 +8367,6 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, goto cleanup; } - if (force_probe) - virStorageSourceBackingStoreClear(src); - /* There is no need to check the backing chain for disks without backing * support */ if (virStorageSourceIsLocalStorage(src) && diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index e748d78adb..1692fa9838 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -718,7 +718,6 @@ int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - bool force_probe, bool report_broken); bool qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4f2855a26b..64e2c274c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7850,7 +7850,7 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm, if (virDomainDiskTranslateSourcePool(disk) < 0) goto cleanup; - if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) + if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) goto cleanup; if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, @@ -16931,7 +16931,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, oldsrc = disk->src; disk->src = disk->mirror; - if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) + if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) goto cleanup; if (disk->mirror->format && diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2b6633a998..98f7a28826 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -669,7 +669,7 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, if (qemuSetUnprivSGIO(dev) < 0) goto cleanup; - if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) + if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) goto cleanup; switch ((virDomainDiskDevice) disk->device) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c903a8e5c8..138e128807 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6015,7 +6015,9 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, if (virStorageSourceIsEmpty(disk->src)) continue; - if (qemuDomainDetermineDiskChain(driver, vm, disk, true, true) >= 0) + virStorageSourceBackingStoreClear(disk->src); + + if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) continue; if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= 0) @@ -7691,7 +7693,8 @@ qemuProcessReconnect(void *opaque) /* This should be the only place that calls * qemuDomainDetermineDiskChain with @report_broken == false * to guarantee best-effort domain reconnect */ - if (qemuDomainDetermineDiskChain(driver, obj, disk, true, false) < 0) + virStorageSourceBackingStoreClear(disk->src); + if (qemuDomainDetermineDiskChain(driver, obj, disk, false) < 0) goto error; } else { VIR_DEBUG("skipping backing chain detection for '%s'", disk->dst); -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:03PM +0200, Peter Krempa wrote:
In some cases backing chain needs to be cleared prior to re-detection. Move this step out of qemuDomainDetermineDiskChain as only certain places need it and the function itself is able to skip to the end of the chain to perform detection.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 4 ++-- src/qemu/qemu_domain.c | 4 ---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_process.c | 7 +++++-- 6 files changed, 10 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The disk backend alias was historically the alias of the -drive backing the storage. For setups with -blockdev this will become more complex as it will depend on other configs and generally will differ. --- src/qemu/qemu_domain.c | 28 ++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 ++++++ 2 files changed, 34 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c92ac8c926..f8a621a5c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8449,6 +8449,34 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, } +/** + * qemuDomainDiskGetBackendAlias: + * @disk: disk definition + * @qemuCaps: emulator capabilities + * @backendAlias: filled with the alias of the disk storage backend + * + * Returns the correct alias for the disk backend. This may be the alias of + * -drive for legacy setup or the correct node name for -blockdev setups. + * + * @backendAlias may be NULL on success if the backend does not exist + * (disk is empty). Caller is responsible for freeing @backendAlias. + * + * Returns 0 on success, -1 on error with libvirt error reported. + */ +int +qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, + char **backendAlias) +{ + *backendAlias = NULL; + + if (!(*backendAlias = qemuAliasDiskDriveFromDisk(disk))) + return -1; + + return 0; +} + + /** * qemuDomainDiskChainElementRevoke: * diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 1692fa9838..22c3a51354 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -732,6 +732,12 @@ int qemuDomainStorageFileInit(virQEMUDriverPtr driver, virStorageSourcePtr parent); char *qemuDomainStorageAlias(const char *device, int depth); +int qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps, + char **backendAlias) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; + void qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr elem); -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:04PM +0200, Peter Krempa wrote:
The disk backend alias was historically the alias of the -drive backing the storage. For setups with -blockdev this will become more complex as it will depend on other configs and generally will differ. --- src/qemu/qemu_domain.c | 28 ++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 ++++++ 2 files changed, 34 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c92ac8c926..f8a621a5c9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8449,6 +8449,34 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, }
+/** + * qemuDomainDiskGetBackendAlias: + * @disk: disk definition + * @qemuCaps: emulator capabilities + * @backendAlias: filled with the alias of the disk storage backend + * + * Returns the correct alias for the disk backend. This may be the alias of + * -drive for legacy setup or the correct node name for -blockdev setups. + *
+ * @backendAlias may be NULL on success if the backend does not exist + * (disk is empty).
I presume this will be true in the future,
Caller is responsible for freeing @backendAlias. + * + * Returns 0 on success, -1 on error with libvirt error reported. + */ +int +qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
and the capabilities will be used.
+ char **backendAlias) +{ + *backendAlias = NULL; + + if (!(*backendAlias = qemuAliasDiskDriveFromDisk(disk))) + return -1; + + return 0; +} + + /** * qemuDomainDiskChainElementRevoke: *
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use the proper backend for the block device both when using -drive and when using -blockdev for disk drives and floppy disks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 44ae8dcef7..6e550cf951 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1818,7 +1818,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, virBuffer opt = VIR_BUFFER_INITIALIZER; const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); const char *contAlias; - char *drivealias; + char *backendAlias = NULL; int controllerModel; if (qemuCheckDiskConfig(disk, qemuCaps) < 0) @@ -2077,10 +2077,14 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISK_SHARE_RW)) virBufferAddLit(&opt, ",share-rw=on"); - if (!(drivealias = qemuAliasDiskDriveFromDisk(disk))) + if (qemuDomainDiskGetBackendAlias(disk, qemuCaps, &backendAlias) < 0) goto error; - virBufferAsprintf(&opt, ",drive=%s,id=%s", drivealias, disk->info.alias); - VIR_FREE(drivealias); + + if (backendAlias) + virBufferAsprintf(&opt, ",drive=%s", backendAlias); + VIR_FREE(backendAlias); + + virBufferAsprintf(&opt, ",id=%s", disk->info.alias); if (bootindex && virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) virBufferAsprintf(&opt, ",bootindex=%u", bootindex); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKIO)) { @@ -2139,6 +2143,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, return virBufferContentAndReset(&opt); error: + VIR_FREE(backendAlias); virBufferFreeAndReset(&opt); return NULL; } @@ -2148,8 +2153,8 @@ static int qemuBuildFloppyCommandLineOptions(virCommandPtr cmd, const virDomainDef *def, virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps, unsigned int bootindex) - { virBuffer fdc_opts = VIR_BUFFER_INITIALIZER; char driveLetter; @@ -2163,10 +2168,11 @@ qemuBuildFloppyCommandLineOptions(virCommandPtr cmd, else driveLetter = 'A'; - if (!(backendAlias = qemuAliasDiskDriveFromDisk(disk))) - return -1; + if (qemuDomainDiskGetBackendAlias(disk, qemuCaps, &backendAlias) < 0) + goto cleanup; - if (virAsprintf(&backendStr, "drive%c=%s", driveLetter, backendAlias) < 0) + if (backendAlias && + virAsprintf(&backendStr, "drive%c=%s", driveLetter, backendAlias) < 0) goto cleanup; if (bootindex && @@ -2281,7 +2287,7 @@ qemuBuildDiskCommandLine(virCommandPtr cmd, if (!qemuDiskBusNeedsDriveArg(disk->bus)) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { - if (qemuBuildFloppyCommandLineOptions(cmd, def, disk, + if (qemuBuildFloppyCommandLineOptions(cmd, def, disk, qemuCaps, bootindex) < 0) return -1; } else { -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:05PM +0200, Peter Krempa wrote:
Use the proper backend for the block device both when using -drive and when using -blockdev for disk drives and floppy disks.
Shouldn't you use future tense here?
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The 'query-blockstats' command does not return statistics for the explicitly named nodes unless the new argument is specified. Add infrastrucuture that will allow us to use the new approach if desired. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 2 +- src/qemu/qemu_monitor.c | 8 ++++++-- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 9 ++++++--- src/qemu/qemu_monitor_json.h | 3 ++- 5 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 7ad79c7e7d..a633bfa0b0 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -356,7 +356,7 @@ qemuBlockNodeNamesDetect(virQEMUDriverPtr driver, return -1; data = qemuMonitorQueryNamedBlockNodes(qemuDomainGetMonitor(vm)); - blockstats = qemuMonitorQueryBlockstats(qemuDomainGetMonitor(vm)); + blockstats = qemuMonitorQueryBlockstats(qemuDomainGetMonitor(vm), false); if (qemuDomainObjExitMonitor(driver, vm) < 0 || !data || !blockstats) goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index bc116e4e2d..8618a44f32 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2243,15 +2243,19 @@ qemuMonitorGetBlockInfo(qemuMonitorPtr mon) /** * qemuMonitorQueryBlockstats: * @mon: monitor object + * @nodenames: include backing chain nodes with explicitly specified name * * Returns data from a call to 'query-blockstats'. */ virJSONValuePtr -qemuMonitorQueryBlockstats(qemuMonitorPtr mon) +qemuMonitorQueryBlockstats(qemuMonitorPtr mon, + bool nodenames) { QEMU_CHECK_MONITOR_NULL(mon); - return qemuMonitorJSONQueryBlockstats(mon); + VIR_DEBUG("nodenames: %d", nodenames); + + return qemuMonitorJSONQueryBlockstats(mon, nodenames); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 81474a04f6..b4b9cf91eb 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -563,7 +563,8 @@ int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon, int qemuMonitorBlockIOStatusToError(const char *status); virHashTablePtr qemuMonitorGetBlockInfo(qemuMonitorPtr mon); -virJSONValuePtr qemuMonitorQueryBlockstats(qemuMonitorPtr mon); +virJSONValuePtr qemuMonitorQueryBlockstats(qemuMonitorPtr mon, + bool nodenames); typedef struct _qemuBlockStats qemuBlockStats; typedef qemuBlockStats *qemuBlockStatsPtr; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index cc3c8f2dd6..e41c1d47aa 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2326,13 +2326,16 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, virJSONValuePtr -qemuMonitorJSONQueryBlockstats(qemuMonitorPtr mon) +qemuMonitorJSONQueryBlockstats(qemuMonitorPtr mon, + bool nodenames) { virJSONValuePtr cmd; virJSONValuePtr reply = NULL; virJSONValuePtr ret = NULL; - if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", NULL))) + if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", + "B:query-nodes", nodenames, + NULL))) return NULL; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) @@ -2361,7 +2364,7 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, size_t i; virJSONValuePtr devices; - if (!(devices = qemuMonitorJSONQueryBlockstats(mon))) + if (!(devices = qemuMonitorJSONQueryBlockstats(mon, false))) return -1; for (i = 0; i < virJSONValueArraySize(devices); i++) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 66536ceb97..a6b6579849 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -82,7 +82,8 @@ int qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table); -virJSONValuePtr qemuMonitorJSONQueryBlockstats(qemuMonitorPtr mon); +virJSONValuePtr qemuMonitorJSONQueryBlockstats(qemuMonitorPtr mon, + bool nodenames); int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, virHashTablePtr hash, bool backingChain); -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:06PM +0200, Peter Krempa wrote:
The 'query-blockstats' command does not return statistics for the explicitly named nodes unless the new argument is specified. Add
Hopefully nobody will need to use profanity in node names.
infrastrucuture that will allow us to use the new approach if desired.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 2 +- src/qemu/qemu_monitor.c | 8 ++++++-- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 9 ++++++--- src/qemu/qemu_monitor_json.h | 3 ++- 5 files changed, 17 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The code is useful also when gathering statistics per node name, so extract it to a separate functions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 51 ++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e41c1d47aa..37a4e59189 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2253,26 +2253,15 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, } -static int -qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, - const char *dev_name, - int depth, - virHashTablePtr hash, - bool backingChain) +static qemuBlockStatsPtr +qemuMonitorJSONBlockStatsCollectData(virJSONValuePtr dev, + int *nstats) { qemuBlockStatsPtr bstats = NULL; - virJSONValuePtr stats; + qemuBlockStatsPtr ret = NULL; virJSONValuePtr parent; virJSONValuePtr parentstats; - int ret = -1; - int nstats = 0; - char *entry_name = qemuDomainStorageAlias(dev_name, depth); - virJSONValuePtr backing; - - if (!entry_name) - goto cleanup; - if (VIR_ALLOC(bstats) < 0) - goto cleanup; + virJSONValuePtr stats; if ((stats = virJSONValueObjectGetObject(dev, "stats")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2281,6 +2270,9 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, goto cleanup; } + if (VIR_ALLOC(bstats) < 0) + goto cleanup; + #define QEMU_MONITOR_BLOCK_STAT_GET(NAME, VAR, MANDATORY) \ if (MANDATORY || virJSONValueObjectHasKey(stats, NAME)) { \ nstats++; \ @@ -2307,6 +2299,33 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, bstats->wr_highest_offset_valid = true; } + VIR_STEAL_PTR(ret, bstats); + + cleanup: + VIR_FREE(bstats); + return ret; +} + + +static int +qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, + const char *dev_name, + int depth, + virHashTablePtr hash, + bool backingChain) +{ + qemuBlockStatsPtr bstats = NULL; + int ret = -1; + int nstats = 0; + char *entry_name = qemuDomainStorageAlias(dev_name, depth); + virJSONValuePtr backing; + + if (!entry_name) + goto cleanup; + + if (!(bstats = qemuMonitorJSONBlockStatsCollectData(dev, &nstats))) + goto cleanup; + if (virHashAddEntry(hash, entry_name, bstats) < 0) goto cleanup; bstats = NULL; -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:07PM +0200, Peter Krempa wrote:
The code is useful also when gathering statistics per node name, so extract it to a separate functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 51 ++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 16 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Extract the code for future reuse. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 37a4e59189..81043e2af5 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2423,39 +2423,50 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, static int -qemuMonitorJSONBlockStatsUpdateCapacityOne(virJSONValuePtr image, - const char *dev_name, - int depth, - virHashTablePtr stats, - bool backingChain) +qemuMonitorJSONBlockStatsUpdateCapacityData(virJSONValuePtr image, + const char *name, + virHashTablePtr stats) { qemuBlockStatsPtr bstats; - int ret = -1; - char *entry_name = qemuDomainStorageAlias(dev_name, depth); - virJSONValuePtr backing; - if (!(bstats = virHashLookup(stats, entry_name))) { + if (!(bstats = virHashLookup(stats, name))) { if (VIR_ALLOC(bstats) < 0) - goto cleanup; + return -1; - if (virHashAddEntry(stats, entry_name, bstats) < 0) { + if (virHashAddEntry(stats, name, bstats) < 0) { VIR_FREE(bstats); - goto cleanup; + return -1; } } - /* After this point, we ignore failures; the stats were - * zero-initialized when created which is a sane fallback. */ - ret = 0; + /* failures can be ignored after this point */ if (virJSONValueObjectGetNumberUlong(image, "virtual-size", &bstats->capacity) < 0) - goto cleanup; + return 0; /* if actual-size is missing, image is not thin provisioned */ if (virJSONValueObjectGetNumberUlong(image, "actual-size", &bstats->physical) < 0) bstats->physical = bstats->capacity; + return 0; +} + + +static int +qemuMonitorJSONBlockStatsUpdateCapacityOne(virJSONValuePtr image, + const char *dev_name, + int depth, + virHashTablePtr stats, + bool backingChain) +{ + int ret = -1; + char *entry_name = qemuDomainStorageAlias(dev_name, depth); + virJSONValuePtr backing; + + if (qemuMonitorJSONBlockStatsUpdateCapacityData(image, entry_name, stats) < 0) + goto cleanup; + if (backingChain && (backing = virJSONValueObjectGetObject(image, "backing-image"))) { ret = qemuMonitorJSONBlockStatsUpdateCapacityOne(backing, -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:08PM +0200, Peter Krempa wrote:
Extract the code for future reuse.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
To allow checking whether a storage source points to the same location add a helper which checks the relevant fields. This will allow replacing a similar check done by formatting the command line arguments for qemu-like syntax. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 47 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1caecb96b6..fb7c8c724d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2855,6 +2855,7 @@ virStorageSourceIsBlockLocal; virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; virStorageSourceIsRelative; +virStorageSourceIsSameLocation; virStorageSourceNetworkAssignDefaultPorts; virStorageSourceNewFromBacking; virStorageSourceNewFromBackingAbsolute; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 58f67278da..96ed4b1489 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2259,6 +2259,49 @@ virStorageSourceCopy(const virStorageSource *src, } +/** + * virStorageSourceIsSameLocation: + * + * Returns true if the sources @a and @b point to the same storage location. + * This does not compare any other configuration option + */ +bool +virStorageSourceIsSameLocation(virStorageSourcePtr a, + virStorageSourcePtr b) +{ + size_t i; + + /* there are multiple possibilities to define an empty source */ + if (virStorageSourceIsEmpty(a) && + virStorageSourceIsEmpty(b)) + return true; + + if (virStorageSourceGetActualType(a) != virStorageSourceGetActualType(b)) + return false; + + if (STRNEQ_NULLABLE(a->path, b->path) || + STRNEQ_NULLABLE(a->volume, b->volume) || + STRNEQ_NULLABLE(a->snapshot, b->snapshot)) + return false; + + if (a->type == VIR_STORAGE_TYPE_NETWORK) { + if (a->protocol != b->protocol || + a->nhosts != b->nhosts) + return false; + + for (i = 0; i < a->nhosts; i++) { + if (a->hosts[i].transport != b->hosts[i].transport || + a->hosts[i].port != b->hosts[i].port || + STRNEQ_NULLABLE(a->hosts[i].name, b->hosts[i].name) || + STRNEQ_NULLABLE(a->hosts[i].socket, b->hosts[i].socket)) + return false; + } + } + + return true; +} + + /** * virStorageSourceInitChainElement: * @newelem: New backing chain element disk source diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 991098e6c6..c2c40edf68 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -436,6 +436,9 @@ virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, bool backingChain) ATTRIBUTE_NONNULL(1); +bool virStorageSourceIsSameLocation(virStorageSourcePtr a, + virStorageSourcePtr b) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageSourceParseRBDColonString(const char *rbdstr, virStorageSourcePtr src) -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:09PM +0200, Peter Krempa wrote:
To allow checking whether a storage source points to the same location add a helper which checks the relevant fields. This will allow replacing a similar check done by formatting the command line arguments for qemu-like syntax.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 47 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Now that we have a saner replacement for checking if the disk source is the same use it instead of formatting qemu command-line chunks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 34 ---------------------------------- src/qemu/qemu_domain.h | 3 --- src/qemu/qemu_driver.c | 2 +- 3 files changed, 1 insertion(+), 38 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f8a621a5c9..acf5cc8dd2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8555,40 +8555,6 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, } -bool -qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk, - virDomainDiskDefPtr origDisk) -{ - char *diskSrc = NULL, *origDiskSrc = NULL; - bool diskEmpty, origDiskEmpty; - bool ret = true; - - diskEmpty = virStorageSourceIsEmpty(disk->src); - origDiskEmpty = virStorageSourceIsEmpty(origDisk->src); - - if (diskEmpty && origDiskEmpty) - return false; - - if (diskEmpty ^ origDiskEmpty) - return true; - - /* This won't be a network storage, so no need to get the diskPriv - * in order to fetch the secret, thus NULL for param2 */ - if (qemuGetDriveSourceString(disk->src, NULL, &diskSrc) < 0 || - qemuGetDriveSourceString(origDisk->src, NULL, &origDiskSrc) < 0) - goto cleanup; - - /* So far in qemu disk sources are considered different - * if either path to disk or its format changes. */ - ret = virDomainDiskGetFormat(disk) != virDomainDiskGetFormat(origDisk) || - STRNEQ_NULLABLE(diskSrc, origDiskSrc); - cleanup: - VIR_FREE(diskSrc); - VIR_FREE(origDiskSrc); - return ret; -} - - /* * Makes sure the @disk differs from @orig_disk only by the source * path and nothing else. Fields that are being checked and the diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 22c3a51354..bff293fc0a 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -720,9 +720,6 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, bool report_broken); -bool qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk, - virDomainDiskDefPtr origDisk); - bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, virDomainDiskDefPtr orig_disk); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 64e2c274c2..dea548f982 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7871,7 +7871,7 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm, if (!qemuDomainDiskChangeSupported(disk, orig_disk)) goto cleanup; - if (qemuDomainDiskSourceDiffers(disk, orig_disk)) { + if (!virStorageSourceIsSameLocation(disk->src, orig_disk->src)) { /* Disk source can be changed only for removable devices */ if (disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM && disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { -- 2.16.2
On Wed, Jul 18, 2018 at 05:22:10PM +0200, Peter Krempa wrote:
Now that we have a saner replacement for checking if the disk source is the same use it instead of formatting qemu command-line chunks.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 34 ---------------------------------- src/qemu/qemu_domain.h | 3 --- src/qemu/qemu_driver.c | 2 +- 3 files changed, 1 insertion(+), 38 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa