| Message ID | 20241111143609.185692-1-pvorel@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] syscalls: Add missing WEXITSTATUS() check | expand |
Hi! > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > I haven't found any other test which would need it. > testcases/kernel/syscalls/madvise/madvise08.c | 2 +- > testcases/kernel/syscalls/select/select03.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/syscalls/madvise/madvise08.c b/testcases/kernel/syscalls/madvise/madvise08.c > index 96bcaf159a..cd5ce722a5 100644 > --- a/testcases/kernel/syscalls/madvise/madvise08.c > +++ b/testcases/kernel/syscalls/madvise/madvise08.c > @@ -173,7 +173,7 @@ static pid_t run_child(int advice) > SAFE_WAITPID(pid, &status, 0); > if (WIFSIGNALED(status) && WCOREDUMP(status)) > return pid; > - if (WIFEXITED(status)) > + if (WIFEXITED(status) && !WEXITSTATUS(status)) > return 0; > > tst_res(TCONF, "No coredump produced after signal (%d)", This one is not complete. We do exit(1) if madvise fails that would be previously caught here so we have to add: diff --git a/testcases/kernel/syscalls/madvise/madvise08.c b/testcases/kernel/syscalls/madvise/madvise08.c index 96bcaf159..f8f74ef4f 100644 --- a/testcases/kernel/syscalls/madvise/madvise08.c +++ b/testcases/kernel/syscalls/madvise/madvise08.c @@ -165,7 +165,7 @@ static pid_t run_child(int advice) fmem, FMEMSIZE, advstr); - exit(1); + exit(0); } abort(); } > diff --git a/testcases/kernel/syscalls/select/select03.c b/testcases/kernel/syscalls/select/select03.c > index 1cec3a4c76..216b22104f 100644 > --- a/testcases/kernel/syscalls/select/select03.c > +++ b/testcases/kernel/syscalls/select/select03.c > @@ -77,7 +77,7 @@ static void run(unsigned int n) > > SAFE_WAITPID(pid, &status, 0); > > - if (WIFEXITED(status)) > + if (WIFEXITED(status) && !WEXITSTATUS(status)) > return; > > if (tst_variant == GLIBC_SELECT_VARIANT && > -- > 2.45.2 >
Hi Cyril, > > +++ b/testcases/kernel/syscalls/madvise/madvise08.c > > @@ -173,7 +173,7 @@ static pid_t run_child(int advice) > > SAFE_WAITPID(pid, &status, 0); > > if (WIFSIGNALED(status) && WCOREDUMP(status)) > > return pid; > > - if (WIFEXITED(status)) > > + if (WIFEXITED(status) && !WEXITSTATUS(status)) > > return 0; > > tst_res(TCONF, "No coredump produced after signal (%d)", > This one is not complete. We do exit(1) if madvise fails that would be > previously caught here so we have to add: > diff --git a/testcases/kernel/syscalls/madvise/madvise08.c b/testcases/kernel/syscalls/madvise/madvise08.c > index 96bcaf159..f8f74ef4f 100644 > --- a/testcases/kernel/syscalls/madvise/madvise08.c > +++ b/testcases/kernel/syscalls/madvise/madvise08.c > @@ -165,7 +165,7 @@ static pid_t run_child(int advice) > fmem, > FMEMSIZE, > advstr); > - exit(1); > + exit(0); > } > abort(); > } Good catch, thanks! Maybe it'd be more logical to keep exit(1) (it's error) and check against WEXITSTATUS(status) == 1, but it's a minor detail, let's keep exit(0). Can I merge with your RBT? Kind regards, Petr
Hi! > > > - if (WIFEXITED(status)) > > > + if (WIFEXITED(status) && !WEXITSTATUS(status)) > > > return 0; > > > > tst_res(TCONF, "No coredump produced after signal (%d)", > > > This one is not complete. We do exit(1) if madvise fails that would be > > previously caught here so we have to add: > > > diff --git a/testcases/kernel/syscalls/madvise/madvise08.c b/testcases/kernel/syscalls/madvise/madvise08.c > > index 96bcaf159..f8f74ef4f 100644 > > --- a/testcases/kernel/syscalls/madvise/madvise08.c > > +++ b/testcases/kernel/syscalls/madvise/madvise08.c > > @@ -165,7 +165,7 @@ static pid_t run_child(int advice) > > fmem, > > FMEMSIZE, > > advstr); > > - exit(1); > > + exit(0); > > } > > abort(); > > } > > Good catch, thanks! > > Maybe it'd be more logical to keep exit(1) (it's error) and check against > WEXITSTATUS(status) == 1, but it's a minor detail, let's keep exit(0). I wouldn't do so, as it may confuse people into thinking that the return value actually carries any information, which it does not since the failure has been already reported. I would just stick to exit(0) which makes it more clear that we just need to exit the process, nothing more. > Can I merge with your RBT? Yes, with the fix above.
Hi Cyril, ... > > > --- a/testcases/kernel/syscalls/madvise/madvise08.c > > > +++ b/testcases/kernel/syscalls/madvise/madvise08.c > > > @@ -165,7 +165,7 @@ static pid_t run_child(int advice) > > > fmem, > > > FMEMSIZE, > > > advstr); > > > - exit(1); > > > + exit(0); > > > } > > > abort(); > > > } > > Good catch, thanks! > > Maybe it'd be more logical to keep exit(1) (it's error) and check against > > WEXITSTATUS(status) == 1, but it's a minor detail, let's keep exit(0). > I wouldn't do so, as it may confuse people into thinking that the return > value actually carries any information, which it does not since the > failure has been already reported. I would just stick to exit(0) which > makes it more clear that we just need to exit the process, nothing more. This makes sense, kept it this way. > > Can I merge with your RBT? > Yes, with the fix above. Thanks, merged. Kind regards, Petr
diff --git a/testcases/kernel/syscalls/madvise/madvise08.c b/testcases/kernel/syscalls/madvise/madvise08.c index 96bcaf159a..cd5ce722a5 100644 --- a/testcases/kernel/syscalls/madvise/madvise08.c +++ b/testcases/kernel/syscalls/madvise/madvise08.c @@ -173,7 +173,7 @@ static pid_t run_child(int advice) SAFE_WAITPID(pid, &status, 0); if (WIFSIGNALED(status) && WCOREDUMP(status)) return pid; - if (WIFEXITED(status)) + if (WIFEXITED(status) && !WEXITSTATUS(status)) return 0; tst_res(TCONF, "No coredump produced after signal (%d)", diff --git a/testcases/kernel/syscalls/select/select03.c b/testcases/kernel/syscalls/select/select03.c index 1cec3a4c76..216b22104f 100644 --- a/testcases/kernel/syscalls/select/select03.c +++ b/testcases/kernel/syscalls/select/select03.c @@ -77,7 +77,7 @@ static void run(unsigned int n) SAFE_WAITPID(pid, &status, 0); - if (WIFEXITED(status)) + if (WIFEXITED(status) && !WEXITSTATUS(status)) return; if (tst_variant == GLIBC_SELECT_VARIANT &&
Signed-off-by: Petr Vorel <pvorel@suse.cz> --- I haven't found any other test which would need it. testcases/kernel/syscalls/madvise/madvise08.c | 2 +- testcases/kernel/syscalls/select/select03.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)