Re: [PATCH v2 2/2] Use virProcessGetStat
On Mon, Nov 22, 2021 at 09:59:07AM +, Daniel P. Berrangé wrote:
On Mon, Nov 22, 2021 at 10:34:38AM +0100, Martin Kletzander wrote:
On Mon, Nov 22, 2021 at 09:18:41AM +, 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
> > ---
> > 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",
> > - , , , ) != 4) {
> > +if (virStrToLong_ullp(proc_stat[13], NULL, 10, ) < 0 ||
> > +virStrToLong_ullp(proc_stat[14], NULL, 10, ) < 0 ||
> > +virStrToLong_l(proc_stat[23], NULL, 10, ) < 0 ||
> > +virStrToLong_i(proc_stat[38], NULL, 10, ) < 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.
BTW ,didn't mean we need constants for all 40+ fields - just the ones
we actually use.
oops, too late, at least it is now complete =)
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 :|
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] Use virProcessGetStat
On Mon, Nov 22, 2021 at 10:34:38AM +0100, Martin Kletzander wrote:
> On Mon, Nov 22, 2021 at 09:18:41AM +, 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
> > > ---
> > > 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",
> > > - , , , ) != 4) {
> > > +if (virStrToLong_ullp(proc_stat[13], NULL, 10, ) < 0 ||
> > > +virStrToLong_ullp(proc_stat[14], NULL, 10, ) < 0 ||
> > > +virStrToLong_l(proc_stat[23], NULL, 10, ) < 0 ||
> > > +virStrToLong_i(proc_stat[38], NULL, 10, ) < 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.
BTW ,didn't mean we need constants for all 40+ fields - just the ones
we actually use.
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 :|
Re: [PATCH v2 2/2] Use virProcessGetStat
On Mon, Nov 22, 2021 at 09:18:41AM +, 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
---
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",
- , , , ) != 4) {
+if (virStrToLong_ullp(proc_stat[13], NULL, 10, ) < 0 ||
+virStrToLong_ullp(proc_stat[14], NULL, 10, ) < 0 ||
+virStrToLong_l(proc_stat[23], NULL, 10, ) < 0 ||
+virStrToLong_i(proc_stat[38], NULL, 10, ) < 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 :|
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] Use virProcessGetStat
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
> ---
> 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",
> - , , , ) != 4) {
> +if (virStrToLong_ullp(proc_stat[13], NULL, 10, ) < 0 ||
> +virStrToLong_ullp(proc_stat[14], NULL, 10, ) < 0 ||
> +virStrToLong_l(proc_stat[23], NULL, 10, ) < 0 ||
> +virStrToLong_i(proc_stat[38], NULL, 10, ) < 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 :|
[PATCH v2 2/2] Use virProcessGetStat
This eliminates one incorrect parsing implementation.
Signed-off-by: Martin Kletzander
---
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",
- , , , ) != 4) {
+if (virStrToLong_ullp(proc_stat[13], NULL, 10, ) < 0 ||
+virStrToLong_ullp(proc_stat[14], NULL, 10, ) < 0 ||
+virStrToLong_l(proc_stat[23], NULL, 10, ) < 0 ||
+virStrToLong_i(proc_stat[38], NULL, 10, ) < 0) {
VIR_WARN("cannot parse process status data");
}
@@ -1450,8 +1431,6 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int
*lastCpu, long *vm_rss,
VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld",
(int)pid, tid, usertime, systime, cpu, rss);
-VIR_FORCE_FCLOSE(pidinfo);
-
return 0;
}
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 73ebcaae422f..4def5ecf5eb3 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1153,56 +1153,20 @@ virProcessSetMaxCoreSize(pid_t pid G_GNUC_UNUSED,
int virProcessGetStartTime(pid_t pid,
unsigned long long *timestamp)
{
-char *tmp;
-int len;
-g_autofree char *filename = NULL;
-g_autofree char *buf = NULL;
-g_auto(GStrv) tokens = NULL;
-
-filename = g_strdup_printf("/proc/%llu/stat", (long long)pid);
-
-if ((len = virFileReadAll(filename, 1024, )) < 0)
-return -1;
+g_auto(GStrv) proc_stat = virProcessGetStat(pid, 0);
-/* start time is the token at index 19 after the '(process name)' entry -
since only this
- * field can contain the ')' character, search backwards for this to avoid
malicious
- * processes trying to fool us
- */
-
-if (!(tmp = strrchr(buf, ')'))) {
+if (!proc_stat || g_strv_length(proc_stat) < 22) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot find start time in %s"),
- filename);
+ _("Cannot find start time for pid %d"), (int)pid);
return -1;
}
-tmp += 2; /* skip ') ' */
-if ((tmp - buf) >= len) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot find start time in %s"),
- filename);
-return -1;
-}
-
-tokens = g_strsplit(tmp, " ", 0);
-if (!tokens ||
-g_strv_length(tokens) < 20) {
+if (virStrToLong_ull(proc_stat[21], NULL, 10, timestamp) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot find start time in %s"),
- filename);
+ _("Cannot parse start time %s for pid %d"),
+ proc_stat[21], (int)pid);
return -1;
}
-
-if (virStrToLong_ull(tokens[19],
- NULL,
- 10,
- timestamp) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot parse start time %s in %s"),
- tokens[19], filename);
-return -1;
-}
-
return 0;
}
#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
--
2.34.0
