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

[Martin Kletzander]

On Wed, Jan 18, 2023 at 10:58:18AM +0100, Michal Privoznik wrote:

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).

With this patch virsh dominfo will fail for all running qemu and ch
domains on non-Linux.  Also virDomainGetVcpus in some cases, although
that is (maybe) not used that much?  The question is do we want it to
fail if the strings cannot be parsed or something more sinister than
just the system not being supported?  Maybe just ignoring the error is
fine since that is how it used to work before.

Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
---
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

signature.asc
Description: PGP signature