| Message ID | 20240222153648.2563-1-andrea.cervesato@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] Add stat04 test | expand |
> From: Andrea Cervesato <andrea.cervesato@suse.com> > This test has been extracted from symlink01 test and it checks that > stat() executed on file provide the same information of symlink linking > to it. > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > --- > free() tmpdir > rename link_stat into link > rename path_stat into path > runtest/smoketest | 2 +- > runtest/syscalls | 4 +- > testcases/kernel/syscalls/stat/.gitignore | 2 + > testcases/kernel/syscalls/stat/stat04.c | 50 +++++++++++++++++++++++ > 4 files changed, 55 insertions(+), 3 deletions(-) > create mode 100644 testcases/kernel/syscalls/stat/stat04.c > diff --git a/runtest/smoketest b/runtest/smoketest > index 83eebfe7b..5608417f9 100644 > --- a/runtest/smoketest > +++ b/runtest/smoketest > @@ -8,7 +8,7 @@ time01 time01 > wait02 wait02 > write01 write01 > symlink01 symlink01 > -stat04 symlink01 -T stat04 > +stat04 stat04 > utime01A symlink01 -T utime01 > rename01A symlink01 -T rename01 > splice02 splice02 -s 20 > diff --git a/runtest/syscalls b/runtest/syscalls > index 7794f1465..ef90076e4 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -1529,8 +1529,8 @@ stat02 stat02 > stat02_64 stat02_64 > stat03 stat03 > stat03_64 stat03_64 > -stat04 symlink01 -T stat04 > -stat04_64 symlink01 -T stat04_64 > +stat04 stat04 > +stat04_64 stat04_64 > statfs01 statfs01 > statfs01_64 statfs01_64 > diff --git a/testcases/kernel/syscalls/stat/.gitignore b/testcases/kernel/syscalls/stat/.gitignore > index fa0a4ce9f..0a62dc6ee 100644 > --- a/testcases/kernel/syscalls/stat/.gitignore > +++ b/testcases/kernel/syscalls/stat/.gitignore > @@ -4,3 +4,5 @@ > /stat02_64 > /stat03 > /stat03_64 > +/stat04 > +/stat04_64 > diff --git a/testcases/kernel/syscalls/stat/stat04.c b/testcases/kernel/syscalls/stat/stat04.c > new file mode 100644 > index 000000000..aebfacf5a > --- /dev/null > +++ b/testcases/kernel/syscalls/stat/stat04.c > @@ -0,0 +1,50 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2000 Silicon Graphics, Inc. All Rights Reserved. > + * Author: David Fenner > + * Copilot: Jon Hendrickson very nit: * Author: David Fenner, Jon Hendrickson > + * Copyright (C) 2024 Andrea Cervesato <andrea.cervesato@suse.com> > + */ > + > +/*\ > + * [Description] > + * > + * This test checks that stat() executed on file provide the same information > + * of symlink linking to it. > + */ > + > +#include <stdlib.h> > +#include "tst_test.h" > + > +static void run(void) > +{ > + char *symname = "my_symlink0"; > + char *tmpdir = tst_get_tmpdir(); > + > + SAFE_SYMLINK(tmpdir, symname); > + > + struct stat path; > + struct stat link; nit: maybe define struct at the top of the function? > + > + TST_EXP_PASS(stat(tmpdir, &path)); > + free(tmpdir); If SAFE_SYMLINK() fails, free() will not happen. I wonder if we should bother (we'd have to set it NULL and add a cleanup function). Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr > + > + TST_EXP_PASS(stat(symname, &link)); > + > + TST_EXP_EQ_LI(path.st_dev, link.st_dev); > + TST_EXP_EQ_LI(path.st_mode, link.st_mode); > + TST_EXP_EQ_LI(path.st_nlink, link.st_nlink); > + TST_EXP_EQ_LI(path.st_uid, link.st_uid); > + TST_EXP_EQ_LI(path.st_gid, link.st_gid); > + TST_EXP_EQ_LI(path.st_size, link.st_size); > + TST_EXP_EQ_LI(path.st_atime, link.st_atime); > + TST_EXP_EQ_LI(path.st_mtime, link.st_mtime); > + TST_EXP_EQ_LI(path.st_ctime, link.st_ctime); > + > + SAFE_UNLINK(symname); > +} > + > +static struct tst_test test = { > + .test_all = run, > + .needs_tmpdir = 1, > +};
Hi! > If SAFE_SYMLINK() fails, free() will not happen. I wonder if we should bother > (we'd have to set it NULL and add a cleanup function). There is no point in bothering to deallocate memory if the process is going to call exit() soon anyways. My main complaint was that we allocated memory in run() function, which is obviously a problem for large enough -i...
Hi! On 2/23/24 00:36, Petr Vorel wrote: >> From: Andrea Cervesato <andrea.cervesato@suse.com> >> This test has been extracted from symlink01 test and it checks that >> stat() executed on file provide the same information of symlink linking >> to it. >> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> >> --- >> free() tmpdir >> rename link_stat into link >> rename path_stat into path >> runtest/smoketest | 2 +- >> runtest/syscalls | 4 +- >> testcases/kernel/syscalls/stat/.gitignore | 2 + >> testcases/kernel/syscalls/stat/stat04.c | 50 +++++++++++++++++++++++ >> 4 files changed, 55 insertions(+), 3 deletions(-) >> create mode 100644 testcases/kernel/syscalls/stat/stat04.c >> diff --git a/runtest/smoketest b/runtest/smoketest >> index 83eebfe7b..5608417f9 100644 >> --- a/runtest/smoketest >> +++ b/runtest/smoketest >> @@ -8,7 +8,7 @@ time01 time01 >> wait02 wait02 >> write01 write01 >> symlink01 symlink01 >> -stat04 symlink01 -T stat04 >> +stat04 stat04 >> utime01A symlink01 -T utime01 >> rename01A symlink01 -T rename01 >> splice02 splice02 -s 20 >> diff --git a/runtest/syscalls b/runtest/syscalls >> index 7794f1465..ef90076e4 100644 >> --- a/runtest/syscalls >> +++ b/runtest/syscalls >> @@ -1529,8 +1529,8 @@ stat02 stat02 >> stat02_64 stat02_64 >> stat03 stat03 >> stat03_64 stat03_64 >> -stat04 symlink01 -T stat04 >> -stat04_64 symlink01 -T stat04_64 >> +stat04 stat04 >> +stat04_64 stat04_64 >> statfs01 statfs01 >> statfs01_64 statfs01_64 >> diff --git a/testcases/kernel/syscalls/stat/.gitignore b/testcases/kernel/syscalls/stat/.gitignore >> index fa0a4ce9f..0a62dc6ee 100644 >> --- a/testcases/kernel/syscalls/stat/.gitignore >> +++ b/testcases/kernel/syscalls/stat/.gitignore >> @@ -4,3 +4,5 @@ >> /stat02_64 >> /stat03 >> /stat03_64 >> +/stat04 >> +/stat04_64 >> diff --git a/testcases/kernel/syscalls/stat/stat04.c b/testcases/kernel/syscalls/stat/stat04.c >> new file mode 100644 >> index 000000000..aebfacf5a >> --- /dev/null >> +++ b/testcases/kernel/syscalls/stat/stat04.c >> @@ -0,0 +1,50 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * Copyright (c) 2000 Silicon Graphics, Inc. All Rights Reserved. >> + * Author: David Fenner >> + * Copilot: Jon Hendrickson > very nit: > * Author: David Fenner, Jon Hendrickson > >> + * Copyright (C) 2024 Andrea Cervesato <andrea.cervesato@suse.com> >> + */ >> + >> +/*\ >> + * [Description] >> + * >> + * This test checks that stat() executed on file provide the same information >> + * of symlink linking to it. >> + */ >> + >> +#include <stdlib.h> >> +#include "tst_test.h" >> + >> +static void run(void) >> +{ >> + char *symname = "my_symlink0"; >> + char *tmpdir = tst_get_tmpdir(); >> + >> + SAFE_SYMLINK(tmpdir, symname); >> + >> + struct stat path; >> + struct stat link; > nit: maybe define struct at the top of the function? This is common in the first versions of C, but a good practice is to define variables as close as possible where they are used in order to improve readability. >> + >> + TST_EXP_PASS(stat(tmpdir, &path)); >> + free(tmpdir); > If SAFE_SYMLINK() fails, free() will not happen. I wonder if we should bother > (we'd have to set it NULL and add a cleanup function). > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > Petr > >> + >> + TST_EXP_PASS(stat(symname, &link)); >> + >> + TST_EXP_EQ_LI(path.st_dev, link.st_dev); >> + TST_EXP_EQ_LI(path.st_mode, link.st_mode); >> + TST_EXP_EQ_LI(path.st_nlink, link.st_nlink); >> + TST_EXP_EQ_LI(path.st_uid, link.st_uid); >> + TST_EXP_EQ_LI(path.st_gid, link.st_gid); >> + TST_EXP_EQ_LI(path.st_size, link.st_size); >> + TST_EXP_EQ_LI(path.st_atime, link.st_atime); >> + TST_EXP_EQ_LI(path.st_mtime, link.st_mtime); >> + TST_EXP_EQ_LI(path.st_ctime, link.st_ctime); >> + >> + SAFE_UNLINK(symname); >> +} >> + >> +static struct tst_test test = { >> + .test_all = run, >> + .needs_tmpdir = 1, >> +}; According to Cyril suggestions we are probably done with this patch so it can be merged. Isn't it? Thanks, Andrea
Hi Andrea, > > > + char *symname = "my_symlink0"; > > > + char *tmpdir = tst_get_tmpdir(); > > > + > > > + SAFE_SYMLINK(tmpdir, symname); > > > + > > > + struct stat path; > > > + struct stat link; > > nit: maybe define struct at the top of the function? > This is common in the first versions of C, but a good practice is to define > variables as close as possible where they are used in order to improve > readability. Fair enough. > > > + > > > + TST_EXP_PASS(stat(tmpdir, &path)); > > > + free(tmpdir); > > If SAFE_SYMLINK() fails, free() will not happen. I wonder if we should bother > > (we'd have to set it NULL and add a cleanup function). > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > > Petr ... > According to Cyril suggestions we are probably done with this patch so it > can be merged. Isn't it? Well you free() later in the function. But Cyril's note sounds reasonable for me: "call it once in the test setup() or use "." instead." Could you please do it? Also most of the tests really run tst_get_tmpdir() in the setup() (or cleanup(), there are exceptions (new API: testcases/kernel/syscalls/pathconf/pathconf01.c, old API: testcases/kernel/syscalls/symlink/symlink01.c, testcases/kernel/syscalls/clone/clone02.c), which should be fixed. IMHO tests which call tst_get_tmpdir() in the run() Kind regards, Petr > Thanks, > Andrea
Hi, > On 23 Feb 2024, at 16:47, Petr Vorel <pvorel@suse.cz> wrote: > > Hi Andrea, > >>>> + char *symname = "my_symlink0"; >>>> + char *tmpdir = tst_get_tmpdir(); >>>> + >>>> + SAFE_SYMLINK(tmpdir, symname); >>>> + >>>> + struct stat path; >>>> + struct stat link; >>> nit: maybe define struct at the top of the function? >> This is common in the first versions of C, but a good practice is to define >> variables as close as possible where they are used in order to improve >> readability. > > Fair enough. > >>>> + >>>> + TST_EXP_PASS(stat(tmpdir, &path)); >>>> + free(tmpdir); >>> If SAFE_SYMLINK() fails, free() will not happen. I wonder if we should bother >>> (we'd have to set it NULL and add a cleanup function). > >>> Reviewed-by: Petr Vorel <pvorel@suse.cz> > >>> Kind regards, >>> Petr > > ... > >> According to Cyril suggestions we are probably done with this patch so it >> can be merged. Isn't it? > > Well you free() later in the function. But Cyril's note sounds reasonable for > me: > "call it once in the test setup() or use "." instead." > > Could you please do it? > Makes sense. I think we should adapt all other tests as well because it’s full of memory leakage when it comes to get temporary directory. > Also most of the tests really run tst_get_tmpdir() in the setup() (or cleanup(), > there are exceptions (new API: testcases/kernel/syscalls/pathconf/pathconf01.c, > old API: testcases/kernel/syscalls/symlink/symlink01.c, > testcases/kernel/syscalls/clone/clone02.c), which should be fixed. > IMHO tests which call tst_get_tmpdir() in the run() > > Kind regards, > Petr > >> Thanks, >> Andrea > Regards, Andrea
diff --git a/runtest/smoketest b/runtest/smoketest index 83eebfe7b..5608417f9 100644 --- a/runtest/smoketest +++ b/runtest/smoketest @@ -8,7 +8,7 @@ time01 time01 wait02 wait02 write01 write01 symlink01 symlink01 -stat04 symlink01 -T stat04 +stat04 stat04 utime01A symlink01 -T utime01 rename01A symlink01 -T rename01 splice02 splice02 -s 20 diff --git a/runtest/syscalls b/runtest/syscalls index 7794f1465..ef90076e4 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1529,8 +1529,8 @@ stat02 stat02 stat02_64 stat02_64 stat03 stat03 stat03_64 stat03_64 -stat04 symlink01 -T stat04 -stat04_64 symlink01 -T stat04_64 +stat04 stat04 +stat04_64 stat04_64 statfs01 statfs01 statfs01_64 statfs01_64 diff --git a/testcases/kernel/syscalls/stat/.gitignore b/testcases/kernel/syscalls/stat/.gitignore index fa0a4ce9f..0a62dc6ee 100644 --- a/testcases/kernel/syscalls/stat/.gitignore +++ b/testcases/kernel/syscalls/stat/.gitignore @@ -4,3 +4,5 @@ /stat02_64 /stat03 /stat03_64 +/stat04 +/stat04_64 diff --git a/testcases/kernel/syscalls/stat/stat04.c b/testcases/kernel/syscalls/stat/stat04.c new file mode 100644 index 000000000..aebfacf5a --- /dev/null +++ b/testcases/kernel/syscalls/stat/stat04.c @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2000 Silicon Graphics, Inc. All Rights Reserved. + * Author: David Fenner + * Copilot: Jon Hendrickson + * Copyright (C) 2024 Andrea Cervesato <andrea.cervesato@suse.com> + */ + +/*\ + * [Description] + * + * This test checks that stat() executed on file provide the same information + * of symlink linking to it. + */ + +#include <stdlib.h> +#include "tst_test.h" + +static void run(void) +{ + char *symname = "my_symlink0"; + char *tmpdir = tst_get_tmpdir(); + + SAFE_SYMLINK(tmpdir, symname); + + struct stat path; + struct stat link; + + TST_EXP_PASS(stat(tmpdir, &path)); + free(tmpdir); + + TST_EXP_PASS(stat(symname, &link)); + + TST_EXP_EQ_LI(path.st_dev, link.st_dev); + TST_EXP_EQ_LI(path.st_mode, link.st_mode); + TST_EXP_EQ_LI(path.st_nlink, link.st_nlink); + TST_EXP_EQ_LI(path.st_uid, link.st_uid); + TST_EXP_EQ_LI(path.st_gid, link.st_gid); + TST_EXP_EQ_LI(path.st_size, link.st_size); + TST_EXP_EQ_LI(path.st_atime, link.st_atime); + TST_EXP_EQ_LI(path.st_mtime, link.st_mtime); + TST_EXP_EQ_LI(path.st_ctime, link.st_ctime); + + SAFE_UNLINK(symname); +} + +static struct tst_test test = { + .test_all = run, + .needs_tmpdir = 1, +};