Re: [LTP] [PATCH v2] Fix mountns01/02/03/04 final umounts using more peculiar dir names

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

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>

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp