| Message ID | 20220620133746.99167-1-cristian.marussi@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] Fix mountns01/02/03/04 final umounts using more peculiar dir names | expand |
Hi Cristian Looks good to me, Reviewed-by: Yang Xu <xuyang2018.jy@fujitsu.com> Best Regards Yang Xu > Running LTP20220527 release it appears that the recently re-written tests > mountns02/03/04 can now throw a warning on their final umount attempt in > some setup: > > <<<test_output>>> > tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s > mountns04.c:38: TPASS: unbindable mount passed > tst_device.c:395: TWARN: umount('A') failed with EINVAL > mountns.h:36: TWARN: umount(A) failed: EINVAL (22) > tst_device.c:434: TINFO: No device is mounted at B > > Moreover, the underlying safe_umount() then upgrades the TWARN emitted > from tst_umount to a TBROK, so causing the test to completely fail: > > Summary: > passed 1 > failed 0 > broken 0 > skipped 0 > warnings 2 > <<<execution_status>>> > initiation_status="ok" > duration=0 termination_type=exited termination_id=4 corefile=no > > Even though the final SAFE_UMOUNTs in the test body properly unmount the > test created mountpoints, the final cleanup functions, that finally check > to see if those mountpoints are still mounted, can be fooled into falsely > thinking that test-chosen mountpoints "A" or "B" are still there: this is > due to the fact that the internal helper tst_is_mounted() uses a simple > strstr() on /proc/mounts to check if a directory is still mounted and > clearly the currently test-chosen names are far too much simple, being > one-letter, and they can be easily matched by other unrelated mountpoints > that happen to exist on a specific setup. > > Use a more peculiar naming for the test chosen mountpoints and generalize > accordingly all the comments. > > Cc: Andrea Cervesato<andrea.cervesato@suse.de> > Cc: Cyril Hrubis<chrubis@suse.cz> > Signed-off-by: Cristian Marussi<cristian.marussi@arm.com> > --- > v1 --> v2 > - using more peculiar naming for mountpoints > - fix comments > - dropped previous SAFE_UMONUT removal > > A better, more long term fix should be to fix/harden tst_is_mounted logic, > but looking at mountpoint(1) implementation this is far from trivial to > be done (especially for bind mounts) and it would require a bit of > 're-inventing the wheel' to bring all the mountpoint/libmount helpers and > logic inside LTP; on the other side a dirty and ugly solution based on > something like tst_system("mountpoint -q<dir>") would be less portable > since would add the new mountpoint application as an LTP pre-requisite. > (and so just breaking a few CI probably without having a 'mountpoint-less' > failover mechanism)...so I just generalized the chosen names for now... > --- > testcases/kernel/containers/mountns/mountns.h | 4 ++-- > .../kernel/containers/mountns/mountns01.c | 18 +++++++++--------- > .../kernel/containers/mountns/mountns02.c | 18 +++++++++--------- > .../kernel/containers/mountns/mountns03.c | 18 +++++++++--------- > .../kernel/containers/mountns/mountns04.c | 8 ++++---- > 5 files changed, 33 insertions(+), 33 deletions(-) > > diff --git a/testcases/kernel/containers/mountns/mountns.h b/testcases/kernel/containers/mountns/mountns.h > index ad8befa71..347f0783a 100644 > --- a/testcases/kernel/containers/mountns/mountns.h > +++ b/testcases/kernel/containers/mountns/mountns.h > @@ -10,8 +10,8 @@ > #include "tst_test.h" > #include "lapi/namespaces_constants.h" > > -#define DIRA "A" > -#define DIRB "B" > +#define DIRA "__DIR_A" > +#define DIRB "__DIR_B" > > static int dummy_child(void *v) > { > diff --git a/testcases/kernel/containers/mountns/mountns01.c b/testcases/kernel/containers/mountns/mountns01.c > index 452fe1d10..e99134aba 100644 > --- a/testcases/kernel/containers/mountns/mountns01.c > +++ b/testcases/kernel/containers/mountns/mountns01.c > @@ -12,21 +12,21 @@ > * > * [Algorithm] > * > - * - Creates directories "A", "B" and files "A/A", "B/B" > + * - Creates directories DIR_A, DIR_B and files DIR_A/"A", DIR_B/"B" > * - Unshares mount namespace and makes it private (so mounts/umounts have no > * effect on a real system) > - * - Bind mounts directory "A" to "A" > - * - Makes directory "A" shared > + * - Bind mounts directory DIR_A to DIR_A > + * - Makes directory DIR_A shared > * - Clones a new child process with CLONE_NEWNS flag > * - There are two test cases (where X is parent namespace and Y child namespace): > * 1. First test case > - * .. X: bind mounts "B" to "A" > - * .. Y: must see "A/B" > - * .. X: umounts "A" > + * .. X: bind mounts DIR_B to DIR_A > + * .. Y: must see DIR_A/"B" > + * .. X: umounts DIR_A > * 2. Second test case > - * .. Y: bind mounts "B" to "A" > - * .. X: must see "A/B" > - * .. Y: umounts "A" > + * .. Y: bind mounts DIR_B to DIR_A > + * .. X: must see DIR_A/"B" > + * .. Y: umounts DIR_A > */ > > #include<sys/wait.h> > diff --git a/testcases/kernel/containers/mountns/mountns02.c b/testcases/kernel/containers/mountns/mountns02.c > index cbd435958..258b61217 100644 > --- a/testcases/kernel/containers/mountns/mountns02.c > +++ b/testcases/kernel/containers/mountns/mountns02.c > @@ -12,22 +12,22 @@ > * > * [Algorithm] > * > - * - Creates directories "A", "B" and files "A/A", "B/B" > + * - Creates directories DIR_A, DIR_B and files DIR_A/"A", DIR_B/"B" > * - Unshares mount namespace and makes it private (so mounts/umounts have no > * effect on a real system) > - * - Bind mounts directory "A" to "A" > - * - Makes directory "A" private > + * - Bind mounts directory DIR_A to DIR_A > + * - Makes directory DIR_A private > * - Clones a new child process with CLONE_NEWNS flag > * - There are two test cases (where X is parent namespace and Y child > * namespace): > * 1. First test case > - * .. X: bind mounts "B" to "A" > - * .. Y: must see "A/A" and must not see "A/B" > - * .. X: umounts "A" > + * .. X: bind mounts DIR_B to DIR_A > + * .. Y: must see DIR_A/"A" and must not see DIR_A/"B" > + * .. X: umounts DIR_A > * 2. Second test case > - * .. Y: bind mounts "B" to "A" > - * .. X: must see "A/A" and must not see "A/B" > - * .. Y: umounts A > + * .. Y: bind mounts DIR_B to DIR_A > + * .. X: must see DIR_A/"A" and must not see DIR_A/"B" > + * .. Y: umounts DIRA > */ > > #include<sys/wait.h> > diff --git a/testcases/kernel/containers/mountns/mountns03.c b/testcases/kernel/containers/mountns/mountns03.c > index 5c19a96a9..f37ae7902 100644 > --- a/testcases/kernel/containers/mountns/mountns03.c > +++ b/testcases/kernel/containers/mountns/mountns03.c > @@ -12,24 +12,24 @@ > * > * [Algorithm] > * > - * - Creates directories "A", "B" and files "A/A", "B/B" > + * - Creates directories DIRA, DIRB and files DIRA/"A", DIRB/"B" > * - Unshares mount namespace and makes it private (so mounts/umounts have no > * effect on a real system) > - * - Bind mounts directory "A" to itself > - * - Makes directory "A" shared > + * - Bind mounts directory DIRA to itself > + * - Makes directory DIRA shared > * - Clones a new child process with CLONE_NEWNS flag and makes "A" a slave > * mount > * - There are two testcases (where X is parent namespace and Y child > * namespace): > * 1. First test case > - * .. X: bind mounts "B" to "A" > - * .. Y: must see the file "A/B" > - * .. X: umounts "A" > + * .. X: bind mounts DIRB to DIRA > + * .. Y: must see the file DIRA/"B" > + * .. X: umounts DIRA > * 2. Second test case > - * .. Y: bind mounts "B" to "A" > - * .. X: must see only the "A/A" and must not see "A/B" (as slave mount does > + * .. Y: bind mounts DIRB to DIRA > + * .. X: must see only the DIRA/"A" and must not see DIRA/"B" (as slave mount does > * not forward propagation) > - * .. Y: umounts "A" > + * .. Y: umounts DIRA > */ > > #include<sys/wait.h> > diff --git a/testcases/kernel/containers/mountns/mountns04.c b/testcases/kernel/containers/mountns/mountns04.c > index cc63a03d9..46937ddcd 100644 > --- a/testcases/kernel/containers/mountns/mountns04.c > +++ b/testcases/kernel/containers/mountns/mountns04.c > @@ -10,12 +10,12 @@ > * Tests an unbindable mount: unbindable mount is an unbindable > * private mount. > * > - * - Creates directories "A", "B" and files "A/A", "B/B" > + * - Creates directories DIRA, DIRB and files DIRA/"A", DIRB/"B" > * - Unshares mount namespace and makes it private (so mounts/umounts have no > * effect on a real system) > - * - Bind mounts directory "A" to "A" > - * - Makes directory "A" unbindable > - * - Check if bind mount unbindable "A" to "B" fails as expected > + * - Bind mounts directory DIRA to DIRA > + * - Makes directory DIRA unbindable > + * - Check if bind mount unbindable DIRA to DIRB fails as expected > */ > > #include<sys/wait.h>
Hi, Am 6/20/2022 um 3:37 PM schrieb Cristian Marussi: > Running LTP20220527 release it appears that the recently re-written tests > mountns02/03/04 can now throw a warning on their final umount attempt in > some setup: > > <<<test_output>>> > tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s > mountns04.c:38: TPASS: unbindable mount passed > tst_device.c:395: TWARN: umount('A') failed with EINVAL > mountns.h:36: TWARN: umount(A) failed: EINVAL (22) > tst_device.c:434: TINFO: No device is mounted at B > > Moreover, the underlying safe_umount() then upgrades the TWARN emitted > from tst_umount to a TBROK, so causing the test to completely fail: > > Summary: > passed 1 > failed 0 > broken 0 > skipped 0 > warnings 2 > <<<execution_status>>> > initiation_status="ok" > duration=0 termination_type=exited termination_id=4 corefile=no > > Even though the final SAFE_UMOUNTs in the test body properly unmount the > test created mountpoints, the final cleanup functions, that finally check > to see if those mountpoints are still mounted, can be fooled into falsely > thinking that test-chosen mountpoints "A" or "B" are still there: this is > due to the fact that the internal helper tst_is_mounted() uses a simple > strstr() on /proc/mounts to check if a directory is still mounted and > clearly the currently test-chosen names are far too much simple, being > one-letter, and they can be easily matched by other unrelated mountpoints > that happen to exist on a specific setup. > > Use a more peculiar naming for the test chosen mountpoints and generalize > accordingly all the comments. > > Cc: Andrea Cervesato <andrea.cervesato@suse.de> > Cc: Cyril Hrubis <chrubis@suse.cz> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > v1 --> v2 > - using more peculiar naming for mountpoints > - fix comments > - dropped previous SAFE_UMONUT removal > > A better, more long term fix should be to fix/harden tst_is_mounted logic, > but looking at mountpoint(1) implementation this is far from trivial to > be done (especially for bind mounts) and it would require a bit of > 're-inventing the wheel' to bring all the mountpoint/libmount helpers and > logic inside LTP; on the other side a dirty and ugly solution based on > something like tst_system("mountpoint -q <dir>") would be less portable > since would add the new mountpoint application as an LTP pre-requisite. > (and so just breaking a few CI probably without having a 'mountpoint-less' > failover mechanism)...so I just generalized the chosen names for now... > --- > testcases/kernel/containers/mountns/mountns.h | 4 ++-- > .../kernel/containers/mountns/mountns01.c | 18 +++++++++--------- > .../kernel/containers/mountns/mountns02.c | 18 +++++++++--------- > .../kernel/containers/mountns/mountns03.c | 18 +++++++++--------- > .../kernel/containers/mountns/mountns04.c | 8 ++++---- > 5 files changed, 33 insertions(+), 33 deletions(-) > > diff --git a/testcases/kernel/containers/mountns/mountns.h b/testcases/kernel/containers/mountns/mountns.h > index ad8befa71..347f0783a 100644 > --- a/testcases/kernel/containers/mountns/mountns.h > +++ b/testcases/kernel/containers/mountns/mountns.h > @@ -10,8 +10,8 @@ > #include "tst_test.h" > #include "lapi/namespaces_constants.h" > > -#define DIRA "A" > -#define DIRB "B" > +#define DIRA "__DIR_A" > +#define DIRB "__DIR_B" This is the only non-comment change. How does renaming the directories change anything? Am I missing something? Joerg
Hi, Am 6/21/2022 um 8:51 AM schrieb Joerg Vehlow: >> --- >> testcases/kernel/containers/mountns/mountns.h | 4 ++-- >> .../kernel/containers/mountns/mountns01.c | 18 +++++++++--------- >> .../kernel/containers/mountns/mountns02.c | 18 +++++++++--------- >> .../kernel/containers/mountns/mountns03.c | 18 +++++++++--------- >> .../kernel/containers/mountns/mountns04.c | 8 ++++---- >> 5 files changed, 33 insertions(+), 33 deletions(-) >> >> diff --git a/testcases/kernel/containers/mountns/mountns.h b/testcases/kernel/containers/mountns/mountns.h >> index ad8befa71..347f0783a 100644 >> --- a/testcases/kernel/containers/mountns/mountns.h >> +++ b/testcases/kernel/containers/mountns/mountns.h >> @@ -10,8 +10,8 @@ >> #include "tst_test.h" >> #include "lapi/namespaces_constants.h" >> >> -#define DIRA "A" >> -#define DIRB "B" >> +#define DIRA "__DIR_A" >> +#define DIRB "__DIR_B" > This is the only non-comment change. How does renaming the directories > change anything? Am I missing something? Nevermind, now after I read the v1-thread, I get it.. Joerg
Hi! > >> --- a/testcases/kernel/containers/mountns/mountns.h > >> +++ b/testcases/kernel/containers/mountns/mountns.h > >> @@ -10,8 +10,8 @@ > >> #include "tst_test.h" > >> #include "lapi/namespaces_constants.h" > >> > >> -#define DIRA "A" > >> -#define DIRB "B" > >> +#define DIRA "__DIR_A" > >> +#define DIRB "__DIR_B" > > This is the only non-comment change. How does renaming the directories > > change anything? Am I missing something? > Nevermind, now after I read the v1-thread, I get it.. We tend to prefix globally visible objects with LTP_ in order to make them unique enough and that makes them clearly recognizable as being created by LTP as well. So maybe we should put the LTP string somewhere in the names too.
On Wed, Jun 22, 2022 at 10:35:22AM +0200, Cyril Hrubis wrote: > Hi! > > >> --- a/testcases/kernel/containers/mountns/mountns.h > > >> +++ b/testcases/kernel/containers/mountns/mountns.h > > >> @@ -10,8 +10,8 @@ > > >> #include "tst_test.h" > > >> #include "lapi/namespaces_constants.h" > > >> > > >> -#define DIRA "A" > > >> -#define DIRB "B" > > >> +#define DIRA "__DIR_A" > > >> +#define DIRB "__DIR_B" > > > This is the only non-comment change. How does renaming the directories > > > change anything? Am I missing something? > > Nevermind, now after I read the v1-thread, I get it.. > > We tend to prefix globally visible objects with LTP_ in order to make > them unique enough and that makes them clearly recognizable as being > created by LTP as well. So maybe we should put the LTP string somewhere > in the names too. > Thanks Cyril for the review. I'll do in v3. Thanks, Cristian
diff --git a/testcases/kernel/containers/mountns/mountns.h b/testcases/kernel/containers/mountns/mountns.h index ad8befa71..347f0783a 100644 --- a/testcases/kernel/containers/mountns/mountns.h +++ b/testcases/kernel/containers/mountns/mountns.h @@ -10,8 +10,8 @@ #include "tst_test.h" #include "lapi/namespaces_constants.h" -#define DIRA "A" -#define DIRB "B" +#define DIRA "__DIR_A" +#define DIRB "__DIR_B" static int dummy_child(void *v) { diff --git a/testcases/kernel/containers/mountns/mountns01.c b/testcases/kernel/containers/mountns/mountns01.c index 452fe1d10..e99134aba 100644 --- a/testcases/kernel/containers/mountns/mountns01.c +++ b/testcases/kernel/containers/mountns/mountns01.c @@ -12,21 +12,21 @@ * * [Algorithm] * - * - Creates directories "A", "B" and files "A/A", "B/B" + * - Creates directories DIR_A, DIR_B and files DIR_A/"A", DIR_B/"B" * - Unshares mount namespace and makes it private (so mounts/umounts have no * effect on a real system) - * - Bind mounts directory "A" to "A" - * - Makes directory "A" shared + * - Bind mounts directory DIR_A to DIR_A + * - Makes directory DIR_A shared * - Clones a new child process with CLONE_NEWNS flag * - There are two test cases (where X is parent namespace and Y child namespace): * 1. First test case - * .. X: bind mounts "B" to "A" - * .. Y: must see "A/B" - * .. X: umounts "A" + * .. X: bind mounts DIR_B to DIR_A + * .. Y: must see DIR_A/"B" + * .. X: umounts DIR_A * 2. Second test case - * .. Y: bind mounts "B" to "A" - * .. X: must see "A/B" - * .. Y: umounts "A" + * .. Y: bind mounts DIR_B to DIR_A + * .. X: must see DIR_A/"B" + * .. Y: umounts DIR_A */ #include <sys/wait.h> diff --git a/testcases/kernel/containers/mountns/mountns02.c b/testcases/kernel/containers/mountns/mountns02.c index cbd435958..258b61217 100644 --- a/testcases/kernel/containers/mountns/mountns02.c +++ b/testcases/kernel/containers/mountns/mountns02.c @@ -12,22 +12,22 @@ * * [Algorithm] * - * - Creates directories "A", "B" and files "A/A", "B/B" + * - Creates directories DIR_A, DIR_B and files DIR_A/"A", DIR_B/"B" * - Unshares mount namespace and makes it private (so mounts/umounts have no * effect on a real system) - * - Bind mounts directory "A" to "A" - * - Makes directory "A" private + * - Bind mounts directory DIR_A to DIR_A + * - Makes directory DIR_A private * - Clones a new child process with CLONE_NEWNS flag * - There are two test cases (where X is parent namespace and Y child * namespace): * 1. First test case - * .. X: bind mounts "B" to "A" - * .. Y: must see "A/A" and must not see "A/B" - * .. X: umounts "A" + * .. X: bind mounts DIR_B to DIR_A + * .. Y: must see DIR_A/"A" and must not see DIR_A/"B" + * .. X: umounts DIR_A * 2. Second test case - * .. Y: bind mounts "B" to "A" - * .. X: must see "A/A" and must not see "A/B" - * .. Y: umounts A + * .. Y: bind mounts DIR_B to DIR_A + * .. X: must see DIR_A/"A" and must not see DIR_A/"B" + * .. Y: umounts DIRA */ #include <sys/wait.h> diff --git a/testcases/kernel/containers/mountns/mountns03.c b/testcases/kernel/containers/mountns/mountns03.c index 5c19a96a9..f37ae7902 100644 --- a/testcases/kernel/containers/mountns/mountns03.c +++ b/testcases/kernel/containers/mountns/mountns03.c @@ -12,24 +12,24 @@ * * [Algorithm] * - * - Creates directories "A", "B" and files "A/A", "B/B" + * - Creates directories DIRA, DIRB and files DIRA/"A", DIRB/"B" * - Unshares mount namespace and makes it private (so mounts/umounts have no * effect on a real system) - * - Bind mounts directory "A" to itself - * - Makes directory "A" shared + * - Bind mounts directory DIRA to itself + * - Makes directory DIRA shared * - Clones a new child process with CLONE_NEWNS flag and makes "A" a slave * mount * - There are two testcases (where X is parent namespace and Y child * namespace): * 1. First test case - * .. X: bind mounts "B" to "A" - * .. Y: must see the file "A/B" - * .. X: umounts "A" + * .. X: bind mounts DIRB to DIRA + * .. Y: must see the file DIRA/"B" + * .. X: umounts DIRA * 2. Second test case - * .. Y: bind mounts "B" to "A" - * .. X: must see only the "A/A" and must not see "A/B" (as slave mount does + * .. Y: bind mounts DIRB to DIRA + * .. X: must see only the DIRA/"A" and must not see DIRA/"B" (as slave mount does * not forward propagation) - * .. Y: umounts "A" + * .. Y: umounts DIRA */ #include <sys/wait.h> diff --git a/testcases/kernel/containers/mountns/mountns04.c b/testcases/kernel/containers/mountns/mountns04.c index cc63a03d9..46937ddcd 100644 --- a/testcases/kernel/containers/mountns/mountns04.c +++ b/testcases/kernel/containers/mountns/mountns04.c @@ -10,12 +10,12 @@ * Tests an unbindable mount: unbindable mount is an unbindable * private mount. * - * - Creates directories "A", "B" and files "A/A", "B/B" + * - Creates directories DIRA, DIRB and files DIRA/"A", DIRB/"B" * - Unshares mount namespace and makes it private (so mounts/umounts have no * effect on a real system) - * - Bind mounts directory "A" to "A" - * - Makes directory "A" unbindable - * - Check if bind mount unbindable "A" to "B" fails as expected + * - Bind mounts directory DIRA to DIRA + * - Makes directory DIRA unbindable + * - Check if bind mount unbindable DIRA to DIRB fails as expected */ #include <sys/wait.h>
Running LTP20220527 release it appears that the recently re-written tests mountns02/03/04 can now throw a warning on their final umount attempt in some setup: <<<test_output>>> tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s mountns04.c:38: TPASS: unbindable mount passed tst_device.c:395: TWARN: umount('A') failed with EINVAL mountns.h:36: TWARN: umount(A) failed: EINVAL (22) tst_device.c:434: TINFO: No device is mounted at B Moreover, the underlying safe_umount() then upgrades the TWARN emitted from tst_umount to a TBROK, so causing the test to completely fail: Summary: passed 1 failed 0 broken 0 skipped 0 warnings 2 <<<execution_status>>> initiation_status="ok" duration=0 termination_type=exited termination_id=4 corefile=no Even though the final SAFE_UMOUNTs in the test body properly unmount the test created mountpoints, the final cleanup functions, that finally check to see if those mountpoints are still mounted, can be fooled into falsely thinking that test-chosen mountpoints "A" or "B" are still there: this is due to the fact that the internal helper tst_is_mounted() uses a simple strstr() on /proc/mounts to check if a directory is still mounted and clearly the currently test-chosen names are far too much simple, being one-letter, and they can be easily matched by other unrelated mountpoints that happen to exist on a specific setup. Use a more peculiar naming for the test chosen mountpoints and generalize accordingly all the comments. Cc: Andrea Cervesato <andrea.cervesato@suse.de> Cc: Cyril Hrubis <chrubis@suse.cz> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- v1 --> v2 - using more peculiar naming for mountpoints - fix comments - dropped previous SAFE_UMONUT removal A better, more long term fix should be to fix/harden tst_is_mounted logic, but looking at mountpoint(1) implementation this is far from trivial to be done (especially for bind mounts) and it would require a bit of 're-inventing the wheel' to bring all the mountpoint/libmount helpers and logic inside LTP; on the other side a dirty and ugly solution based on something like tst_system("mountpoint -q <dir>") would be less portable since would add the new mountpoint application as an LTP pre-requisite. (and so just breaking a few CI probably without having a 'mountpoint-less' failover mechanism)...so I just generalized the chosen names for now... --- testcases/kernel/containers/mountns/mountns.h | 4 ++-- .../kernel/containers/mountns/mountns01.c | 18 +++++++++--------- .../kernel/containers/mountns/mountns02.c | 18 +++++++++--------- .../kernel/containers/mountns/mountns03.c | 18 +++++++++--------- .../kernel/containers/mountns/mountns04.c | 8 ++++---- 5 files changed, 33 insertions(+), 33 deletions(-)