Re: [PATCH v2 2/3] virprocess: Make virProcessGetStatInfo() fail if unable to parse data

Yeah, we've already seen this commit (v8.0.0-rc2~4) and also its revert (v8.1.0-rc1~345). While the original idea was sound, the implementation was less so and it changed behaviour of some public APIs (e.g. whilst getting stats for a running guest was best effort it started to return errors).

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 11 ++++------- src/util/virprocess.c | 28 +++++++--------------------- 2 files changed, 11 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c576c601ad..25c681bfd2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2508,13 +2508,10 @@ qemuDomainGetInfo(virDomainPtr dom, } if (virDomainObjIsActive(vm)) { - if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL, - NULL, NULL, - vm->pid, 0) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("cannot read cputime for domain")); - goto cleanup; - } + /* Specifically ignore the error here. QEMU's PID might be already gone + * even though the check above says it's still active. */ + virProcessGetStatInfo(&(info->cpuTime), NULL, + NULL, NULL, NULL, vm->pid, 0); } if (VIR_ASSIGN_IS_OVERFLOW(info->nrVirtCpu, virDomainDefGetVcpus(vm->def))) { diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 39ca5de811..800af29537 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1761,7 +1761,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime, virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &stime) < 0 || virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { - VIR_WARN("cannot parse process status data"); + return -1; } utime *= jiff2nsec; @@ -1778,7 +1778,6 @@ virProcessGetStatInfo(unsigned long long *cpuTime, if (vm_rss) *vm_rss = rss * virGetSystemPageSizeKB(); - VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld", (int) pid, tid, utime, stime, cpu, rss); @@ -1852,28 +1851,15 @@ virProcessGetSchedInfo(unsigned long long *cpuWait, #else int -virProcessGetStatInfo(unsigned long long *cpuTime, - unsigned long long *userTime, - unsigned long long *sysTime, - int *lastCpu, - long *vm_rss, +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED, + unsigned long long *userTime G_GNUC_UNUSED, + unsigned long long *sysTime G_GNUC_UNUSED, + int *lastCpu G_GNUC_UNUSED, + long *vm_rss G_GNUC_UNUSED, pid_t pid G_GNUC_UNUSED, pid_t tid G_GNUC_UNUSED) { - /* We don't have a way to collect this information on non-Linux - * platforms, so just report neutral values */ - if (cpuTime) - *cpuTime = 0; - if (userTime) - *userTime = 0; - if (sysTime) - *sysTime = 0; - if (lastCpu) - *lastCpu = 0; - if (vm_rss) - *vm_rss = 0; - - return 0; + return -1; } int -- 2.38.2