Re: [PATCH v2 2/2] Use virProcessGetStat

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(a)redhat.com&gt; > > --- > > 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.