| Message ID | 20200409211529.5269-11-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | more random fixes | expand |
Cc'ing Ludovic in case he can test with Guix-HPC. On 4/9/20 11:15 PM, Alex Bennée wrote: > In the original bug report long files names in Guix caused > /proc/self/stat be truncated without the trailing ") " as specified in > proc manpage which says: > (2) comm %s > The filename of the executable, in parentheses. This > is visible whether or not the executable is swapped > out. > > Additionally it should only be reporting the executable name rather > than the full path. Fix both these failings while cleaning up the code > to use GString to build up the reported values. As the whole function > is cleaned up also adjust the white space to the current coding style. > > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr> > Reported-by: Brice Goglin <Brice.Goglin@inria.fr> > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > linux-user/syscall.c | 43 +++++++++++++++++++------------------------ > 1 file changed, 19 insertions(+), 24 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 6495ddc4cda..674f70e70a5 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd) > { > CPUState *cpu = env_cpu((CPUArchState *)cpu_env); > TaskState *ts = cpu->opaque; > - abi_ulong start_stack = ts->info->start_stack; > + g_autoptr(GString) buf = g_string_new(NULL); > int i; > > for (i = 0; i < 44; i++) { > - char buf[128]; > - int len; > - uint64_t val = 0; > - > - if (i == 0) { > - /* pid */ > - val = getpid(); > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); > - } else if (i == 1) { > - /* app name */ > - snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]); > - } else if (i == 27) { > - /* stack bottom */ > - val = start_stack; > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); > - } else { > - /* for the rest, there is MasterCard */ > - snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' '); > - } > + if (i == 0) { > + /* pid */ > + g_string_printf(buf, FMT_pid " ", getpid()); > + } else if (i == 1) { > + /* app name */ > + gchar *bin = g_strrstr(ts->bprm->argv[0], "/"); > + bin = bin ? bin + 1 : ts->bprm->argv[0]; > + g_string_printf(buf, "(%.15s) ", bin); 15 or 125? 15 seems short. From your previous test I understood it was 124, for sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40"). > + } else if (i == 27) { > + /* stack bottom */ > + g_string_printf(buf, TARGET_ABI_FMT_ld " ", ts->info->start_stack); > + } else { > + /* for the rest, there is MasterCard */ > + g_string_printf(buf, "0%c", i == 43 ? '\n' : ' '); > + } > > - len = strlen(buf); > - if (write(fd, buf, len) != len) { > - return -1; > - } > + if (write(fd, buf->str, buf->len) != buf->len) { > + return -1; > + } > } > > return 0; >
That was by inspection on my system which seems to truncate a lot earlier. It would be nice to find where in the Linux kernel it is output but I failed to grep the relevant function last night. On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <philmd@redhat.com> wrote: > Cc'ing Ludovic in case he can test with Guix-HPC. > > On 4/9/20 11:15 PM, Alex Bennée wrote: > > In the original bug report long files names in Guix caused > > /proc/self/stat be truncated without the trailing ") " as specified in > > proc manpage which says: > > (2) comm %s > > The filename of the executable, in parentheses. This > > is visible whether or not the executable is swapped > > out. > > > > Additionally it should only be reporting the executable name rather > > than the full path. Fix both these failings while cleaning up the code > > to use GString to build up the reported values. As the whole function > > is cleaned up also adjust the white space to the current coding style. > > > > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr> > > Reported-by: Brice Goglin <Brice.Goglin@inria.fr> > > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > > linux-user/syscall.c | 43 +++++++++++++++++++------------------------ > > 1 file changed, 19 insertions(+), 24 deletions(-) > > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > index 6495ddc4cda..674f70e70a5 100644 > > --- a/linux-user/syscall.c > > +++ b/linux-user/syscall.c > > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd) > > { > > CPUState *cpu = env_cpu((CPUArchState *)cpu_env); > > TaskState *ts = cpu->opaque; > > - abi_ulong start_stack = ts->info->start_stack; > > + g_autoptr(GString) buf = g_string_new(NULL); > > int i; > > > > for (i = 0; i < 44; i++) { > > - char buf[128]; > > - int len; > > - uint64_t val = 0; > > - > > - if (i == 0) { > > - /* pid */ > > - val = getpid(); > > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); > > - } else if (i == 1) { > > - /* app name */ > > - snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]); > > - } else if (i == 27) { > > - /* stack bottom */ > > - val = start_stack; > > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); > > - } else { > > - /* for the rest, there is MasterCard */ > > - snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' '); > > - } > > + if (i == 0) { > > + /* pid */ > > + g_string_printf(buf, FMT_pid " ", getpid()); > > + } else if (i == 1) { > > + /* app name */ > > + gchar *bin = g_strrstr(ts->bprm->argv[0], "/"); > > + bin = bin ? bin + 1 : ts->bprm->argv[0]; > > + g_string_printf(buf, "(%.15s) ", bin); > > 15 or 125? 15 seems short. From your previous test I understood it was > 124, for > > sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40"). > > > + } else if (i == 27) { > > + /* stack bottom */ > > + g_string_printf(buf, TARGET_ABI_FMT_ld " ", > ts->info->start_stack); > > + } else { > > + /* for the rest, there is MasterCard */ > > + g_string_printf(buf, "0%c", i == 43 ? '\n' : ' '); > > + } > > > > - len = strlen(buf); > > - if (write(fd, buf, len) != len) { > > - return -1; > > - } > > + if (write(fd, buf->str, buf->len) != buf->len) { > > + return -1; > > + } > > } > > > > return 0; > > > > <div dir="auto">That was by inspection on my system which seems to truncate a lot earlier. It would be nice to find where in the Linux kernel it is output but I failed to grep the relevant function last night.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <<a href="mailto:philmd@redhat.com">philmd@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Cc'ing Ludovic in case he can test with Guix-HPC.<br> <br> On 4/9/20 11:15 PM, Alex Bennée wrote:<br> > In the original bug report long files names in Guix caused<br> > /proc/self/stat be truncated without the trailing ") " as specified in<br> > proc manpage which says:<br> > (2) comm %s<br> > The filename of the executable, in parentheses. This<br> > is visible whether or not the executable is swapped<br> > out.<br> > <br> > Additionally it should only be reporting the executable name rather<br> > than the full path. Fix both these failings while cleaning up the code<br> > to use GString to build up the reported values. As the whole function<br> > is cleaned up also adjust the white space to the current coding style.<br> > <br> > Message-ID: <<a href="mailto:fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr" target="_blank" rel="noreferrer">fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr</a>><br> > Reported-by: Brice Goglin <<a href="mailto:Brice.Goglin@inria.fr" target="_blank" rel="noreferrer">Brice.Goglin@inria.fr</a>><br> > Cc: Philippe_Mathieu-Daudé <<a href="mailto:philmd@redhat.com" target="_blank" rel="noreferrer">philmd@redhat.com</a>><br> > Signed-off-by: Alex Bennée <<a href="mailto:alex.bennee@linaro.org" target="_blank" rel="noreferrer">alex.bennee@linaro.org</a>><br> > ---<br> > linux-user/syscall.c | 43 +++++++++++++++++++------------------------<br> > 1 file changed, 19 insertions(+), 24 deletions(-)<br> > <br> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c<br> > index 6495ddc4cda..674f70e70a5 100644<br> > --- a/linux-user/syscall.c<br> > +++ b/linux-user/syscall.c<br> > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd)<br> > {<br> > CPUState *cpu = env_cpu((CPUArchState *)cpu_env);<br> > TaskState *ts = cpu->opaque;<br> > - abi_ulong start_stack = ts->info->start_stack;<br> > + g_autoptr(GString) buf = g_string_new(NULL);<br> > int i;<br> > <br> > for (i = 0; i < 44; i++) {<br> > - char buf[128];<br> > - int len;<br> > - uint64_t val = 0;<br> > -<br> > - if (i == 0) {<br> > - /* pid */<br> > - val = getpid();<br> > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val);<br> > - } else if (i == 1) {<br> > - /* app name */<br> > - snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);<br> > - } else if (i == 27) {<br> > - /* stack bottom */<br> > - val = start_stack;<br> > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val);<br> > - } else {<br> > - /* for the rest, there is MasterCard */<br> > - snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');<br> > - }<br> > + if (i == 0) {<br> > + /* pid */<br> > + g_string_printf(buf, FMT_pid " ", getpid());<br> > + } else if (i == 1) {<br> > + /* app name */<br> > + gchar *bin = g_strrstr(ts->bprm->argv[0], "/");<br> > + bin = bin ? bin + 1 : ts->bprm->argv[0];<br> > + g_string_printf(buf, "(%.15s) ", bin);<br> <br> 15 or 125? 15 seems short. From your previous test I understood it was <br> 124, for <br> sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40").<br> <br> > + } else if (i == 27) {<br> > + /* stack bottom */<br> > + g_string_printf(buf, TARGET_ABI_FMT_ld " ", ts->info->start_stack);<br> > + } else {<br> > + /* for the rest, there is MasterCard */<br> > + g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');<br> > + }<br> > <br> > - len = strlen(buf);<br> > - if (write(fd, buf, len) != len) {<br> > - return -1;<br> > - }<br> > + if (write(fd, buf->str, buf->len) != buf->len) {<br> > + return -1;<br> > + }<br> > }<br> > <br> > return 0;<br> > <br> <br> </blockquote></div>
On 4/10/20 2:33 PM, Alex Bennée wrote: > That was by inspection on my system which seems to truncate a lot > earlier. It would be nice to find where in the Linux kernel it is output > but I failed to grep the relevant function last night. OK. Patch is correct with this value, so: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <philmd@redhat.com > <mailto:philmd@redhat.com>> wrote: > > Cc'ing Ludovic in case he can test with Guix-HPC. > > On 4/9/20 11:15 PM, Alex Bennée wrote: > > In the original bug report long files names in Guix caused > > /proc/self/stat be truncated without the trailing ") " as > specified in > > proc manpage which says: > > (2) comm %s > > The filename of the executable, in parentheses. This > > is visible whether or not the executable is swapped > > out. > > > > Additionally it should only be reporting the executable name rather > > than the full path. Fix both these failings while cleaning up the > code > > to use GString to build up the reported values. As the whole function > > is cleaned up also adjust the white space to the current coding > style. > > > > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr > <mailto:fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>> > > Reported-by: Brice Goglin <Brice.Goglin@inria.fr > <mailto:Brice.Goglin@inria.fr>> > > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com > <mailto:philmd@redhat.com>> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org > <mailto:alex.bennee@linaro.org>> > > --- > > linux-user/syscall.c | 43 > +++++++++++++++++++------------------------ > > 1 file changed, 19 insertions(+), 24 deletions(-) > > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > index 6495ddc4cda..674f70e70a5 100644 > > --- a/linux-user/syscall.c > > +++ b/linux-user/syscall.c > > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, > int fd) > > { > > CPUState *cpu = env_cpu((CPUArchState *)cpu_env); > > TaskState *ts = cpu->opaque; > > - abi_ulong start_stack = ts->info->start_stack; > > + g_autoptr(GString) buf = g_string_new(NULL); > > int i; > > > > for (i = 0; i < 44; i++) { > > - char buf[128]; > > - int len; > > - uint64_t val = 0; > > - > > - if (i == 0) { > > - /* pid */ > > - val = getpid(); > > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); > > - } else if (i == 1) { > > - /* app name */ > > - snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]); > > - } else if (i == 27) { > > - /* stack bottom */ > > - val = start_stack; > > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); > > - } else { > > - /* for the rest, there is MasterCard */ > > - snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' '); > > - } > > + if (i == 0) { > > + /* pid */ > > + g_string_printf(buf, FMT_pid " ", getpid()); > > + } else if (i == 1) { > > + /* app name */ > > + gchar *bin = g_strrstr(ts->bprm->argv[0], "/"); > > + bin = bin ? bin + 1 : ts->bprm->argv[0]; > > + g_string_printf(buf, "(%.15s) ", bin); > > 15 or 125? 15 seems short. From your previous test I understood it was > 124, for > sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40"). > > > + } else if (i == 27) { > > + /* stack bottom */ > > + g_string_printf(buf, TARGET_ABI_FMT_ld " ", > ts->info->start_stack); > > + } else { > > + /* for the rest, there is MasterCard */ Already 23 years =) > > + g_string_printf(buf, "0%c", i == 43 ? '\n' : ' '); > > + } > > > > - len = strlen(buf); > > - if (write(fd, buf, len) != len) { > > - return -1; > > - } > > + if (write(fd, buf->str, buf->len) != buf->len) { > > + return -1; > > + } > > } > > > > return 0; > > >
Le 10/04/2020 à 14:33, Alex Bennée a écrit : > That was by inspection on my system which seems to truncate a lot > earlier. It would be nice to find where in the Linux kernel it is > output but I failed to grep the relevant function last night. It's in proc/array.c, do_task_stat() calls proc_task_name(). In the end, it seems to use task->tcomm or task->comm which is limited by #define TASK_COMM_LEN 16 Brice > > On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <philmd@redhat.com > <mailto:philmd@redhat.com>> wrote: > > Cc'ing Ludovic in case he can test with Guix-HPC. > > On 4/9/20 11:15 PM, Alex Bennée wrote: > > In the original bug report long files names in Guix caused > > /proc/self/stat be truncated without the trailing ") " as > specified in > > proc manpage which says: > > (2) comm %s > > The filename of the executable, in parentheses. This > > is visible whether or not the executable is swapped > > out. > > > > Additionally it should only be reporting the executable name rather > > than the full path. Fix both these failings while cleaning up > the code > > to use GString to build up the reported values. As the whole > function > > is cleaned up also adjust the white space to the current coding > style. > > > > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr > <mailto:fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>> > > Reported-by: Brice Goglin <Brice.Goglin@inria.fr > <mailto:Brice.Goglin@inria.fr>> > > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com > <mailto:philmd@redhat.com>> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org > <mailto:alex.bennee@linaro.org>> > > --- > > linux-user/syscall.c | 43 > +++++++++++++++++++------------------------ > > 1 file changed, 19 insertions(+), 24 deletions(-) > > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > index 6495ddc4cda..674f70e70a5 100644 > > --- a/linux-user/syscall.c > > +++ b/linux-user/syscall.c > > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, > int fd) > > { > > CPUState *cpu = env_cpu((CPUArchState *)cpu_env); > > TaskState *ts = cpu->opaque; > > - abi_ulong start_stack = ts->info->start_stack; > > + g_autoptr(GString) buf = g_string_new(NULL); > > int i; > > > > for (i = 0; i < 44; i++) { > > - char buf[128]; > > - int len; > > - uint64_t val = 0; > > - > > - if (i == 0) { > > - /* pid */ > > - val = getpid(); > > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); > > - } else if (i == 1) { > > - /* app name */ > > - snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]); > > - } else if (i == 27) { > > - /* stack bottom */ > > - val = start_stack; > > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); > > - } else { > > - /* for the rest, there is MasterCard */ > > - snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' '); > > - } > > + if (i == 0) { > > + /* pid */ > > + g_string_printf(buf, FMT_pid " ", getpid()); > > + } else if (i == 1) { > > + /* app name */ > > + gchar *bin = g_strrstr(ts->bprm->argv[0], "/"); > > + bin = bin ? bin + 1 : ts->bprm->argv[0]; > > + g_string_printf(buf, "(%.15s) ", bin); > > 15 or 125? 15 seems short. From your previous test I understood it > was > 124, for > sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40"). > > > + } else if (i == 27) { > > + /* stack bottom */ > > + g_string_printf(buf, TARGET_ABI_FMT_ld " ", > ts->info->start_stack); > > + } else { > > + /* for the rest, there is MasterCard */ > > + g_string_printf(buf, "0%c", i == 43 ? '\n' : ' '); > > + } > > > > - len = strlen(buf); > > - if (write(fd, buf, len) != len) { > > - return -1; > > - } > > + if (write(fd, buf->str, buf->len) != buf->len) { > > + return -1; > > + } > > } > > > > return 0; > > > <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> <div class="moz-cite-prefix">Le 10/04/2020 à 14:33, Alex Bennée a écrit :<br> </div> <blockquote type="cite" cite="mid:CAHDbmO0iF+_TfBH7ZMQD+AYX3=+xS7N_-q-4=sdkEpbYp+tFEQ@mail.gmail.com"> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> <div dir="auto">That was by inspection on my system which seems to truncate a lot earlier. It would be nice to find where in the Linux kernel it is output but I failed to grep the relevant function last night.</div> </blockquote> <p><br> </p> <p>It's in proc/array.c, do_task_stat() calls proc_task_name(). In the end, it seems to use task->tcomm or task->comm which is limited by <br> </p> <pre>#define TASK_COMM_LEN 16</pre> <p>Brice</p> <p><br> </p> <p><br> </p> <blockquote type="cite" cite="mid:CAHDbmO0iF+_TfBH7ZMQD+AYX3=+xS7N_-q-4=sdkEpbYp+tFEQ@mail.gmail.com"><br> <div class="gmail_quote"> <div dir="ltr" class="gmail_attr">On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <<a href="mailto:philmd@redhat.com" moz-do-not-send="true">philmd@redhat.com</a>> wrote:<br> </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Cc'ing Ludovic in case he can test with Guix-HPC.<br> <br> On 4/9/20 11:15 PM, Alex Bennée wrote:<br> > In the original bug report long files names in Guix caused<br> > /proc/self/stat be truncated without the trailing ") " as specified in<br> > proc manpage which says:<br> > (2) comm %s<br> > The filename of the executable, in parentheses. This<br> > is visible whether or not the executable is swapped<br> > out.<br> > <br> > Additionally it should only be reporting the executable name rather<br> > than the full path. Fix both these failings while cleaning up the code<br> > to use GString to build up the reported values. As the whole function<br> > is cleaned up also adjust the white space to the current coding style.<br> > <br> > Message-ID: <<a href="mailto:fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr" target="_blank" rel="noreferrer" moz-do-not-send="true">fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr</a>><br> > Reported-by: Brice Goglin <<a href="mailto:Brice.Goglin@inria.fr" target="_blank" rel="noreferrer" moz-do-not-send="true">Brice.Goglin@inria.fr</a>><br> > Cc: Philippe_Mathieu-Daudé <<a href="mailto:philmd@redhat.com" target="_blank" rel="noreferrer" moz-do-not-send="true">philmd@redhat.com</a>><br> > Signed-off-by: Alex Bennée <<a href="mailto:alex.bennee@linaro.org" target="_blank" rel="noreferrer" moz-do-not-send="true">alex.bennee@linaro.org</a>><br> > ---<br> > linux-user/syscall.c | 43 +++++++++++++++++++------------------------<br> > 1 file changed, 19 insertions(+), 24 deletions(-)<br> > <br> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c<br> > index 6495ddc4cda..674f70e70a5 100644<br> > --- a/linux-user/syscall.c<br> > +++ b/linux-user/syscall.c<br> > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd)<br> > {<br> > CPUState *cpu = env_cpu((CPUArchState *)cpu_env);<br> > TaskState *ts = cpu->opaque;<br> > - abi_ulong start_stack = ts->info->start_stack;<br> > + g_autoptr(GString) buf = g_string_new(NULL);<br> > int i;<br> > <br> > for (i = 0; i < 44; i++) {<br> > - char buf[128];<br> > - int len;<br> > - uint64_t val = 0;<br> > -<br> > - if (i == 0) {<br> > - /* pid */<br> > - val = getpid();<br> > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val);<br> > - } else if (i == 1) {<br> > - /* app name */<br> > - snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]);<br> > - } else if (i == 27) {<br> > - /* stack bottom */<br> > - val = start_stack;<br> > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val);<br> > - } else {<br> > - /* for the rest, there is MasterCard */<br> > - snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' ');<br> > - }<br> > + if (i == 0) {<br> > + /* pid */<br> > + g_string_printf(buf, FMT_pid " ", getpid());<br> > + } else if (i == 1) {<br> > + /* app name */<br> > + gchar *bin = g_strrstr(ts->bprm->argv[0], "/");<br> > + bin = bin ? bin + 1 : ts->bprm->argv[0];<br> > + g_string_printf(buf, "(%.15s) ", bin);<br> <br> 15 or 125? 15 seems short. From your previous test I understood it was <br> 124, for <br> sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40").<br> <br> > + } else if (i == 27) {<br> > + /* stack bottom */<br> > + g_string_printf(buf, TARGET_ABI_FMT_ld " ", ts->info->start_stack);<br> > + } else {<br> > + /* for the rest, there is MasterCard */<br> > + g_string_printf(buf, "0%c", i == 43 ? '\n' : ' ');<br> > + }<br> > <br> > - len = strlen(buf);<br> > - if (write(fd, buf, len) != len) {<br> > - return -1;<br> > - }<br> > + if (write(fd, buf->str, buf->len) != buf->len) {<br> > + return -1;<br> > + }<br> > }<br> > <br> > return 0;<br> > <br> <br> </blockquote> </div> </blockquote> </body> </html>
On 4/9/20 2:15 PM, Alex Bennée wrote: > In the original bug report long files names in Guix caused > /proc/self/stat be truncated without the trailing ") " as specified in > proc manpage which says: > (2) comm %s > The filename of the executable, in parentheses. This > is visible whether or not the executable is swapped > out. > > Additionally it should only be reporting the executable name rather > than the full path. Fix both these failings while cleaning up the code > to use GString to build up the reported values. As the whole function > is cleaned up also adjust the white space to the current coding style. > > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr> > Reported-by: Brice Goglin <Brice.Goglin@inria.fr> > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > linux-user/syscall.c | 43 +++++++++++++++++++------------------------ > 1 file changed, 19 insertions(+), 24 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Brice Goglin <Brice.Goglin@inria.fr> writes: > Le 10/04/2020 à 14:33, Alex Bennée a écrit : >> That was by inspection on my system which seems to truncate a lot >> earlier. It would be nice to find where in the Linux kernel it is >> output but I failed to grep the relevant function last night. > > > It's in proc/array.c, do_task_stat() calls proc_task_name(). In the end, > it seems to use task->tcomm or task->comm which is limited by > > #define TASK_COMM_LEN 16 Thanks. I'll amend the commit message. Are you happy with the fix on your end? > > Brice > > > >> >> On Fri, 10 Apr 2020, 12:11 Philippe Mathieu-Daudé, <philmd@redhat.com >> <mailto:philmd@redhat.com>> wrote: >> >> Cc'ing Ludovic in case he can test with Guix-HPC. >> >> On 4/9/20 11:15 PM, Alex Bennée wrote: >> > In the original bug report long files names in Guix caused >> > /proc/self/stat be truncated without the trailing ") " as >> specified in >> > proc manpage which says: >> > (2) comm %s >> > The filename of the executable, in parentheses. This >> > is visible whether or not the executable is swapped >> > out. >> > >> > Additionally it should only be reporting the executable name rather >> > than the full path. Fix both these failings while cleaning up >> the code >> > to use GString to build up the reported values. As the whole >> function >> > is cleaned up also adjust the white space to the current coding >> style. >> > >> > Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr >> <mailto:fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr>> >> > Reported-by: Brice Goglin <Brice.Goglin@inria.fr >> <mailto:Brice.Goglin@inria.fr>> >> > Cc: Philippe_Mathieu-Daudé <philmd@redhat.com >> <mailto:philmd@redhat.com>> >> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org >> <mailto:alex.bennee@linaro.org>> >> > --- >> > linux-user/syscall.c | 43 >> +++++++++++++++++++------------------------ >> > 1 file changed, 19 insertions(+), 24 deletions(-) >> > >> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> > index 6495ddc4cda..674f70e70a5 100644 >> > --- a/linux-user/syscall.c >> > +++ b/linux-user/syscall.c >> > @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, >> int fd) >> > { >> > CPUState *cpu = env_cpu((CPUArchState *)cpu_env); >> > TaskState *ts = cpu->opaque; >> > - abi_ulong start_stack = ts->info->start_stack; >> > + g_autoptr(GString) buf = g_string_new(NULL); >> > int i; >> > >> > for (i = 0; i < 44; i++) { >> > - char buf[128]; >> > - int len; >> > - uint64_t val = 0; >> > - >> > - if (i == 0) { >> > - /* pid */ >> > - val = getpid(); >> > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); >> > - } else if (i == 1) { >> > - /* app name */ >> > - snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]); >> > - } else if (i == 27) { >> > - /* stack bottom */ >> > - val = start_stack; >> > - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); >> > - } else { >> > - /* for the rest, there is MasterCard */ >> > - snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' '); >> > - } >> > + if (i == 0) { >> > + /* pid */ >> > + g_string_printf(buf, FMT_pid " ", getpid()); >> > + } else if (i == 1) { >> > + /* app name */ >> > + gchar *bin = g_strrstr(ts->bprm->argv[0], "/"); >> > + bin = bin ? bin + 1 : ts->bprm->argv[0]; >> > + g_string_printf(buf, "(%.15s) ", bin); >> >> 15 or 125? 15 seems short. From your previous test I understood it >> was >> 124, for >> sizeof("cat_with9_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890___40"). >> >> > + } else if (i == 27) { >> > + /* stack bottom */ >> > + g_string_printf(buf, TARGET_ABI_FMT_ld " ", >> ts->info->start_stack); >> > + } else { >> > + /* for the rest, there is MasterCard */ >> > + g_string_printf(buf, "0%c", i == 43 ? '\n' : ' '); >> > + } >> > >> > - len = strlen(buf); >> > - if (write(fd, buf, len) != len) { >> > - return -1; >> > - } >> > + if (write(fd, buf->str, buf->len) != buf->len) { >> > + return -1; >> > + } >> > } >> > >> > return 0; >> > >> -- Alex Bennée
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 6495ddc4cda..674f70e70a5 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7295,34 +7295,29 @@ static int open_self_stat(void *cpu_env, int fd) { CPUState *cpu = env_cpu((CPUArchState *)cpu_env); TaskState *ts = cpu->opaque; - abi_ulong start_stack = ts->info->start_stack; + g_autoptr(GString) buf = g_string_new(NULL); int i; for (i = 0; i < 44; i++) { - char buf[128]; - int len; - uint64_t val = 0; - - if (i == 0) { - /* pid */ - val = getpid(); - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); - } else if (i == 1) { - /* app name */ - snprintf(buf, sizeof(buf), "(%s) ", ts->bprm->argv[0]); - } else if (i == 27) { - /* stack bottom */ - val = start_stack; - snprintf(buf, sizeof(buf), "%"PRId64 " ", val); - } else { - /* for the rest, there is MasterCard */ - snprintf(buf, sizeof(buf), "0%c", i == 43 ? '\n' : ' '); - } + if (i == 0) { + /* pid */ + g_string_printf(buf, FMT_pid " ", getpid()); + } else if (i == 1) { + /* app name */ + gchar *bin = g_strrstr(ts->bprm->argv[0], "/"); + bin = bin ? bin + 1 : ts->bprm->argv[0]; + g_string_printf(buf, "(%.15s) ", bin); + } else if (i == 27) { + /* stack bottom */ + g_string_printf(buf, TARGET_ABI_FMT_ld " ", ts->info->start_stack); + } else { + /* for the rest, there is MasterCard */ + g_string_printf(buf, "0%c", i == 43 ? '\n' : ' '); + } - len = strlen(buf); - if (write(fd, buf, len) != len) { - return -1; - } + if (write(fd, buf->str, buf->len) != buf->len) { + return -1; + } } return 0;
In the original bug report long files names in Guix caused /proc/self/stat be truncated without the trailing ") " as specified in proc manpage which says: (2) comm %s The filename of the executable, in parentheses. This is visible whether or not the executable is swapped out. Additionally it should only be reporting the executable name rather than the full path. Fix both these failings while cleaning up the code to use GString to build up the reported values. As the whole function is cleaned up also adjust the white space to the current coding style. Message-ID: <fb4c55fa-d539-67ee-c6c9-de8fb63c8488@inria.fr> Reported-by: Brice Goglin <Brice.Goglin@inria.fr> Cc: Philippe_Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- linux-user/syscall.c | 43 +++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 24 deletions(-) -- 2.20.1