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