[PATCH v2 2/2] Use virProcessGetStat
Martin Kletzander
mkletzan at redhat.com
Mon Nov 22 09:34:38 UTC 2021
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 at redhat.com>
>> ---
>> 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 :|
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211122/96f14505/attachment-0001.sig>
More information about the libvir-list
mailing list