| Message ID | 20240205102653.2789879-1-liwang@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | process_state: Enhancement of process state detection | expand |
Hi Li, Ian, > The functions will be more robust against process names with > unusual characters and will correctly read the state character > from the /proc/[pid]/stat file. This is a necessary change > because the process name, which is a free-form string, can > contain spaces and other characters that would otherwise > disrupt the simple parsing logic of the original format string. > e.g. > $ cat /proc/792442/stat > 792442 (Web Content) S 164213 4351 4351 0 -1 4194560 ... > Reported-by: Ian Wienand <iwienand@redhat.com> > Signed-off-by: Li Wang <liwang@redhat.com> > Cc: Chunyu Hu <chuhu@redhat.com> > --- > lib/tst_process_state.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > diff --git a/lib/tst_process_state.c b/lib/tst_process_state.c > index 08a9d0966..c15283c3d 100644 > --- a/lib/tst_process_state.c > +++ b/lib/tst_process_state.c > @@ -22,7 +22,7 @@ int tst_process_state_wait(const char *file, const int lineno, > for (;;) { > safe_file_scanf(file, lineno, cleanup_fn, proc_path, > - "%*i %*s %c", &cur_state); > + "%*[^)]%*c %c", &cur_state); Obviously correct, thanks. Reviewed-by: Petr Vorel <pvorel@suse.cz> But there is also the same issue in lib/tst_thread_state.c, I guess it applies to that. Li, could you please also fix it before merge? Also, Andrea, you added tst_thread_state_wait() and TST_THREAD_STATE_WAIT() for futex_waitv0[23] related tests [1], but it's now not used anywhere due Jan's changes [2] [3]. I wonder if it's still useful or whether we should remove it. Kind regards, Petr [1] https://lore.kernel.org/ltp/20220209091756.17245-2-andrea.cervesato@suse.de/ [2] https://lore.kernel.org/ltp/6c5b161bc3bcf753cbda92954ca3f47cb268c68f.1663665637.git.jstancek@redhat.com/ [3] https://lore.kernel.org/ltp/6bac7035adc2cfc8ab3800fe1d2d03223ec57ff5.1663662348.git.jstancek@redhat.com/ > if (state == cur_state) > break; > @@ -54,7 +54,7 @@ int tst_process_state_wait2(pid_t pid, const char state) > return 1; > } > - if (fscanf(f, "%*i %*s %c", &cur_state) != 1) { > + if (fscanf(f, "%*[^)]%*c %c", &cur_state) != 1) { > fclose(f); > fprintf(stderr, "Failed to read '%s': %s\n", > proc_path, strerror(errno));
On Tue, Feb 6, 2024 at 12:51 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, Ian, > > > The functions will be more robust against process names with > > unusual characters and will correctly read the state character > > from the /proc/[pid]/stat file. This is a necessary change > > because the process name, which is a free-form string, can > > contain spaces and other characters that would otherwise > > disrupt the simple parsing logic of the original format string. > > > e.g. > > $ cat /proc/792442/stat > > 792442 (Web Content) S 164213 4351 4351 0 -1 4194560 ... > > > Reported-by: Ian Wienand <iwienand@redhat.com> > > Signed-off-by: Li Wang <liwang@redhat.com> > > Cc: Chunyu Hu <chuhu@redhat.com> > > --- > > lib/tst_process_state.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > diff --git a/lib/tst_process_state.c b/lib/tst_process_state.c > > index 08a9d0966..c15283c3d 100644 > > --- a/lib/tst_process_state.c > > +++ b/lib/tst_process_state.c > > @@ -22,7 +22,7 @@ int tst_process_state_wait(const char *file, const int lineno, > > > for (;;) { > > safe_file_scanf(file, lineno, cleanup_fn, proc_path, > > - "%*i %*s %c", &cur_state); > > + "%*[^)]%*c %c", &cur_state); > > Obviously correct, thanks. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> Acked-by: Jan Stancek <jstancek@redhat.com> > > But there is also the same issue in lib/tst_thread_state.c, > I guess it applies to that. Li, could you please also fix it before merge? > > Also, Andrea, you added tst_thread_state_wait() and TST_THREAD_STATE_WAIT() for > futex_waitv0[23] related tests [1], but it's now not used anywhere due Jan's > changes [2] [3]. I wonder if it's still useful or whether we should remove it. I'd keep it, it's a small function and someone could find useful for some new test. > > Kind regards, > Petr > > [1] https://lore.kernel.org/ltp/20220209091756.17245-2-andrea.cervesato@suse.de/ > [2] https://lore.kernel.org/ltp/6c5b161bc3bcf753cbda92954ca3f47cb268c68f.1663665637.git.jstancek@redhat.com/ > [3] https://lore.kernel.org/ltp/6bac7035adc2cfc8ab3800fe1d2d03223ec57ff5.1663662348.git.jstancek@redhat.com/ > > > if (state == cur_state) > > break; > > @@ -54,7 +54,7 @@ int tst_process_state_wait2(pid_t pid, const char state) > > return 1; > > } > > > - if (fscanf(f, "%*i %*s %c", &cur_state) != 1) { > > + if (fscanf(f, "%*[^)]%*c %c", &cur_state) != 1) { > > fclose(f); > > fprintf(stderr, "Failed to read '%s': %s\n", > > proc_path, strerror(errno)); >
On Tue, Feb 6, 2024 at 7:51 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, Ian, > > > The functions will be more robust against process names with > > unusual characters and will correctly read the state character > > from the /proc/[pid]/stat file. This is a necessary change > > because the process name, which is a free-form string, can > > contain spaces and other characters that would otherwise > > disrupt the simple parsing logic of the original format string. > > > e.g. > > $ cat /proc/792442/stat > > 792442 (Web Content) S 164213 4351 4351 0 -1 4194560 ... > > > Reported-by: Ian Wienand <iwienand@redhat.com> > > Signed-off-by: Li Wang <liwang@redhat.com> > > Cc: Chunyu Hu <chuhu@redhat.com> > > --- > > lib/tst_process_state.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > diff --git a/lib/tst_process_state.c b/lib/tst_process_state.c > > index 08a9d0966..c15283c3d 100644 > > --- a/lib/tst_process_state.c > > +++ b/lib/tst_process_state.c > > @@ -22,7 +22,7 @@ int tst_process_state_wait(const char *file, const int > lineno, > > > for (;;) { > > safe_file_scanf(file, lineno, cleanup_fn, proc_path, > > - "%*i %*s %c", &cur_state); > > + "%*[^)]%*c %c", &cur_state); > > Obviously correct, thanks. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > But there is also the same issue in lib/tst_thread_state.c, > I guess it applies to that. Li, could you please also fix it before merge? > Good point, applied with the tst_thread_state change. > > Also, Andrea, you added tst_thread_state_wait() and > TST_THREAD_STATE_WAIT() for > futex_waitv0[23] related tests [1], but it's now not used anywhere due > Jan's > changes [2] [3]. I wonder if it's still useful or whether we should remove > it. > Agreed with Jan, it is still useful in the future.
diff --git a/lib/tst_process_state.c b/lib/tst_process_state.c index 08a9d0966..c15283c3d 100644 --- a/lib/tst_process_state.c +++ b/lib/tst_process_state.c @@ -22,7 +22,7 @@ int tst_process_state_wait(const char *file, const int lineno, for (;;) { safe_file_scanf(file, lineno, cleanup_fn, proc_path, - "%*i %*s %c", &cur_state); + "%*[^)]%*c %c", &cur_state); if (state == cur_state) break; @@ -54,7 +54,7 @@ int tst_process_state_wait2(pid_t pid, const char state) return 1; } - if (fscanf(f, "%*i %*s %c", &cur_state) != 1) { + if (fscanf(f, "%*[^)]%*c %c", &cur_state) != 1) { fclose(f); fprintf(stderr, "Failed to read '%s': %s\n", proc_path, strerror(errno));
The functions will be more robust against process names with unusual characters and will correctly read the state character from the /proc/[pid]/stat file. This is a necessary change because the process name, which is a free-form string, can contain spaces and other characters that would otherwise disrupt the simple parsing logic of the original format string. e.g. $ cat /proc/792442/stat 792442 (Web Content) S 164213 4351 4351 0 -1 4194560 ... Reported-by: Ian Wienand <iwienand@redhat.com> Signed-off-by: Li Wang <liwang@redhat.com> Cc: Chunyu Hu <chuhu@redhat.com> --- lib/tst_process_state.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)