* [PATCH] kvm tools: Change method of retrieving process name
@ 2011-08-09 10:17 Sasha Levin
2011-08-09 14:53 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2011-08-09 10:17 UTC (permalink / raw)
To: penberg
Cc: psuriset, mingo, asias.hejun, prasadjoshi124, gorcunov, kvm,
Sasha Levin
This patch changes './kvm list' to retrieve process name from
'/proc/<pid>/stat' instead of '/proc/<pid>/comm' as it appears the latter
does not exist by default on several systems.
Reported-by: pradeep <psuriset@linux.vnet.ibm.com>
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
tools/kvm/builtin-list.c | 21 ++++++++++++++-------
1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/tools/kvm/builtin-list.c b/tools/kvm/builtin-list.c
index 2d37ecb..89a0465 100644
--- a/tools/kvm/builtin-list.c
+++ b/tools/kvm/builtin-list.c
@@ -13,25 +13,32 @@
static void print_guest(const char *name, int pid)
{
char proc_name[PATH_MAX];
- char comm[sizeof(PROCESS_NAME)];
- int fd;
+ char *comm = NULL;
+ FILE *fd;
- sprintf(proc_name, "/proc/%d/comm", pid);
- fd = open(proc_name, O_RDONLY);
- if (fd <= 0)
+ sprintf(proc_name, "/proc/%d/stat", pid);
+ fd = fopen(proc_name, "r");
+ if (fd == NULL)
goto cleanup;
- if (read(fd, comm, sizeof(PROCESS_NAME)) == 0)
+ if (fscanf(fd, "%*u (%as)", &comm) == 0)
goto cleanup;
if (strncmp(comm, PROCESS_NAME, strlen(PROCESS_NAME)))
goto cleanup;
printf("%s (PID: %d)\n", name, pid);
- close(fd);
+ free(comm);
+
+ fclose(fd);
return;
cleanup:
+ if (fd)
+ fclose(fd);
+ if (comm)
+ free(comm);
+
kvm__remove_pidfile(name);
}
--
1.7.6
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] kvm tools: Change method of retrieving process name
2011-08-09 10:17 [PATCH] kvm tools: Change method of retrieving process name Sasha Levin
@ 2011-08-09 14:53 ` Ingo Molnar
2011-08-09 14:56 ` Pekka Enberg
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2011-08-09 14:53 UTC (permalink / raw)
To: Sasha Levin; +Cc: penberg, psuriset, asias.hejun, prasadjoshi124, gorcunov, kvm
* Sasha Levin <levinsasha928@gmail.com> wrote:
> This patch changes './kvm list' to retrieve process name from
> '/proc/<pid>/stat' instead of '/proc/<pid>/comm' as it appears the latter
> does not exist by default on several systems.
>
> Reported-by: pradeep <psuriset@linux.vnet.ibm.com>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
> tools/kvm/builtin-list.c | 21 ++++++++++++++-------
> 1 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/tools/kvm/builtin-list.c b/tools/kvm/builtin-list.c
> index 2d37ecb..89a0465 100644
> --- a/tools/kvm/builtin-list.c
> +++ b/tools/kvm/builtin-list.c
> @@ -13,25 +13,32 @@
> static void print_guest(const char *name, int pid)
> {
> char proc_name[PATH_MAX];
> - char comm[sizeof(PROCESS_NAME)];
> - int fd;
> + char *comm = NULL;
> + FILE *fd;
>
> - sprintf(proc_name, "/proc/%d/comm", pid);
> - fd = open(proc_name, O_RDONLY);
> - if (fd <= 0)
> + sprintf(proc_name, "/proc/%d/stat", pid);
> + fd = fopen(proc_name, "r");
> + if (fd == NULL)
> goto cleanup;
No, instead we should fall back to 'stat' if the 'comm' access fails.
The 'stat' field contains a lot more data and is thus slower - while
'comm' only outputs the comm.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kvm tools: Change method of retrieving process name
2011-08-09 14:53 ` Ingo Molnar
@ 2011-08-09 14:56 ` Pekka Enberg
2011-08-09 14:59 ` Ingo Molnar
2011-08-09 15:02 ` Sasha Levin
0 siblings, 2 replies; 6+ messages in thread
From: Pekka Enberg @ 2011-08-09 14:56 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, psuriset, asias.hejun, prasadjoshi124, gorcunov, kvm
On Tue, Aug 9, 2011 at 5:53 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Sasha Levin <levinsasha928@gmail.com> wrote:
>
>> This patch changes './kvm list' to retrieve process name from
>> '/proc/<pid>/stat' instead of '/proc/<pid>/comm' as it appears the latter
>> does not exist by default on several systems.
>>
>> Reported-by: pradeep <psuriset@linux.vnet.ibm.com>
>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>> ---
>> tools/kvm/builtin-list.c | 21 ++++++++++++++-------
>> 1 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/kvm/builtin-list.c b/tools/kvm/builtin-list.c
>> index 2d37ecb..89a0465 100644
>> --- a/tools/kvm/builtin-list.c
>> +++ b/tools/kvm/builtin-list.c
>> @@ -13,25 +13,32 @@
>> static void print_guest(const char *name, int pid)
>> {
>> char proc_name[PATH_MAX];
>> - char comm[sizeof(PROCESS_NAME)];
>> - int fd;
>> + char *comm = NULL;
>> + FILE *fd;
>>
>> - sprintf(proc_name, "/proc/%d/comm", pid);
>> - fd = open(proc_name, O_RDONLY);
>> - if (fd <= 0)
>> + sprintf(proc_name, "/proc/%d/stat", pid);
>> + fd = fopen(proc_name, "r");
>> + if (fd == NULL)
>> goto cleanup;
>
> No, instead we should fall back to 'stat' if the 'comm' access fails.
> The 'stat' field contains a lot more data and is thus slower - while
> 'comm' only outputs the comm.
Doh. I already merged the patch. Sasha, can you please send an
incremental one to fix it up?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kvm tools: Change method of retrieving process name
2011-08-09 14:56 ` Pekka Enberg
@ 2011-08-09 14:59 ` Ingo Molnar
2011-08-09 15:02 ` Sasha Levin
1 sibling, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2011-08-09 14:59 UTC (permalink / raw)
To: Pekka Enberg
Cc: Sasha Levin, psuriset, asias.hejun, prasadjoshi124, gorcunov, kvm
* Pekka Enberg <penberg@kernel.org> wrote:
> On Tue, Aug 9, 2011 at 5:53 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Sasha Levin <levinsasha928@gmail.com> wrote:
> >
> >> This patch changes './kvm list' to retrieve process name from
> >> '/proc/<pid>/stat' instead of '/proc/<pid>/comm' as it appears the latter
> >> does not exist by default on several systems.
> >>
> >> Reported-by: pradeep <psuriset@linux.vnet.ibm.com>
> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> >> ---
> >> tools/kvm/builtin-list.c | 21 ++++++++++++++-------
> >> 1 files changed, 14 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tools/kvm/builtin-list.c b/tools/kvm/builtin-list.c
> >> index 2d37ecb..89a0465 100644
> >> --- a/tools/kvm/builtin-list.c
> >> +++ b/tools/kvm/builtin-list.c
> >> @@ -13,25 +13,32 @@
> >> static void print_guest(const char *name, int pid)
> >> {
> >> char proc_name[PATH_MAX];
> >> - char comm[sizeof(PROCESS_NAME)];
> >> - int fd;
> >> + char *comm = NULL;
> >> + FILE *fd;
> >>
> >> - sprintf(proc_name, "/proc/%d/comm", pid);
> >> - fd = open(proc_name, O_RDONLY);
> >> - if (fd <= 0)
> >> + sprintf(proc_name, "/proc/%d/stat", pid);
> >> + fd = fopen(proc_name, "r");
> >> + if (fd == NULL)
> >> goto cleanup;
> >
> > No, instead we should fall back to 'stat' if the 'comm' access fails.
> > The 'stat' field contains a lot more data and is thus slower - while
> > 'comm' only outputs the comm.
>
> Doh. I already merged the patch. Sasha, can you please send an
> incremental one to fix it up?
well, it's not bad code per se - the speedup probably isnt really
noticeable either - but we should do it nevertheless, out of
principle :)
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kvm tools: Change method of retrieving process name
2011-08-09 14:56 ` Pekka Enberg
2011-08-09 14:59 ` Ingo Molnar
@ 2011-08-09 15:02 ` Sasha Levin
2011-08-09 15:13 ` Pekka Enberg
1 sibling, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2011-08-09 15:02 UTC (permalink / raw)
To: Pekka Enberg
Cc: Ingo Molnar, psuriset, asias.hejun, prasadjoshi124, gorcunov, kvm
On Tue, 2011-08-09 at 17:56 +0300, Pekka Enberg wrote:
> On Tue, Aug 9, 2011 at 5:53 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Sasha Levin <levinsasha928@gmail.com> wrote:
> >
> >> This patch changes './kvm list' to retrieve process name from
> >> '/proc/<pid>/stat' instead of '/proc/<pid>/comm' as it appears the latter
> >> does not exist by default on several systems.
> >>
> >> Reported-by: pradeep <psuriset@linux.vnet.ibm.com>
> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> >> ---
> >> tools/kvm/builtin-list.c | 21 ++++++++++++++-------
> >> 1 files changed, 14 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tools/kvm/builtin-list.c b/tools/kvm/builtin-list.c
> >> index 2d37ecb..89a0465 100644
> >> --- a/tools/kvm/builtin-list.c
> >> +++ b/tools/kvm/builtin-list.c
> >> @@ -13,25 +13,32 @@
> >> static void print_guest(const char *name, int pid)
> >> {
> >> char proc_name[PATH_MAX];
> >> - char comm[sizeof(PROCESS_NAME)];
> >> - int fd;
> >> + char *comm = NULL;
> >> + FILE *fd;
> >>
> >> - sprintf(proc_name, "/proc/%d/comm", pid);
> >> - fd = open(proc_name, O_RDONLY);
> >> - if (fd <= 0)
> >> + sprintf(proc_name, "/proc/%d/stat", pid);
> >> + fd = fopen(proc_name, "r");
> >> + if (fd == NULL)
> >> goto cleanup;
> >
> > No, instead we should fall back to 'stat' if the 'comm' access fails.
> > The 'stat' field contains a lot more data and is thus slower - while
> > 'comm' only outputs the comm.
>
> Doh. I already merged the patch. Sasha, can you please send an
> incremental one to fix it up?
Does it matter when running './kvm list'? The speed difference isn't
really noticeable and it's not something that runs all the time.
--
Sasha.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kvm tools: Change method of retrieving process name
2011-08-09 15:02 ` Sasha Levin
@ 2011-08-09 15:13 ` Pekka Enberg
0 siblings, 0 replies; 6+ messages in thread
From: Pekka Enberg @ 2011-08-09 15:13 UTC (permalink / raw)
To: Sasha Levin
Cc: Ingo Molnar, psuriset, asias.hejun, prasadjoshi124, gorcunov, kvm
On Tue, Aug 9, 2011 at 6:02 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Tue, 2011-08-09 at 17:56 +0300, Pekka Enberg wrote:
>> On Tue, Aug 9, 2011 at 5:53 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Sasha Levin <levinsasha928@gmail.com> wrote:
>> >
>> >> This patch changes './kvm list' to retrieve process name from
>> >> '/proc/<pid>/stat' instead of '/proc/<pid>/comm' as it appears the latter
>> >> does not exist by default on several systems.
>> >>
>> >> Reported-by: pradeep <psuriset@linux.vnet.ibm.com>
>> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>> >> ---
>> >> tools/kvm/builtin-list.c | 21 ++++++++++++++-------
>> >> 1 files changed, 14 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/tools/kvm/builtin-list.c b/tools/kvm/builtin-list.c
>> >> index 2d37ecb..89a0465 100644
>> >> --- a/tools/kvm/builtin-list.c
>> >> +++ b/tools/kvm/builtin-list.c
>> >> @@ -13,25 +13,32 @@
>> >> static void print_guest(const char *name, int pid)
>> >> {
>> >> char proc_name[PATH_MAX];
>> >> - char comm[sizeof(PROCESS_NAME)];
>> >> - int fd;
>> >> + char *comm = NULL;
>> >> + FILE *fd;
>> >>
>> >> - sprintf(proc_name, "/proc/%d/comm", pid);
>> >> - fd = open(proc_name, O_RDONLY);
>> >> - if (fd <= 0)
>> >> + sprintf(proc_name, "/proc/%d/stat", pid);
>> >> + fd = fopen(proc_name, "r");
>> >> + if (fd == NULL)
>> >> goto cleanup;
>> >
>> > No, instead we should fall back to 'stat' if the 'comm' access fails.
>> > The 'stat' field contains a lot more data and is thus slower - while
>> > 'comm' only outputs the comm.
>>
>> Doh. I already merged the patch. Sasha, can you please send an
>> incremental one to fix it up?
>
> Does it matter when running './kvm list'? The speed difference isn't
> really noticeable and it's not something that runs all the time.
What's the downside? I think it's a reasonable optimization to do
although like Ingo already said, it's probably not very noticeable.
But hey, you never know what people will use the code for in the
future.
Pekka
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-08-09 15:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-09 10:17 [PATCH] kvm tools: Change method of retrieving process name Sasha Levin
2011-08-09 14:53 ` Ingo Molnar
2011-08-09 14:56 ` Pekka Enberg
2011-08-09 14:59 ` Ingo Molnar
2011-08-09 15:02 ` Sasha Levin
2011-08-09 15:13 ` Pekka Enberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.