Re: [PATCH] kvm tools: Change method of retrieving process name

[Ingo Molnar <mingo@elte.hu>]

* 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