Re: [PATCH v2 2/2] Use virProcessGetStat

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Nov 22, 2021 at 09:18:41AM +0000, Daniel P. Berrangé wrote:
On Sun, Nov 21, 2021 at 12:04:26AM +0100, Martin Kletzander wrote:
This eliminates one incorrect parsing implementation.

Please explain what was being done wrongly / what was the
effect of the bug ?


One of the implementations was just looking for first closing
parenthesis to find the end of the command name, which should be done by
looking at the _last_ closing parenthesis.  This might fail in a very
small corner case which is tested for in the first patch.  But you are
right, I should add this to the commit message.  Will do in v3.


Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
---
 src/qemu/qemu_driver.c | 33 ++++++-----------------------
 src/util/virprocess.c  | 48 ++++++------------------------------------
 2 files changed, 12 insertions(+), 69 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d954635dde2a..0468d6aaf314 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1399,36 +1399,17 @@ qemuGetSchedInfo(unsigned long long *cpuWait,

 static int
 qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
-                   pid_t pid, int tid)
+                   pid_t pid, pid_t tid)
 {
-    g_autofree char *proc = NULL;
-    FILE *pidinfo;
+    g_auto(GStrv) proc_stat = virProcessGetStat(pid, tid);
     unsigned long long usertime = 0, systime = 0;
     long rss = 0;
     int cpu = 0;

-    /* In general, we cannot assume pid_t fits in int; but /proc parsing
-     * is specific to Linux where int works fine.  */
-    if (tid)
-        proc = g_strdup_printf("/proc/%d/task/%d/stat", (int)pid, tid);
-    else
-        proc = g_strdup_printf("/proc/%d/stat", (int)pid);
-    if (!proc)
-        return -1;
-
-    pidinfo = fopen(proc, "r");
-
-    /* See 'man proc' for information about what all these fields are. We're
-     * only interested in a very few of them */
-    if (!pidinfo ||
-        fscanf(pidinfo,
-               /* pid -> stime */
-               "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu"
-               /* cutime -> endcode */
-               "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
-               /* startstack -> processor */
-               "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
-               &usertime, &systime, &rss, &cpu) != 4) {
+    if (virStrToLong_ullp(proc_stat[13], NULL, 10, &usertime) < 0 ||
+        virStrToLong_ullp(proc_stat[14], NULL, 10, &systime) < 0 ||
+        virStrToLong_l(proc_stat[23], NULL, 10, &rss) < 0 ||
+        virStrToLong_i(proc_stat[38], NULL, 10, &cpu) < 0) {

Since you're adding a formal API, I think we'd benefit from
constants for these array indexes in virprocess.h


I was thinking about that and also tried figuring out how to encode the
proper field types in the header file.  But since we are not doing lot
of /proc/*/stat parsing in our codebase I though that would be an
overkill.  I'll add at least the constants.

Thanks,
Martin


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 :|

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux