[PATCH 00/21] qemu: Refactor domstats code to avoid error reporting
The workers of qemuDomainGetStats should not report errors if they can't fetch data; but rather omit the entries. Refactor the code to de-incentivize error reporting. Peter Krempa (21): qemuDomainGetStatsBlockExportHeader: Remove return value qemuDomainGetStatsBlockExportFrontend: Remove return value qemuDomainGetStatsBlockExportBackendStorage: Remove return value qemuDomainGetStatsOneBlockFallback: Remove return value qemuDomainGetStatsOneBlock: Remove return value qemuDomainStorageAlias: Remove NULL checks from callers qemuDomainGetStatsBlockExportHeader: Remove return value virBitmapFormat: Clarify returned values virDomainResctrlMonDefParse: Refactor temporary variables virDomainCputuneDefFormat: Refactor bitmap formatting virBitmapFormat: Don't check return value qemuDomainGetStatsCpuCgroup: Remove return value qemuDomainGetStatsCpuProc: Remove return value qemuDomainGetStatsCpuHaltPollTime: Remove return value qemuDomainGetStatsCpuCache: Don't error out virPerfReadEvent: Refactor to return -errno on failure qemuDomainGetStatsPerfOneEvent: Ignore erros from 'virPerfReadEvent' qemuDomainGetStatsIOThread: Don't error out if fetching iothread info fails qemuDomainGetStatsMemoryBandwidth: Don't error out qemuDomainGetStatsDirtyRate: Don't error out qemuDomainGetStats: Convert worker functions to void src/ch/ch_driver.c | 3 +- src/conf/capabilities.c | 9 +- src/conf/domain_conf.c | 69 ++----- src/conf/numa_conf.c | 18 +- src/conf/virnetworkobj.c | 3 - src/hypervisor/domain_cgroup.c | 6 +- src/libxl/libxl_driver.c | 3 +- src/libxl/xen_common.c | 6 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_domain.c | 10 +- src/qemu/qemu_driver.c | 341 ++++++++++++--------------------- src/qemu/qemu_monitor_json.c | 5 +- src/util/virbitmap.c | 11 +- src/util/vircgroup.c | 5 +- src/util/virperf.c | 19 +- src/vz/vz_sdk.c | 3 +- 16 files changed, 178 insertions(+), 336 deletions(-) -- 2.48.1
The function always returns 0. Remove return value and fix callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1d0da1028f..7a39e83d39 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17359,7 +17359,7 @@ qemuDomainGetStatsBlockExportFrontend(const char *frontendname, } -static int +static void qemuDomainGetStatsBlockExportHeader(virDomainDiskDef *disk, virStorageSource *src, size_t recordnr, @@ -17372,8 +17372,6 @@ qemuDomainGetStatsBlockExportHeader(virDomainDiskDef *disk, if (src->id) virTypedParamListAddUInt(params, src->id, "block.%zu.backingIndex", recordnr); - - return 0; } @@ -17399,10 +17397,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, VIR_INFO("optional disk '%s' source file is missing, " "skip getting stats", disk->dst); - if (qemuDomainGetStatsBlockExportHeader(disk, disk->src, *recordnr, - params) < 0) { - return -1; - } + qemuDomainGetStatsBlockExportHeader(disk, disk->src, *recordnr, params); (*recordnr)++; @@ -17411,10 +17406,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, /* vhost-user disk doesn't support getting block stats */ if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER) { - if (qemuDomainGetStatsBlockExportHeader(disk, disk->src, *recordnr, - params) < 0) { - return -1; - } + qemuDomainGetStatsBlockExportHeader(disk, disk->src, *recordnr, params); (*recordnr)++; @@ -17445,8 +17437,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, backendstoragealias = alias; } - if (qemuDomainGetStatsBlockExportHeader(disk, n, *recordnr, params) < 0) - return -1; + qemuDomainGetStatsBlockExportHeader(disk, n, *recordnr, params); /* The following stats make sense only for the frontend device */ if (n == disk->src) { @@ -17479,8 +17470,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, if (disk->mirror && disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { - if (qemuDomainGetStatsBlockExportHeader(disk, disk->mirror, *recordnr, params) < 0) - return -1; + qemuDomainGetStatsBlockExportHeader(disk, disk->mirror, *recordnr, params); if (qemuDomainGetStatsOneBlock(cfg, dom, params, qemuBlockStorageSourceGetEffectiveNodename(disk->mirror), @@ -17507,9 +17497,8 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, continue; if (backupdisk->store) { - if (qemuDomainGetStatsBlockExportHeader(disk, backupdisk->store, - *recordnr, params) < 0) - return -1; + qemuDomainGetStatsBlockExportHeader(disk, backupdisk->store, + *recordnr, params); if (qemuDomainGetStatsOneBlock(cfg, dom, params, qemuBlockStorageSourceGetEffectiveNodename(backupdisk->store), -- 2.48.1
The function always returns 0. Remove return value and fix callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7a39e83d39..3dee0a4a91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17332,7 +17332,7 @@ qemuDomainGetStatsBlockExportBackendStorage(const char *entryname, } -static int +static void qemuDomainGetStatsBlockExportFrontend(const char *frontendname, GHashTable *stats, size_t idx, @@ -17344,7 +17344,7 @@ qemuDomainGetStatsBlockExportFrontend(const char *frontendname, * trying to refresh the stats from the disk. Inability to provide stats is * usually caused by blocked storage so this would make libvirtd hang */ if (!stats || !frontendname || !(en = virHashLookup(stats, frontendname))) - return 0; + return; virTypedParamListAddULLong(par, en->rd_req, "block.%zu.rd.reqs", idx); virTypedParamListAddULLong(par, en->rd_bytes, "block.%zu.rd.bytes", idx); @@ -17354,8 +17354,6 @@ qemuDomainGetStatsBlockExportFrontend(const char *frontendname, virTypedParamListAddULLong(par, en->wr_total_times, "block.%zu.wr.times", idx); virTypedParamListAddULLong(par, en->flush_req, "block.%zu.fl.reqs", idx); virTypedParamListAddULLong(par, en->flush_total_times, "block.%zu.fl.times", idx); - - return 0; } @@ -17441,9 +17439,8 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, /* The following stats make sense only for the frontend device */ if (n == disk->src) { - if (qemuDomainGetStatsBlockExportFrontend(frontendalias, stats, *recordnr, - params) < 0) - return -1; + qemuDomainGetStatsBlockExportFrontend(frontendalias, stats, *recordnr, + params); } if (qemuDomainGetStatsOneBlock(cfg, dom, params, -- 2.48.1
The function always returns 0. Remove return value and fix callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3dee0a4a91..327edfe506 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17314,7 +17314,7 @@ qemuDomainGetStatsOneBlock(virQEMUDriverConfig *cfg, } -static int +static void qemuDomainGetStatsBlockExportBackendStorage(const char *entryname, GHashTable *stats, size_t recordnr, @@ -17323,12 +17323,10 @@ qemuDomainGetStatsBlockExportBackendStorage(const char *entryname, qemuBlockStats *entry; if (!stats || !entryname || !(entry = virHashLookup(stats, entryname))) - return 0; + return; if (entry->write_threshold) virTypedParamListAddULLong(params, entry->write_threshold, "block.%zu.threshold", recordnr); - - return 0; } @@ -17448,10 +17446,8 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, stats) < 0) return -1; - if (qemuDomainGetStatsBlockExportBackendStorage(backendstoragealias, - stats, *recordnr, - params) < 0) - return -1; + qemuDomainGetStatsBlockExportBackendStorage(backendstoragealias, + stats, *recordnr, params); (*recordnr)++; @@ -17476,10 +17472,8 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, stats) < 0) return -1; - if (qemuDomainGetStatsBlockExportBackendStorage(qemuBlockStorageSourceGetStorageNodename(disk->mirror), - stats, *recordnr, - params) < 0) - return -1; + qemuDomainGetStatsBlockExportBackendStorage(qemuBlockStorageSourceGetStorageNodename(disk->mirror), + stats, *recordnr, params); (*recordnr)++; } @@ -17504,10 +17498,9 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, stats) < 0) return -1; - if (qemuDomainGetStatsBlockExportBackendStorage(qemuBlockStorageSourceGetStorageNodename(backupdisk->store), - stats, *recordnr, - params) < 0) - return -1; + qemuDomainGetStatsBlockExportBackendStorage(qemuBlockStorageSourceGetStorageNodename(backupdisk->store), + stats, *recordnr, + params); (*recordnr)++; } -- 2.48.1
The function always returns 0. Remove return value and fix callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 327edfe506..7318c145d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17242,22 +17242,20 @@ qemuDomainGetStatsInterface(virQEMUDriver *driver G_GNUC_UNUSED, #undef QEMU_ADD_NET_PARAM /* refresh information by opening images on the disk */ -static int +static void qemuDomainGetStatsOneBlockFallback(virQEMUDriverConfig *cfg, virDomainObj *dom, virTypedParamList *params, virStorageSource *src, size_t block_idx) { - if (virStorageSourceIsEmpty(src)) - return 0; - - if (virStorageSourceIsFD(src)) - return 0; + if (virStorageSourceIsEmpty(src) || + virStorageSourceIsFD(src)) + return; if (qemuStorageLimitsRefresh(cfg, dom, src, true) <= 0) { virResetLastError(); - return 0; + return; } if (src->allocation) @@ -17268,8 +17266,6 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriverConfig *cfg, if (src->physical) virTypedParamListAddULLong(params, src->physical, "block.%zu.physical", block_idx); - - return 0; } @@ -17287,8 +17283,8 @@ qemuDomainGetStatsOneBlock(virQEMUDriverConfig *cfg, /* the VM is offline so we have to go and load the stast from the disk by * ourselves */ if (!virDomainObjIsActive(dom)) { - return qemuDomainGetStatsOneBlockFallback(cfg, dom, params, - src, block_idx); + qemuDomainGetStatsOneBlockFallback(cfg, dom, params, src, block_idx); + return 0; } /* In case where qemu didn't provide the stats we stop here rather than -- 2.48.1
The function always returns 0. Remove return value and fix callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7318c145d0..aee66b2bc7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17269,7 +17269,7 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriverConfig *cfg, } -static int +static void qemuDomainGetStatsOneBlock(virQEMUDriverConfig *cfg, virDomainObj *dom, virTypedParamList *params, @@ -17284,14 +17284,14 @@ qemuDomainGetStatsOneBlock(virQEMUDriverConfig *cfg, * ourselves */ if (!virDomainObjIsActive(dom)) { qemuDomainGetStatsOneBlockFallback(cfg, dom, params, src, block_idx); - return 0; + return; } /* In case where qemu didn't provide the stats we stop here rather than * trying to refresh the stats from the disk. Inability to provide stats is * usually caused by blocked storage so this would make libvirtd hang */ if (!stats || !entryname || !(entry = virHashLookup(stats, entryname))) - return 0; + return; virTypedParamListAddULLong(params, entry->wr_highest_offset, "block.%zu.allocation", block_idx); @@ -17305,8 +17305,6 @@ qemuDomainGetStatsOneBlock(virQEMUDriverConfig *cfg, virTypedParamListAddULLong(params, src->physical, "block.%zu.physical", block_idx); } } - - return 0; } @@ -17437,10 +17435,8 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, params); } - if (qemuDomainGetStatsOneBlock(cfg, dom, params, - backendalias, n, *recordnr, - stats) < 0) - return -1; + qemuDomainGetStatsOneBlock(cfg, dom, params, + backendalias, n, *recordnr, stats); qemuDomainGetStatsBlockExportBackendStorage(backendstoragealias, stats, *recordnr, params); @@ -17461,12 +17457,9 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { qemuDomainGetStatsBlockExportHeader(disk, disk->mirror, *recordnr, params); - if (qemuDomainGetStatsOneBlock(cfg, dom, params, - qemuBlockStorageSourceGetEffectiveNodename(disk->mirror), - disk->mirror, - *recordnr, - stats) < 0) - return -1; + qemuDomainGetStatsOneBlock(cfg, dom, params, + qemuBlockStorageSourceGetEffectiveNodename(disk->mirror), + disk->mirror, *recordnr, stats); qemuDomainGetStatsBlockExportBackendStorage(qemuBlockStorageSourceGetStorageNodename(disk->mirror), stats, *recordnr, params); @@ -17487,12 +17480,9 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, qemuDomainGetStatsBlockExportHeader(disk, backupdisk->store, *recordnr, params); - if (qemuDomainGetStatsOneBlock(cfg, dom, params, - qemuBlockStorageSourceGetEffectiveNodename(backupdisk->store), - backupdisk->store, - *recordnr, - stats) < 0) - return -1; + qemuDomainGetStatsOneBlock(cfg, dom, params, + qemuBlockStorageSourceGetEffectiveNodename(backupdisk->store), + backupdisk->store, *recordnr, stats); qemuDomainGetStatsBlockExportBackendStorage(qemuBlockStorageSourceGetStorageNodename(backupdisk->store), stats, *recordnr, -- 2.48.1
'qemuDomainStorageAlias' always returns non-NULL pointer if it gets a non-NULL string on input. Remove unneeded checks from callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 5 ++--- src/qemu/qemu_monitor_json.c | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aee66b2bc7..877fefaa38 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17415,9 +17415,8 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, backendstoragealias = qemuBlockStorageSourceGetStorageNodename(n); } else { /* alias may be NULL if the VM is not running */ - if (disk->info.alias && - !(alias = qemuDomainStorageAlias(disk->info.alias, n->id))) - return -1; + if (disk->info.alias) + alias = qemuDomainStorageAlias(disk->info.alias, n->id); /* for 'sd' disks we won't be displaying stats for the backing chain * as we don't update the stats correctly */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 554572b13d..74420b2ee7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2332,9 +2332,8 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValue *dev, g_autofree char *devicename = NULL; virJSONValue *backing; - if (dev_name && - !(devicename = qemuDomainStorageAlias(dev_name, depth))) - return -1; + if (dev_name) + devicename = qemuDomainStorageAlias(dev_name, depth); qdevname = virJSONValueObjectGetString(dev, "qdev"); nodename = virJSONValueObjectGetString(dev, "node-name"); -- 2.48.1
The function always returns 0. Remove return value and fix callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 877fefaa38..174dad64b0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17365,7 +17365,7 @@ qemuDomainGetStatsBlockExportHeader(virDomainDiskDef *disk, } -static int +static void qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, GHashTable *stats, virTypedParamList *params, @@ -17391,7 +17391,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, (*recordnr)++; - return 0; + return; } /* vhost-user disk doesn't support getting block stats */ @@ -17400,7 +17400,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, (*recordnr)++; - return 0; + return; } for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { @@ -17494,8 +17494,6 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, } } } - - return 0; } @@ -17530,10 +17528,8 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, } for (i = 0; i < dom->def->ndisks; i++) { - if (qemuDomainGetStatsBlockExportDisk(dom->def->disks[i], stats, - blockparams, &visited, - visitBacking, cfg, dom) < 0) - return -1; + qemuDomainGetStatsBlockExportDisk(dom->def->disks[i], stats, blockparams, + &visited, visitBacking, cfg, dom); } virTypedParamListAddUInt(params, visited, "block.count"); -- 2.48.1
NULL can't be returned; don't mention it in the docs. Avoid extra cofusing variable when returning copy of empty string. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virbitmap.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 138c1ac5af..e42f5b872c 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -327,8 +327,8 @@ virBitmapToString(virBitmap *bitmap) * * If bitmap is NULL or it has no bits set, an empty string is returned. * - * Returns the string on success or NULL otherwise. Caller should call - * VIR_FREE to free the string. + * Returns the formatted string. Caller is responsible for freeing the returned + * string. */ char * virBitmapFormat(virBitmap *bitmap) @@ -337,11 +337,8 @@ virBitmapFormat(virBitmap *bitmap) bool first = true; int start, cur, prev; - if (!bitmap || (cur = virBitmapNextSetBit(bitmap, -1)) < 0) { - char *ret; - ret = g_strdup(""); - return ret; - } + if (!bitmap || (cur = virBitmapNextSetBit(bitmap, -1)) < 0) + return g_strdup(""); start = prev = cur; while (prev >= 0) { -- 2.48.1
Decrease scope of temporary variables so that they don't have to be autofreed and VIR_FREE()d at the same time. Remove unneeded checks and temporary variables. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 49555efc56..1da59cf83a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18144,8 +18144,6 @@ virDomainResctrlMonDefParse(virDomainDef *def, int rv = -1; int ret = -1; g_autofree xmlNodePtr *nodes = NULL; - g_autofree char *tmp = NULL; - g_autofree char *id = NULL; ctxt->node = node; @@ -18153,6 +18151,8 @@ virDomainResctrlMonDefParse(virDomainDef *def, goto cleanup; for (i = 0; i < n; i++) { + g_autofree char *id = NULL; + domresmon = g_new0(virDomainResctrlMonDef, 1); domresmon->tag = tag; @@ -18188,12 +18188,9 @@ virDomainResctrlMonDefParse(virDomainDef *def, * associated allocation, set monitor's id to the same value * as the allocation. */ if (rv == 1) { - const char *alloc_id = virResctrlAllocGetID(resctrl->alloc); - - id = g_strdup(alloc_id); + id = g_strdup(virResctrlAllocGetID(resctrl->alloc)); } else { - if (!(tmp = virBitmapFormat(domresmon->vcpus))) - goto cleanup; + g_autofree char *tmp = virBitmapFormat(domresmon->vcpus); id = g_strdup_printf("vcpus_%s", tmp); } @@ -18204,9 +18201,6 @@ virDomainResctrlMonDefParse(virDomainDef *def, goto cleanup; VIR_APPEND_ELEMENT(resctrl->monitors, resctrl->nmonitors, domresmon); - - VIR_FREE(id); - VIR_FREE(tmp); } ret = 0; -- 2.48.1
Use g_autofree for the temporary variables, remove error checks for virBitmapFormat and simplify formatting of multiple attributes. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1da59cf83a..c79831fe0b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27479,47 +27479,35 @@ virDomainCputuneDefFormat(virBuffer *buf, def->cputune.iothread_quota); for (i = 0; i < def->maxvcpus; i++) { - char *cpumask; + g_autofree char *cpumask = NULL; virDomainVcpuDef *vcpu = def->vcpus[i]; if (!vcpu->cpumask) continue; - if (!(cpumask = virBitmapFormat(vcpu->cpumask))) - return -1; + cpumask = virBitmapFormat(vcpu->cpumask); virBufferAsprintf(&childrenBuf, "<vcpupin vcpu='%zu' cpuset='%s'/>\n", i, cpumask); - - VIR_FREE(cpumask); } if (def->cputune.emulatorpin) { - char *cpumask; - virBufferAddLit(&childrenBuf, "<emulatorpin "); - - if (!(cpumask = virBitmapFormat(def->cputune.emulatorpin))) - return -1; + g_autofree char *cpumask = virBitmapFormat(def->cputune.emulatorpin); - virBufferAsprintf(&childrenBuf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + virBufferAsprintf(&childrenBuf, "<emulatorpin cpuset='%s'/>\n", cpumask); } for (i = 0; i < def->niothreadids; i++) { - char *cpumask; + g_autofree char *cpumask = NULL; /* Ignore iothreadids with no cpumask */ if (!def->iothreadids[i]->cpumask) continue; - virBufferAsprintf(&childrenBuf, "<iothreadpin iothread='%u' ", - def->iothreadids[i]->iothread_id); - - if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask))) - return -1; + cpumask = virBitmapFormat(def->iothreadids[i]->cpumask); - virBufferAsprintf(&childrenBuf, "cpuset='%s'/>\n", cpumask); - VIR_FREE(cpumask); + virBufferAsprintf(&childrenBuf, "<iothreadpin iothread='%u' cpuset='%s'/>\n", + def->iothreadids[i]->iothread_id, cpumask); } if (def->cputune.emulatorsched) { -- 2.48.1
'virBitmapFormat' always returns a string; remove pointless checks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/ch/ch_driver.c | 3 +-- src/conf/capabilities.c | 9 +-------- src/conf/domain_conf.c | 27 +++++++-------------------- src/conf/numa_conf.c | 18 ++++++------------ src/conf/virnetworkobj.c | 3 --- src/hypervisor/domain_cgroup.c | 6 ++---- src/libxl/libxl_driver.c | 3 +-- src/libxl/xen_common.c | 6 ++---- src/qemu/qemu_command.c | 3 +-- src/qemu/qemu_domain.c | 10 ++++------ src/qemu/qemu_driver.c | 21 ++++++--------------- src/util/vircgroup.c | 5 +---- src/vz/vz_sdk.c | 3 +-- 13 files changed, 33 insertions(+), 84 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index fa6e4edcf6..3bdcf66ebd 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -2083,8 +2083,7 @@ chDomainSetNumaParamsLive(virDomainObj *vm, return -1; /* Ensure the cpuset string is formatted before passing to cgroup */ - if (!(nodeset_str = virBitmapFormat(nodeset))) - return -1; + nodeset_str = virBitmapFormat(nodeset); if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, false, &cgroup_temp) < 0 || diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 3d0602e1b6..1821e36e61 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -816,8 +816,7 @@ virCapsHostNUMACellCPUFormat(virBuffer *buf, if (cpus[j].siblings) { g_autofree char *siblings = NULL; - if (!(siblings = virBitmapFormat(cpus[j].siblings))) - return -1; + siblings = virBitmapFormat(cpus[j].siblings); virBufferAsprintf(&childBuf, " socket_id='%d' die_id='%d' cluster_id='%d' core_id='%d' siblings='%s'", @@ -954,9 +953,6 @@ virCapabilitiesFormatCaches(virBuffer *buf, const char *unit = NULL; unsigned long long short_size = virFormatIntPretty(bank->size, &unit); - if (!cpus_str) - return -1; - /* * Let's just *hope* the size is aligned to KiBs so that it does not * bite is back in the future @@ -1038,9 +1034,6 @@ virCapabilitiesFormatMemoryBandwidth(virBuffer *buf, virResctrlInfoMemBWPerNode *control = &node->control; g_autofree char *cpus_str = virBitmapFormat(node->cpus); - if (!cpus_str) - return -1; - virBufferAsprintf(&attrBuf, " id='%u' cpus='%s'", node->id, cpus_str); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c79831fe0b..7db4368b2a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18223,8 +18223,6 @@ virDomainResctrlNew(xmlNodePtr node, /* We need to format it back because we need to be consistent in the naming * even when users specify some "sub-optimal" string there. */ vcpus_str = virBitmapFormat(vcpus); - if (!vcpus_str) - return NULL; if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) alloc_id = virXMLPropString(node, "id"); @@ -25881,8 +25879,7 @@ virDomainMemorySourceDefFormat(virBuffer *buf, switch (def->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: if (def->source.dimm.nodes) { - if (!(bitmap = virBitmapFormat(def->source.dimm.nodes))) - return -1; + bitmap = virBitmapFormat(def->source.dimm.nodes); virBufferAsprintf(&childBuf, "<nodemask>%s</nodemask>\n", bitmap); } @@ -25893,8 +25890,7 @@ virDomainMemorySourceDefFormat(virBuffer *buf, break; case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM: if (def->source.virtio_mem.nodes) { - if (!(bitmap = virBitmapFormat(def->source.virtio_mem.nodes))) - return -1; + bitmap = virBitmapFormat(def->source.virtio_mem.nodes); virBufferAsprintf(&childBuf, "<nodemask>%s</nodemask>\n", bitmap); } @@ -25921,8 +25917,7 @@ virDomainMemorySourceDefFormat(virBuffer *buf, case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: if (def->source.sgx_epc.nodes) { - if (!(bitmap = virBitmapFormat(def->source.sgx_epc.nodes))) - return -1; + bitmap = virBitmapFormat(def->source.sgx_epc.nodes); virBufferAsprintf(&childBuf, "<nodemask>%s</nodemask>\n", bitmap); } @@ -26992,9 +26987,8 @@ virDomainHugepagesFormatBuf(virBuffer *buf, hugepage->size); if (hugepage->nodemask) { - g_autofree char *nodeset = NULL; - if (!(nodeset = virBitmapFormat(hugepage->nodemask))) - return -1; + g_autofree char *nodeset = virBitmapFormat(hugepage->nodemask); + virBufferAsprintf(buf, " nodeset='%s'", nodeset); } @@ -27321,8 +27315,6 @@ virDomainResctrlMonDefFormatHelper(virDomainResctrlMonDef *domresmon, } vcpus = virBitmapFormat(domresmon->vcpus); - if (!vcpus) - return -1; virBufferAsprintf(buf, "vcpus='%s'/>\n", vcpus); @@ -27356,8 +27348,6 @@ virDomainCachetuneDefFormat(virBuffer *buf, return 0; vcpus = virBitmapFormat(resctrl->vcpus); - if (!vcpus) - return -1; virBufferAsprintf(&attrBuf, " vcpus='%s'", vcpus); @@ -27415,8 +27405,6 @@ virDomainMemorytuneDefFormat(virBuffer *buf, return 0; vcpus = virBitmapFormat(resctrl->vcpus); - if (!vcpus) - return -1; virBufferAsprintf(&attrBuf, " vcpus='%s'", vcpus); @@ -27545,15 +27533,14 @@ virDomainCpuDefFormat(virBuffer *buf, { virDomainVcpuDef *vcpu; size_t i; - g_autofree char *cpumask = NULL; virBufferAddLit(buf, "<vcpu"); virBufferAsprintf(buf, " placement='%s'", virDomainCpuPlacementModeTypeToString(def->placement_mode)); if (def->cpumask && !virBitmapIsAllSet(def->cpumask)) { - if ((cpumask = virBitmapFormat(def->cpumask)) == NULL) - return -1; + g_autofree char *cpumask = virBitmapFormat(def->cpumask); + virBufferAsprintf(buf, " cpuset='%s'", cpumask); } if (virDomainDefHasVcpusOffline(def)) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 0a0e2911f7..6bc54f9405 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -273,7 +273,6 @@ virDomainNumatuneFormatXML(virBuffer *buf, virDomainNuma *numatune) { const char *tmp = NULL; - char *nodeset = NULL; bool nodesetSpecified = false; size_t i = 0; @@ -298,10 +297,9 @@ virDomainNumatuneFormatXML(virBuffer *buf, virBufferAsprintf(buf, "<memory mode='%s' ", tmp); if (numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC) { - if (!(nodeset = virBitmapFormat(numatune->memory.nodeset))) - return -1; + char *nodeset = virBitmapFormat(numatune->memory.nodeset); + virBufferAsprintf(buf, "nodeset='%s'/>\n", nodeset); - VIR_FREE(nodeset); } else if (numatune->memory.placement) { tmp = virDomainNumatunePlacementTypeToString(numatune->memory.placement); virBufferAsprintf(buf, "placement='%s'/>\n", tmp); @@ -310,19 +308,18 @@ virDomainNumatuneFormatXML(virBuffer *buf, for (i = 0; i < numatune->nmem_nodes; i++) { virDomainNumaNode *mem_node = &numatune->mem_nodes[i]; + g_autofree char *nodeset = NULL; if (!mem_node->nodeset) continue; - if (!(nodeset = virBitmapFormat(mem_node->nodeset))) - return -1; + nodeset = virBitmapFormat(mem_node->nodeset); virBufferAsprintf(buf, "<memnode cellid='%zu' mode='%s' nodeset='%s'/>\n", i, virDomainNumatuneMemModeTypeToString(mem_node->mode), nodeset); - VIR_FREE(nodeset); } virBufferAdjustIndent(buf, -2); @@ -461,9 +458,8 @@ virDomainNumatuneMaybeFormatNodeset(virDomainNuma *numatune, cellid) < 0) return -1; - if (nodeset && - !(*mask = virBitmapFormat(nodeset))) - return -1; + if (nodeset) + *mask = virBitmapFormat(nodeset); return 0; } @@ -1010,8 +1006,6 @@ virDomainNumaDefFormatXML(virBuffer *buf, if (cpumask) { g_autofree char *cpustr = virBitmapFormat(cpumask); - if (!cpustr) - return -1; virBufferAsprintf(&attrBuf, " cpus='%s'", cpustr); } virBufferAsprintf(&attrBuf, " memory='%llu'", diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 5abc295506..4405645d6d 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -799,9 +799,6 @@ virNetworkObjFormat(virNetworkObj *obj, char *classIdStr = virBitmapFormat(obj->classIdMap); size_t i; - if (!classIdStr) - return NULL; - virBufferAddLit(&buf, "<networkstatus>\n"); virBufferAdjustIndent(&buf, 2); virBufferAsprintf(&buf, "<class_id bitmap='%s'/>\n", classIdStr); diff --git a/src/hypervisor/domain_cgroup.c b/src/hypervisor/domain_cgroup.c index 2b9a1cc9b1..fda495faf5 100644 --- a/src/hypervisor/domain_cgroup.c +++ b/src/hypervisor/domain_cgroup.c @@ -410,8 +410,7 @@ virDomainCgroupRestoreCgroupState(virDomainObj *vm, if (!(all_nodes = virNumaGetHostMemoryNodeset())) goto error; - if (!(mem_mask = virBitmapFormat(all_nodes))) - goto error; + mem_mask = virBitmapFormat(all_nodes); if (virCgroupHasEmptyTasks(cgroup, VIR_CGROUP_CONTROLLER_CPUSET) <= 0) goto error; @@ -648,8 +647,7 @@ virDomainCgroupEmulatorAllNodesAllow(virCgroup *cgroup, if (!(all_nodes = virNumaGetHostMemoryNodeset())) goto cleanup; - if (!(all_nodes_str = virBitmapFormat(all_nodes))) - goto cleanup; + all_nodes_str = virBitmapFormat(all_nodes); data = g_new0(virCgroupEmulatorAllNodesData, 1); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index edf7b37581..512b456bfe 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5051,8 +5051,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom, } } - if (!(nodeset = virBitmapFormat(nodes))) - goto cleanup; + nodeset = virBitmapFormat(nodes); if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET, VIR_TYPED_PARAM_STRING, nodeset) < 0) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 3a64f565f7..b7ec552631 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -2030,10 +2030,8 @@ xenFormatCPUAllocation(virConf *conf, virDomainDef *def) if (xenConfigSetInt(conf, "vcpus", virDomainDefGetVcpus(def)) < 0) return -1; - if ((def->cpumask != NULL) && - ((cpus = virBitmapFormat(def->cpumask)) == NULL)) { - return -1; - } + if (def->cpumask != NULL) + cpus = virBitmapFormat(def->cpumask); if (cpus && xenConfigSetString(conf, "cpus", cpus) < 0) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 54130ac4f0..d898e548e1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7498,8 +7498,7 @@ qemuBuildNumaCPUs(virBuffer *buf, if (!cpu) return 0; - if (!(cpumask = virBitmapFormat(cpu))) - return -1; + cpumask = virBitmapFormat(cpu); for (tmpmask = cpumask; tmpmask; tmpmask = next) { if ((next = strchr(tmpmask, ','))) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index df1ed0223d..a9bcd62123 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2306,13 +2306,11 @@ qemuDomainObjPrivateXMLFormatAutomaticPlacement(virBuffer *buf, if (!priv->autoNodeset && !priv->autoCpuset) return 0; - if (priv->autoNodeset && - !((nodeset = virBitmapFormat(priv->autoNodeset)))) - return -1; + if (priv->autoNodeset) + nodeset = virBitmapFormat(priv->autoNodeset); - if (priv->autoCpuset && - !((cpuset = virBitmapFormat(priv->autoCpuset)))) - return -1; + if (priv->autoCpuset) + cpuset = virBitmapFormat(priv->autoCpuset); virBufferAddLit(buf, "<numad"); virBufferEscapeString(buf, " nodeset='%s'", nodeset); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 174dad64b0..d486169bc8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4300,9 +4300,7 @@ qemuDomainPinVcpuLive(virDomainObj *vm, } tmpmap = virBitmapNewCopy(cpumap); - - if (!(str = virBitmapFormat(cpumap))) - goto cleanup; + str = virBitmapFormat(cpumap); if (vcpuinfo->online) { /* Configure the corresponding cpuset cgroup before set affinity. */ @@ -8233,9 +8231,7 @@ qemuDomainSetNumaParamsLive(virDomainObj *vm, if (!virNumaNodesetIsAvailable(nodeset)) return -1; - /* Ensure the cpuset string is formatted before passing to cgroup */ - if (!(nodeset_str = virBitmapFormat(nodeset))) - return -1; + nodeset_str = virBitmapFormat(nodeset); if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, false, &cgroup_thread) < 0 || @@ -16682,9 +16678,7 @@ qemuDomainGetResctrlMonData(virQEMUDriver *driver, * res.vcpus is assigned with an memory space holding it, * let this newly allocated memory buffer to be freed along with * the free of 'res' */ - if (!(res->vcpus = virBitmapFormat(domresmon->vcpus))) - goto error; - + res->vcpus = virBitmapFormat(domresmon->vcpus); res->name = g_strdup(virResctrlMonitorGetID(monitor)); if (virResctrlMonitorGetStats(monitor, (const char **)features, @@ -18451,9 +18445,7 @@ qemuDomainGetGuestVcpusParams(virTypedParameterPtr *params, #define ADD_BITMAP(name) \ do { \ - g_autofree char *tmp = NULL; \ - if (!(tmp = virBitmapFormat(name))) \ - goto cleanup; \ + g_autofree char *tmp = virBitmapFormat(name); \ if (virTypedParamsAddString(&par, &npar, &maxpar, #name, tmp) < 0) \ goto cleanup; \ } while (0) @@ -18583,10 +18575,9 @@ qemuDomainSetGuestVcpus(virDomainPtr dom, } if (!virBitmapIsAllClear(map)) { - char *tmp = virBitmapFormat(map); + g_autofree char *tmp = virBitmapFormat(map); virReportError(VIR_ERR_INVALID_ARG, - _("guest is missing vCPUs '%1$s'"), NULLSTR(tmp)); - VIR_FREE(tmp); + _("guest is missing vCPUs '%1$s'"), tmp); goto endjob; } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b424e1b5d4..1daa95e178 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3946,10 +3946,7 @@ virCgroupSetupBlkioDeviceWriteBps(virCgroup *cgroup, const char *path, int virCgroupSetupCpusetCpus(virCgroup *cgroup, virBitmap *cpumask) { - g_autofree char *new_cpus = NULL; - - if (!(new_cpus = virBitmapFormat(cpumask))) - return -1; + g_autofree char *new_cpus = virBitmapFormat(cpumask); if (virCgroupSetCpusetCpus(cgroup, new_cpus) < 0) return -1; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index b20d454fb8..b5a78d3668 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3903,8 +3903,7 @@ prlsdkDoApplyConfig(struct _vzDriver *driver, pret = PrlVmCfg_SetCpuCount(sdkdom, virDomainDefGetVcpus(def)); prlsdkCheckRetGoto(pret, error); - if (!(mask = virBitmapFormat(def->cpumask))) - goto error; + mask = virBitmapFormat(def->cpumask); pret = PrlVmCfg_SetCpuMask(sdkdom, mask); prlsdkCheckRetGoto(pret, error); -- 2.48.1
On Thu, Feb 20, 2025 at 10:01:24 +0100, Peter Krempa wrote:
'virBitmapFormat' always returns a string; remove pointless checks.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/ch/ch_driver.c | 3 +-- src/conf/capabilities.c | 9 +-------- src/conf/domain_conf.c | 27 +++++++-------------------- src/conf/numa_conf.c | 18 ++++++------------ src/conf/virnetworkobj.c | 3 --- src/hypervisor/domain_cgroup.c | 6 ++---- src/libxl/libxl_driver.c | 3 +-- src/libxl/xen_common.c | 6 ++---- src/qemu/qemu_command.c | 3 +-- src/qemu/qemu_domain.c | 10 ++++------ src/qemu/qemu_driver.c | 21 ++++++--------------- src/util/vircgroup.c | 5 +---- src/vz/vz_sdk.c | 3 +-- 13 files changed, 33 insertions(+), 84 deletions(-)
[...]
@@ -298,10 +297,9 @@ virDomainNumatuneFormatXML(virBuffer *buf, virBufferAsprintf(buf, "<memory mode='%s' ", tmp);
if (numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC) { - if (!(nodeset = virBitmapFormat(numatune->memory.nodeset))) - return -1; + char *nodeset = virBitmapFormat(numatune->memory.nodeset);
This is supposed to be 'g_autofree char *' ...
+ virBufferAsprintf(buf, "nodeset='%s'/>\n", nodeset); - VIR_FREE(nodeset); } else if (numatune->memory.placement) { tmp = virDomainNumatunePlacementTypeToString(numatune->memory.placement); virBufferAsprintf(buf, "placement='%s'/>\n", tmp); @@ -310,19 +308,18 @@ virDomainNumatuneFormatXML(virBuffer *buf,
for (i = 0; i < numatune->nmem_nodes; i++) { virDomainNumaNode *mem_node = &numatune->mem_nodes[i]; + g_autofree char *nodeset = NULL;
... just like here.
if (!mem_node->nodeset) continue;
Consider the above fixed in my branch.
The function can't fail. Remove return value and refactor callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d486169bc8..1b5e7b1d70 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16812,7 +16812,7 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver, } -static int +static void qemuDomainGetStatsCpuCgroup(virDomainObj *dom, virTypedParamList *params) { @@ -16822,7 +16822,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, unsigned long long sys_time = 0; if (!priv->cgroup) - return 0; + return; if (virCgroupGetCpuacctUsage(priv->cgroup, &cpu_time) == 0) virTypedParamListAddULLong(params, cpu_time, "cpu.time"); @@ -16831,8 +16831,6 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, virTypedParamListAddULLong(params, user_time, "cpu.user"); virTypedParamListAddULLong(params, sys_time, "cpu.system"); } - - return 0; } @@ -16955,8 +16953,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, qemuDomainObjPrivate *priv = dom->privateData; if (priv->cgroup) { - if (qemuDomainGetStatsCpuCgroup(dom, params) < 0) - return -1; + qemuDomainGetStatsCpuCgroup(dom, params); } else { if (qemuDomainGetStatsCpuProc(dom, params) < 0) return -1; -- 2.48.1
The function can't fail. Remove return value and refactor callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1b5e7b1d70..a93899ceb5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16834,7 +16834,7 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, } -static int +static void qemuDomainGetStatsCpuProc(virDomainObj *vm, virTypedParamList *params) { @@ -16845,14 +16845,12 @@ qemuDomainGetStatsCpuProc(virDomainObj *vm, if (virProcessGetStatInfo(&cpuTime, &userTime, &sysTime, NULL, NULL, vm->pid, 0) < 0) { /* ignore error */ - return 0; + return; } virTypedParamListAddULLong(params, cpuTime, "cpu.time"); virTypedParamListAddULLong(params, userTime, "cpu.user"); virTypedParamListAddULLong(params, sysTime, "cpu.system"); - - return 0; } @@ -16955,8 +16953,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, if (priv->cgroup) { qemuDomainGetStatsCpuCgroup(dom, params); } else { - if (qemuDomainGetStatsCpuProc(dom, params) < 0) - return -1; + qemuDomainGetStatsCpuProc(dom, params); } if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) -- 2.48.1
The function can't fail. Remove return value and refactor callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a93899ceb5..e2db449a7a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16920,7 +16920,7 @@ qemuDomainGetStatsCpuHaltPollTimeFromStats(virDomainObj *dom, } -static int +static void qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, virTypedParamList *params, unsigned int privflags) @@ -16929,17 +16929,17 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, unsigned long long haltPollFail = 0; if (!virDomainObjIsActive(dom)) - return 0; + return; if (qemuDomainGetStatsCpuHaltPollTimeFromStats(dom, privflags, &haltPollSuccess, &haltPollFail) < 0 && virHostCPUGetHaltPollTime(dom->pid, &haltPollSuccess, &haltPollFail) < 0) - return 0; + return; virTypedParamListAddULLong(params, haltPollSuccess, "cpu.haltpoll.success.time"); virTypedParamListAddULLong(params, haltPollFail, "cpu.haltpoll.fail.time"); - return 0; + return; } static int @@ -16959,8 +16959,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) return -1; - if (qemuDomainGetStatsCpuHaltPollTime(dom, params, privflags) < 0) - return -1; + qemuDomainGetStatsCpuHaltPollTime(dom, params, privflags); return 0; } -- 2.48.1
The bulk domain stats API is meant to collect as much data as possible without erroring out. If fetching of the cache stats fails just skip outputing them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e2db449a7a..efb6e2c454 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16760,7 +16760,7 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriver *driver, } -static int +static void qemuDomainGetStatsCpuCache(virQEMUDriver *driver, virDomainObj *dom, virTypedParamList *params) @@ -16769,14 +16769,16 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver, size_t nresdata = 0; size_t i = 0; size_t j = 0; - int ret = -1; if (!virDomainObjIsActive(dom)) - return 0; + return; if (qemuDomainGetResctrlMonData(driver, dom, &resdata, &nresdata, - VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0) - goto cleanup; + VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0) { + /* don't return cache stats if we can't fetch them */ + virResetLastError(); + return; + } virTypedParamListAddUInt(params, nresdata, "cpu.cache.monitor.count"); @@ -16803,12 +16805,9 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver, } } - ret = 0; - cleanup: for (i = 0; i < nresdata; i++) qemuDomainFreeResctrlMonData(resdata[i]); VIR_FREE(resdata); - return ret; } @@ -16956,8 +16955,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, qemuDomainGetStatsCpuProc(dom, params); } - if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) - return -1; + qemuDomainGetStatsCpuCache(driver, dom, params); qemuDomainGetStatsCpuHaltPollTime(dom, params, privflags); -- 2.48.1
The function didn't comply with libvirt's error reporting scheme as it reported libvirt errors only sometimes. As callers may want to ignore errors convert it to returning -errno on failure instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 +++++- src/util/virperf.c | 19 ++++++++++--------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index efb6e2c454..e5d5c8c484 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17581,9 +17581,13 @@ qemuDomainGetStatsPerfOneEvent(virPerf *perf, virTypedParamList *params) { uint64_t value = 0; + int rv; - if (virPerfReadEvent(perf, type, &value) < 0) + if ((rv = virPerfReadEvent(perf, type, &value)) < 0) { + virReportSystemError(-rv, "%s", + _("Unable to read cache data")); return -1; + } virTypedParamListAddULLong(params, value, "perf.%s", virPerfEventTypeToString(type)); diff --git a/src/util/virperf.c b/src/util/virperf.c index 91f2ca632a..7f0253d5b2 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -290,6 +290,12 @@ bool virPerfEventIsEnabled(virPerf *perf, return perf && perf->events[type].enabled; } + +/** + * virPerfReadEvent: + * + * Returns 0 on success -ERRNO on failure. + */ int virPerfReadEvent(virPerf *perf, virPerfEventType type, @@ -297,13 +303,10 @@ virPerfReadEvent(virPerf *perf, { struct virPerfEvent *event = &perf->events[type]; if (!event->enabled) - return -1; + return -EINVAL; - if (saferead(event->fd, value, sizeof(uint64_t)) < 0) { - virReportSystemError(errno, "%s", - _("Unable to read cache data")); - return -1; - } + if (saferead(event->fd, value, sizeof(uint64_t)) < 0) + return -errno; if (type == VIR_PERF_EVENT_CMT) *value *= event->efields.cmt.scale; @@ -350,9 +353,7 @@ virPerfReadEvent(virPerf *perf G_GNUC_UNUSED, virPerfEventType type G_GNUC_UNUSED, uint64_t *value G_GNUC_UNUSED) { - virReportSystemError(ENXIO, "%s", - _("Perf not supported on this platform")); - return -1; + return -ENOSYS; } #endif -- 2.48.1
The bulk domain stats API is meant to collect as much data as possible without erroring out. Skip the perf stats if we can't fetch them instead of erroring out. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e5d5c8c484..8d413fc4df 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17575,23 +17575,17 @@ qemuDomainGetStatsIOThread(virQEMUDriver *driver G_GNUC_UNUSED, } -static int +static void qemuDomainGetStatsPerfOneEvent(virPerf *perf, virPerfEventType type, virTypedParamList *params) { uint64_t value = 0; - int rv; - if ((rv = virPerfReadEvent(perf, type, &value)) < 0) { - virReportSystemError(-rv, "%s", - _("Unable to read cache data")); - return -1; - } + if (virPerfReadEvent(perf, type, &value) < 0) + return; virTypedParamListAddULLong(params, value, "perf.%s", virPerfEventTypeToString(type)); - - return 0; } static int @@ -17607,8 +17601,7 @@ qemuDomainGetStatsPerf(virQEMUDriver *driver G_GNUC_UNUSED, if (!virPerfEventIsEnabled(priv->perf, i)) continue; - if (qemuDomainGetStatsPerfOneEvent(priv->perf, i, params) < 0) - return -1; + qemuDomainGetStatsPerfOneEvent(priv->perf, i, params); } return 0; -- 2.48.1
The bulk domain stats API is meant to collect as much data as possible without erroring out. Ignore errors from 'qemuDomainGetIOThreadsMon()' and skip the data if an error happens. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8d413fc4df..e513223de2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17531,22 +17531,21 @@ qemuDomainGetStatsIOThread(virQEMUDriver *driver G_GNUC_UNUSED, unsigned int privflags) { size_t i; - qemuMonitorIOThreadInfo **iothreads = NULL; + g_autofree qemuMonitorIOThreadInfo **iothreads = NULL; int niothreads = 0; - int ret = -1; if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) return 0; - if (qemuDomainGetIOThreadsMon(dom, &iothreads, &niothreads) < 0) - return -1; + if (qemuDomainGetIOThreadsMon(dom, &iothreads, &niothreads) < 0) { + virResetLastError(); + return 0; + } /* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must free * it even if it returns 0 */ - if (niothreads == 0) { - ret = 0; - goto cleanup; - } + if (niothreads == 0) + return 0; virTypedParamListAddUInt(params, niothreads, "iothread.count"); @@ -17564,14 +17563,10 @@ qemuDomainGetStatsIOThread(virQEMUDriver *driver G_GNUC_UNUSED, } } - ret = 0; - - cleanup: for (i = 0; i < niothreads; i++) VIR_FREE(iothreads[i]); - VIR_FREE(iothreads); - return ret; + return 0; } -- 2.48.1
The bulk domain stats API is meant to collect as much data as possible without erroring out. If fetching of the memory bandwidth stats fails just skip outputing them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e513223de2..45f286994e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16697,28 +16697,30 @@ qemuDomainGetResctrlMonData(virQEMUDriver *driver, } -static int +static void qemuDomainGetStatsMemoryBandwidth(virQEMUDriver *driver, virDomainObj *dom, virTypedParamList *params) { - virQEMUResctrlMonData **resdata = NULL; + g_autofree virQEMUResctrlMonData **resdata = NULL; char **features = NULL; size_t nresdata = 0; size_t i = 0; size_t j = 0; size_t k = 0; - int ret = -1; if (!virDomainObjIsActive(dom)) - return 0; + return; if (qemuDomainGetResctrlMonData(driver, dom, &resdata, &nresdata, - VIR_RESCTRL_MONITOR_TYPE_MEMBW) < 0) - goto cleanup; + VIR_RESCTRL_MONITOR_TYPE_MEMBW) < 0) { + /* don't return cache stats if we can't fetch them */ + virResetLastError(); + return; + } if (nresdata == 0) - return 0; + return; virTypedParamListAddUInt(params, nresdata, "memory.bandwidth.monitor.count"); @@ -16751,12 +16753,8 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriver *driver, } } - ret = 0; - cleanup: for (i = 0; i < nresdata; i++) qemuDomainFreeResctrlMonData(resdata[i]); - VIR_FREE(resdata); - return ret; } @@ -16970,7 +16968,8 @@ qemuDomainGetStatsMemory(virQEMUDriver *driver, unsigned int privflags G_GNUC_UNUSED) { - return qemuDomainGetStatsMemoryBandwidth(driver, dom, params); + qemuDomainGetStatsMemoryBandwidth(driver, dom, params); + return 0; } -- 2.48.1
The bulk domain stats API is meant to collect as much data as possible without erroring out. If fetching of the dirty rate stats fails just skip outputing them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45f286994e..9e125d8b24 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17601,19 +17601,6 @@ qemuDomainGetStatsPerf(virQEMUDriver *driver G_GNUC_UNUSED, return 0; } -static int -qemuDomainGetStatsDirtyRateMon(virDomainObj *vm, - qemuMonitorDirtyRateInfo *info) -{ - qemuDomainObjPrivate *priv = vm->privateData; - int ret; - - qemuDomainObjEnterMonitor(vm); - ret = qemuMonitorQueryDirtyRate(priv->mon, info); - qemuDomainObjExitMonitor(vm); - - return ret; -} static int qemuDomainGetStatsDirtyRate(virQEMUDriver *driver G_GNUC_UNUSED, @@ -17621,13 +17608,21 @@ qemuDomainGetStatsDirtyRate(virQEMUDriver *driver G_GNUC_UNUSED, virTypedParamList *params, unsigned int privflags) { + qemuDomainObjPrivate *priv = dom->privateData; qemuMonitorDirtyRateInfo info; + int rv; if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) return 0; - if (qemuDomainGetStatsDirtyRateMon(dom, &info) < 0) - return -1; + qemuDomainObjEnterMonitor(dom); + rv = qemuMonitorQueryDirtyRate(priv->mon, &info); + qemuDomainObjExitMonitor(dom); + + if (rv < 0) { + virResetLastError(); + return 0; + } virTypedParamListAddInt(params, info.status, "dirtyrate.calc_status"); virTypedParamListAddLLong(params, info.startTime, "dirtyrate.calc_start_time"); -- 2.48.1
The presence of a return value made it seem that it's expected to fail on errors which is not the case. The function is designed to skip anything it can't fill and not fail when fetching individual stats. Convert the workers to void to make it clear that it's expected not to fail. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 73 +++++++++++++++--------------------------- 1 file changed, 25 insertions(+), 48 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9e125d8b24..80c918312b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16556,7 +16556,7 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, } -static int +static void qemuDomainGetStatsState(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *dom, virTypedParamList *params, @@ -16564,8 +16564,6 @@ qemuDomainGetStatsState(virQEMUDriver *driver G_GNUC_UNUSED, { virTypedParamListAddInt(params, dom->state.state, "state.state"); virTypedParamListAddInt(params, dom->state.reason, "state.reason"); - - return 0; } @@ -16939,7 +16937,7 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, return; } -static int +static void qemuDomainGetStatsCpu(virQEMUDriver *driver, virDomainObj *dom, virTypedParamList *params, @@ -16956,12 +16954,10 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, qemuDomainGetStatsCpuCache(driver, dom, params); qemuDomainGetStatsCpuHaltPollTime(dom, params, privflags); - - return 0; } -static int +static void qemuDomainGetStatsMemory(virQEMUDriver *driver, virDomainObj *dom, virTypedParamList *params, @@ -16969,11 +16965,10 @@ qemuDomainGetStatsMemory(virQEMUDriver *driver, { qemuDomainGetStatsMemoryBandwidth(driver, dom, params); - return 0; } -static int +static void qemuDomainGetStatsBalloon(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *dom, virTypedParamList *params, @@ -16994,12 +16989,12 @@ qemuDomainGetStatsBalloon(virQEMUDriver *driver G_GNUC_UNUSED, virTypedParamListAddULLong(params, virDomainDefGetMemoryTotal(dom->def), "balloon.maximum"); if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) - return 0; + return; nr_stats = qemuDomainMemoryStatsInternal(dom, stats, VIR_DOMAIN_MEMORY_STAT_NR); if (nr_stats < 0) - return 0; + return; #define STORE_MEM_RECORD(TAG, NAME) \ if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_ ##TAG) \ @@ -17021,8 +17016,6 @@ qemuDomainGetStatsBalloon(virQEMUDriver *driver G_GNUC_UNUSED, } #undef STORE_MEM_RECORD - - return 0; } @@ -17085,7 +17078,7 @@ qemuDomainAddStatsFromHashTable(GHashTable *stats, } -static int +static void qemuDomainGetStatsVcpu(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *dom, virTypedParamList *params, @@ -17118,7 +17111,7 @@ qemuDomainGetStatsVcpu(virQEMUDriver *driver G_GNUC_UNUSED, virDomainDefGetVcpus(dom->def), NULL, 0) < 0) { virResetLastError(); - return 0; + return; } if (HAVE_JOB(privflags) && qemuDomainRefreshStatsSchema(dom) == 0) { @@ -17163,15 +17156,13 @@ qemuDomainGetStatsVcpu(virQEMUDriver *driver G_GNUC_UNUSED, qemuDomainAddStatsFromHashTable(stats, priv->statsSchema, prefix, params); } - - return 0; } #define QEMU_ADD_NET_PARAM(params, num, name, value) \ if (value >= 0)\ virTypedParamListAddULLong((params), (value), "net.%zu.%s", (num), (name)); -static int +static void qemuDomainGetStatsInterface(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *dom, virTypedParamList *params, @@ -17180,7 +17171,7 @@ qemuDomainGetStatsInterface(virQEMUDriver *driver G_GNUC_UNUSED, size_t i; if (!virDomainObjIsActive(dom)) - return 0; + return; virTypedParamListAddUInt(params, dom->def->nnets, "net.count"); @@ -17219,8 +17210,6 @@ qemuDomainGetStatsInterface(virQEMUDriver *driver G_GNUC_UNUSED, QEMU_ADD_NET_PARAM(params, i, "tx.errs", tmp.tx_errs); QEMU_ADD_NET_PARAM(params, i, "tx.drop", tmp.tx_drop); } - - return 0; } #undef QEMU_ADD_NET_PARAM @@ -17481,7 +17470,7 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDef *disk, } -static int +static void qemuDomainGetStatsBlock(virQEMUDriver *driver, virDomainObj *dom, virTypedParamList *params, @@ -17518,12 +17507,10 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, virTypedParamListAddUInt(params, visited, "block.count"); virTypedParamListConcat(params, &blockparams); - - return 0; } -static int +static void qemuDomainGetStatsIOThread(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *dom, virTypedParamList *params, @@ -17534,17 +17521,17 @@ qemuDomainGetStatsIOThread(virQEMUDriver *driver G_GNUC_UNUSED, int niothreads = 0; if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) - return 0; + return; if (qemuDomainGetIOThreadsMon(dom, &iothreads, &niothreads) < 0) { virResetLastError(); - return 0; + return; } /* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must free * it even if it returns 0 */ if (niothreads == 0) - return 0; + return; virTypedParamListAddUInt(params, niothreads, "iothread.count"); @@ -17564,8 +17551,6 @@ qemuDomainGetStatsIOThread(virQEMUDriver *driver G_GNUC_UNUSED, for (i = 0; i < niothreads; i++) VIR_FREE(iothreads[i]); - - return 0; } @@ -17582,7 +17567,7 @@ qemuDomainGetStatsPerfOneEvent(virPerf *perf, virTypedParamListAddULLong(params, value, "perf.%s", virPerfEventTypeToString(type)); } -static int +static void qemuDomainGetStatsPerf(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *dom, virTypedParamList *params, @@ -17597,12 +17582,10 @@ qemuDomainGetStatsPerf(virQEMUDriver *driver G_GNUC_UNUSED, qemuDomainGetStatsPerfOneEvent(priv->perf, i, params); } - - return 0; } -static int +static void qemuDomainGetStatsDirtyRate(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *dom, virTypedParamList *params, @@ -17613,7 +17596,7 @@ qemuDomainGetStatsDirtyRate(virQEMUDriver *driver G_GNUC_UNUSED, int rv; if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) - return 0; + return; qemuDomainObjEnterMonitor(dom); rv = qemuMonitorQueryDirtyRate(priv->mon, &info); @@ -17621,7 +17604,7 @@ qemuDomainGetStatsDirtyRate(virQEMUDriver *driver G_GNUC_UNUSED, if (rv < 0) { virResetLastError(); - return 0; + return; } virTypedParamListAddInt(params, info.status, "dirtyrate.calc_status"); @@ -17642,12 +17625,10 @@ qemuDomainGetStatsDirtyRate(virQEMUDriver *driver G_GNUC_UNUSED, } } } - - return 0; } -static int +static void qemuDomainGetStatsVm(virQEMUDriver *driver G_GNUC_UNUSED, virDomainObj *dom, virTypedParamList *params, @@ -17659,10 +17640,10 @@ qemuDomainGetStatsVm(virQEMUDriver *driver G_GNUC_UNUSED, virJSONValue *stats_obj = NULL; if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) - return 0; + return; if (qemuDomainRefreshStatsSchema(dom) < 0) - return 0; + return; qemuDomainObjEnterMonitor(dom); queried_stats = qemuMonitorQueryStats(priv->mon, @@ -17671,17 +17652,15 @@ qemuDomainGetStatsVm(virQEMUDriver *driver G_GNUC_UNUSED, qemuDomainObjExitMonitor(dom); if (!queried_stats || virJSONValueArraySize(queried_stats) != 1) - return 0; + return; stats_obj = virJSONValueArrayGet(queried_stats, 0); stats = qemuMonitorExtractQueryStats(stats_obj); qemuDomainAddStatsFromHashTable(stats, priv->statsSchema, "vm", params); - - return 0; } -typedef int +typedef void (*qemuDomainGetStatsFunc)(virQEMUDriver *driver, virDomainObj *dom, virTypedParamList *list, @@ -17795,9 +17774,7 @@ qemuDomainGetStats(virConnectPtr conn, for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) { if (stats & qemuDomainGetStatsWorkers[i].stats) { - if (qemuDomainGetStatsWorkers[i].func(conn->privateData, dom, params, - flags) < 0) - return -1; + qemuDomainGetStatsWorkers[i].func(conn->privateData, dom, params, flags); } } -- 2.48.1
On 2/20/25 10:01, Peter Krempa wrote:
The workers of qemuDomainGetStats should not report errors if they can't fetch data; but rather omit the entries. Refactor the code to de-incentivize error reporting.
Peter Krempa (21): qemuDomainGetStatsBlockExportHeader: Remove return value qemuDomainGetStatsBlockExportFrontend: Remove return value qemuDomainGetStatsBlockExportBackendStorage: Remove return value qemuDomainGetStatsOneBlockFallback: Remove return value qemuDomainGetStatsOneBlock: Remove return value qemuDomainStorageAlias: Remove NULL checks from callers qemuDomainGetStatsBlockExportHeader: Remove return value virBitmapFormat: Clarify returned values virDomainResctrlMonDefParse: Refactor temporary variables virDomainCputuneDefFormat: Refactor bitmap formatting virBitmapFormat: Don't check return value qemuDomainGetStatsCpuCgroup: Remove return value qemuDomainGetStatsCpuProc: Remove return value qemuDomainGetStatsCpuHaltPollTime: Remove return value qemuDomainGetStatsCpuCache: Don't error out virPerfReadEvent: Refactor to return -errno on failure qemuDomainGetStatsPerfOneEvent: Ignore erros from 'virPerfReadEvent' qemuDomainGetStatsIOThread: Don't error out if fetching iothread info fails qemuDomainGetStatsMemoryBandwidth: Don't error out qemuDomainGetStatsDirtyRate: Don't error out qemuDomainGetStats: Convert worker functions to void
src/ch/ch_driver.c | 3 +- src/conf/capabilities.c | 9 +- src/conf/domain_conf.c | 69 ++----- src/conf/numa_conf.c | 18 +- src/conf/virnetworkobj.c | 3 - src/hypervisor/domain_cgroup.c | 6 +- src/libxl/libxl_driver.c | 3 +- src/libxl/xen_common.c | 6 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_domain.c | 10 +- src/qemu/qemu_driver.c | 341 ++++++++++++--------------------- src/qemu/qemu_monitor_json.c | 5 +- src/util/virbitmap.c | 11 +- src/util/vircgroup.c | 5 +- src/util/virperf.c | 19 +- src/vz/vz_sdk.c | 3 +- 16 files changed, 178 insertions(+), 336 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On a Thursday in 2025, Peter Krempa wrote:
The workers of qemuDomainGetStats should not report errors if they can't fetch data; but rather omit the entries. Refactor the code to de-incentivize error reporting.
Peter Krempa (21): qemuDomainGetStatsBlockExportHeader: Remove return value qemuDomainGetStatsBlockExportFrontend: Remove return value qemuDomainGetStatsBlockExportBackendStorage: Remove return value qemuDomainGetStatsOneBlockFallback: Remove return value qemuDomainGetStatsOneBlock: Remove return value qemuDomainStorageAlias: Remove NULL checks from callers qemuDomainGetStatsBlockExportHeader: Remove return value virBitmapFormat: Clarify returned values virDomainResctrlMonDefParse: Refactor temporary variables virDomainCputuneDefFormat: Refactor bitmap formatting virBitmapFormat: Don't check return value qemuDomainGetStatsCpuCgroup: Remove return value qemuDomainGetStatsCpuProc: Remove return value qemuDomainGetStatsCpuHaltPollTime: Remove return value qemuDomainGetStatsCpuCache: Don't error out virPerfReadEvent: Refactor to return -errno on failure qemuDomainGetStatsPerfOneEvent: Ignore erros from 'virPerfReadEvent' qemuDomainGetStatsIOThread: Don't error out if fetching iothread info fails qemuDomainGetStatsMemoryBandwidth: Don't error out qemuDomainGetStatsDirtyRate: Don't error out qemuDomainGetStats: Convert worker functions to void
src/ch/ch_driver.c | 3 +- src/conf/capabilities.c | 9 +- src/conf/domain_conf.c | 69 ++----- src/conf/numa_conf.c | 18 +- src/conf/virnetworkobj.c | 3 - src/hypervisor/domain_cgroup.c | 6 +- src/libxl/libxl_driver.c | 3 +- src/libxl/xen_common.c | 6 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_domain.c | 10 +- src/qemu/qemu_driver.c | 341 ++++++++++++--------------------- src/qemu/qemu_monitor_json.c | 5 +- src/util/virbitmap.c | 11 +- src/util/vircgroup.c | 5 +- src/util/virperf.c | 19 +- src/vz/vz_sdk.c | 3 +- 16 files changed, 178 insertions(+), 336 deletions(-)
s/outputing/outputting/ in the commit messages Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Feb 20, 2025 at 10:01:13AM +0100, Peter Krempa wrote:
The workers of qemuDomainGetStats should not report errors if they can't fetch data; but rather omit the entries. Refactor the code to de-incentivize error reporting.
Why shouldn't they report errors ? Can you give an example of a problem seen with error reporting ? Silently dropping data has the problem that its hard for people to realize that anything has gone wrong, and in some cases dropped data will be not distinguishable from unimplemented data. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Feb 24, 2025 at 09:33:51 +0000, Daniel P. Berrangé wrote:
On Thu, Feb 20, 2025 at 10:01:13AM +0100, Peter Krempa wrote:
The workers of qemuDomainGetStats should not report errors if they can't fetch data; but rather omit the entries. Refactor the code to de-incentivize error reporting.
Why shouldn't they report errors ?
Can you give an example of a problem seen with error reporting ? Silently
The API itself allows querying multiple VMs for stats. Now if fetching just one of the stats fields fails you'll get an error instead of returning stats for everything else.
dropping data has the problem that its hard for people to realize that anything has gone wrong, and in some cases dropped data will be not distinguishable from unimplemented data.
The API explicitly documents this behaviour and was intended from the beginning: virConnectGetAllDomainStats: * Note that entire stats groups or individual stat fields may be missing from * the output in case they are not supported by the given hypervisor, are not * applicable for the current state of the guest domain, or their retrieval * was not successful. * * Using 0 for @stats returns all stats groups supported by the given * hypervisor. * * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes * the function return error in case some of the stat types in @stats were * not recognized by the daemon. However, even with this flag, a hypervisor * may omit individual fields within a known group if the information is not * available; as an extreme example, a supported group may produce zero * fields for offline domains if the statistics are meaningful only for a * running domain. *
On Mon, Feb 24, 2025 at 10:42:05AM +0100, Peter Krempa wrote:
On Mon, Feb 24, 2025 at 09:33:51 +0000, Daniel P. Berrangé wrote:
On Thu, Feb 20, 2025 at 10:01:13AM +0100, Peter Krempa wrote:
The workers of qemuDomainGetStats should not report errors if they can't fetch data; but rather omit the entries. Refactor the code to de-incentivize error reporting.
Why shouldn't they report errors ?
Can you give an example of a problem seen with error reporting ? Silently
The API itself allows querying multiple VMs for stats. Now if fetching just one of the stats fields fails you'll get an error instead of returning stats for everything else.
dropping data has the problem that its hard for people to realize that anything has gone wrong, and in some cases dropped data will be not distinguishable from unimplemented data.
The API explicitly documents this behaviour and was intended from the beginning:
Ok, that's good. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Feb 24, 2025 at 10:42:05 +0100, Peter Krempa wrote:
On Mon, Feb 24, 2025 at 09:33:51 +0000, Daniel P. Berrangé wrote:
On Thu, Feb 20, 2025 at 10:01:13AM +0100, Peter Krempa wrote:
The workers of qemuDomainGetStats should not report errors if they can't fetch data; but rather omit the entries. Refactor the code to de-incentivize error reporting.
Why shouldn't they report errors ?
N.B: The error reporting infrastructure that was present and is removed by this patch existed solely for reporting allocation errors when adding the results to the list and the originally implemnted workers never reported errors when fetching actual data; just irrecoverable daemon-side errors. It's just the new workers that did report errors, which was very likely based on the fact that the callback function did require an error code to be returned.
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Prívozník -
Peter Krempa