| Message ID | 1312885059-842-1-git-send-email-levinsasha928@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(-)