| Message ID | 20230901144407.364630-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] clone302: Fix short size test | expand |
Hi, On 01. 09. 23 16:44, Petr Vorel wrote: > Test uses sizeof(struct clone_args)-1 as invalid size. Original struct > size 64 was suitable (because > 64 results in EINVAL), but unrelated Nit: <64 > change in 45f6916ba increased the size to 88, thus test failed to due > unexpected clone3() pass. > > Depending on struct clone_args size is not good idea, let's use > arbitrary value 1. This will work, but we could also define a "minimal" struct clone_args (without any fields other than the required ones) and use the size of that (-1 of course). That would give better coverage and leave no untested gap between 1 and minimal struct size. > > Fixes: 1b0e8dd71 ("syscalls/clone3: New tests") > Suggested-by: Martin Doucha <mdoucha@suse.cz> > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Value is really arbitrary, any hint for better value. > Or should we delete the test? > > NOTE: tested only on x86_64. > > Kind regards, > Petr > > testcases/kernel/syscalls/clone3/clone302.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/clone3/clone302.c b/testcases/kernel/syscalls/clone3/clone302.c > index b1b4ccebb..98848e4bc 100644 > --- a/testcases/kernel/syscalls/clone3/clone302.c > +++ b/testcases/kernel/syscalls/clone3/clone302.c > @@ -34,7 +34,7 @@ static struct tcase { > } tcases[] = { > {"invalid args", &invalid_args, sizeof(*valid_args), 0, NULL, SIGCHLD, 0, 0, 0, EFAULT}, > {"zero size", &valid_args, 0, 0, NULL, SIGCHLD, 0, 0, 0, EINVAL}, > - {"short size", &valid_args, sizeof(*valid_args) - 1, 0, NULL, SIGCHLD, 0, 0, 0, EINVAL}, > + {"short size", &valid_args, 1, 0, NULL, SIGCHLD, 0, 0, 0, EINVAL}, > {"extra size", &valid_args, sizeof(*valid_args) + 1, 0, NULL, SIGCHLD, 0, 0, 0, EFAULT}, > {"sighand-no-VM", &valid_args, sizeof(*valid_args), CLONE_SIGHAND, NULL, SIGCHLD, 0, 0, 0, EINVAL}, > {"thread-no-sighand", &valid_args, sizeof(*valid_args), CLONE_THREAD, NULL, SIGCHLD, 0, 0, 0, EINVAL},
Hi Martin, > Hi, > On 01. 09. 23 16:44, Petr Vorel wrote: > > Test uses sizeof(struct clone_args)-1 as invalid size. Original struct > > size 64 was suitable (because > 64 results in EINVAL), but unrelated > Nit: <64 Thanks! (I meant that, but fingers wrote the opposite comparator.) > > change in 45f6916ba increased the size to 88, thus test failed to due > > unexpected clone3() pass. > > Depending on struct clone_args size is not good idea, let's use > > arbitrary value 1. > This will work, but we could also define a "minimal" struct clone_args > (without any fields other than the required ones) and use the size of that > (-1 of course). That would give better coverage and leave no untested gap > between 1 and minimal struct size. Very good idea. I'm setting this as changes requested. Kind regards, Petr
On Fri, Sep 01, 2023 at 07:00:38PM +0200, Petr Vorel wrote: > Hi Martin, > > > Hi, > > > On 01. 09. 23 16:44, Petr Vorel wrote: > > > Test uses sizeof(struct clone_args)-1 as invalid size. Original struct > > > size 64 was suitable (because > 64 results in EINVAL), but unrelated > > > Nit: <64 > > Thanks! (I meant that, but fingers wrote the opposite comparator.) > > > > change in 45f6916ba increased the size to 88, thus test failed to due > > > unexpected clone3() pass. > > > > Depending on struct clone_args size is not good idea, let's use > > > arbitrary value 1. > > > This will work, but we could also define a "minimal" struct clone_args > > (without any fields other than the required ones) and use the size of that > > (-1 of course). That would give better coverage and leave no untested gap > > between 1 and minimal struct size. > > Very good idea. I'm setting this as changes requested. I have made another patch for fix this, hope this can work. https://patchwork.ozlabs.org/project/ltp/patch/20230902055638.14256-1-wegao@suse.com/ > > Kind regards, > Petr > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
diff --git a/testcases/kernel/syscalls/clone3/clone302.c b/testcases/kernel/syscalls/clone3/clone302.c index b1b4ccebb..98848e4bc 100644 --- a/testcases/kernel/syscalls/clone3/clone302.c +++ b/testcases/kernel/syscalls/clone3/clone302.c @@ -34,7 +34,7 @@ static struct tcase { } tcases[] = { {"invalid args", &invalid_args, sizeof(*valid_args), 0, NULL, SIGCHLD, 0, 0, 0, EFAULT}, {"zero size", &valid_args, 0, 0, NULL, SIGCHLD, 0, 0, 0, EINVAL}, - {"short size", &valid_args, sizeof(*valid_args) - 1, 0, NULL, SIGCHLD, 0, 0, 0, EINVAL}, + {"short size", &valid_args, 1, 0, NULL, SIGCHLD, 0, 0, 0, EINVAL}, {"extra size", &valid_args, sizeof(*valid_args) + 1, 0, NULL, SIGCHLD, 0, 0, 0, EFAULT}, {"sighand-no-VM", &valid_args, sizeof(*valid_args), CLONE_SIGHAND, NULL, SIGCHLD, 0, 0, 0, EINVAL}, {"thread-no-sighand", &valid_args, sizeof(*valid_args), CLONE_THREAD, NULL, SIGCHLD, 0, 0, 0, EINVAL},
Test uses sizeof(struct clone_args)-1 as invalid size. Original struct size 64 was suitable (because > 64 results in EINVAL), but unrelated change in 45f6916ba increased the size to 88, thus test failed to due unexpected clone3() pass. Depending on struct clone_args size is not good idea, let's use arbitrary value 1. Fixes: 1b0e8dd71 ("syscalls/clone3: New tests") Suggested-by: Martin Doucha <mdoucha@suse.cz> Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Value is really arbitrary, any hint for better value. Or should we delete the test? NOTE: tested only on x86_64. Kind regards, Petr testcases/kernel/syscalls/clone3/clone302.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)