Re: [LTP] [RESEND PATCH v3 1/4] lib: tst_fs_type: add new filesystem types

Re: [LTP] [RESEND PATCH v3 1/4] lib: tst_fs_type: add new filesystem types

[Cyril Hrubis]

> +#define TST_EXT2_MAGIC     0xEF53
> +#define TST_EXT3_MAGIC     0xEF53
> +#define TST_EXT4_MAGIC     0xEF53

Maybe it would be less confusing to define these as one constant,
something like:

#define TST_EXT234_MAGIC 0xEF53

Otherwise it looks fine.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [RESEND PATCH v3 4/4] syscall/{rename11, renameat01}: do not test EMLINK if subdir limit not availiable

[Cyril Hrubis]

Hi!
>  static void test_emlink(void)
>  {
> -	if (max_subdirs == 0) {
> +	if (max_subdirs == 0 || max_subdirs == -1) {

Hmm, can't we just return 0 as well when the code that tries to figure
out max_subdirs was skipped?

Or is there really reason why we need to differentiate between these two
conditions?

>  		tst_resm(TCONF, "EMLINK test is not appropriate");
>  		return;
>  	}
> diff --git a/testcases/kernel/syscalls/renameat/renameat01.c b/testcases/kernel/syscalls/renameat/renameat01.c
> index bb5e6df..11b82f1 100644
> --- a/testcases/kernel/syscalls/renameat/renameat01.c
> +++ b/testcases/kernel/syscalls/renameat/renameat01.c
> @@ -211,7 +211,8 @@ static void setup(void)
>  
>  static void renameat_verify(const struct test_case_t *tc)
>  {
> -	if (tc->exp_errno == EMLINK && max_subdirs == 0) {
> +	if (tc->exp_errno == EMLINK &&
> +	    (max_subdirs == 0 || max_subdirs == -1)) {
>  		tst_resm(TCONF, "EMLINK test is not appropriate");
>  		return;
>  	}
> -- 
> 1.8.3.4
> 

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [RESEND PATCH v3 4/4] syscall/{rename11, renameat01}: do not test EMLINK if subdir limit not availiable

[Cyril Hrubis]

Hi!
> > Or is there really reason why we need to differentiate between these two
> > conditions?
> This is also what I'm a little confused. Use different return values, I just
> think we should distinguish different situations. If it returns -1 (or some
> other appropriate values), then we can just skip the test, and this is why
> I passed TPASS to tst_resm() in v2. If it returns 0, which means the fs has
> the limit, but we cannot reach it, something may go wrong (or be changed).

The TCONF flag means "test is not appropriate for current configuration"
which is the case in both cases.

I simply do not see a difference between the case where we know that the
filesystem has no limit and skip the test and the case where we try and
find out that the filesystem has no limit. So I just argue that the
return value for both cases should be 0 because it does not much sense
to distinguish these two in the actuall test.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [RESEND PATCH v3 3/4] lib/tests: do not print max dir number if subdir limit not availiable

[Cyril Hrubis]

Hi!
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
>  lib/tests/tst_fs_fill_subdirs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/tests/tst_fs_fill_subdirs.c b/lib/tests/tst_fs_fill_subdirs.c
> index 496db59..7c956f6 100644
> --- a/lib/tests/tst_fs_fill_subdirs.c
> +++ b/lib/tests/tst_fs_fill_subdirs.c
> @@ -33,10 +33,13 @@ static void cleanup(void)
>  
>  int main(void)
>  {
> +	int ret;
> +
>  	tst_tmpdir();
>  
> -	fprintf(stderr, "Max dir elements = %i\n",
> -	        tst_fs_fill_subdirs(NULL, "dir"));
> +	ret = tst_fs_fill_subdirs(NULL, "dir");
> +	if (ret != -1)
> +		fprintf(stderr, "Max dir elements = %i\n", ret);

Given that this is just a simple test for the library API in order to
make sure the code compiles and to figure out what the exact return
value was, there is no point in silencing the output.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [RESEND PATCH v3 2/4] lib/tst_fs_link_count.c: check fs type and do not fill subdir if no limit

[Cyril Hrubis]

> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
>  doc/test-writing-guidelines.txt |  5 ++++-
>  lib/tst_fs_link_count.c         | 35 +++++++++++++++++++++++++++++++++--
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
> index a59fdd9..8f690f7 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -892,7 +892,10 @@ int tst_fs_fill_subdirs(void (*cleanup)(void), const char *dir);
>  
>  Try to get maximum number of subdirectories in directory.
>  
> -NOTE: This number depends on the filesystem 'dir' is on.
> +NOTE: This number depends on the filesystem 'dir' is on. For current kernel,
> +subdir limit is not availiable for all filesystems (availiable for ext2, ext3,
> +minix, sysv and more). If the test runs on some other filesystems, like ramfs,
> +tmpfs, it will always fail.

This sounds a bit confusing. Maybe it will be better to say:

it will not even try to reach the limit and return 0 (or -1 if that ends
up the case).

>  This function uses 'mkdir(2)' to create directories in 'dir' until it gets
>  'EMLINK' or creates 65535 directories. If the limit is hit, the maximum number
> diff --git a/lib/tst_fs_link_count.c b/lib/tst_fs_link_count.c
> index 1b45f79..4a691bb 100644
> --- a/lib/tst_fs_link_count.c
> +++ b/lib/tst_fs_link_count.c
> @@ -24,9 +24,24 @@
>  #include "test.h"
>  #include "usctest.h"
>  #include "safe_macros.h"
> +#include "tst_fs_type.h"
>  
>  #define MAX_SANE_HARD_LINKS	65535
>  
> +/*
> + * filesystems whose subdir limit is less than MAX_SANE_HARD_LINKS
> + * XXX: we cannot filter ext4 out, because ext2/ext3/ext4 have the
> + * same magic number
> + */
> +const long subdir_limit_whitelist[] = {
> +	TST_EXT2_OLD_MAGIC, TST_EXT2_MAGIC,    TST_EXT3_MAGIC,
> +	TST_MINIX_MAGIC,    TST_MINIX_MAGIC2, TST_MINIX2_MAGIC,
> +	TST_MINIX2_MAGIC2,  TST_MINIX3_MAGIC,  TST_UDF_MAGIC,
> +	TST_SYSV2_MAGIC,    TST_SYSV4_MAGIC,   TST_UFS_MAGIC,
> +	TST_UFS2_MAGIC,     TST_F2FS_MAGIC,    TST_NILFS_MAGIC,
> +	TST_EXOFS_MAGIC
> +};
> +
>  int tst_fs_fill_hardlinks(void (*cleanup) (void), const char *dir)
>  {
>  	unsigned int i, j;
> @@ -88,9 +103,10 @@ max_hardlinks_cleanup:
>  
>  int tst_fs_fill_subdirs(void (*cleanup) (void), const char *dir)
>  {
> -	unsigned int i, j;
> +	unsigned int i, j, whitelist_size;
>  	char dirname[PATH_MAX];
>  	struct stat s;
> +	long fs_type;
>  
>  	if (stat(dir, &s) == -1 && errno == ENOENT)
>  		SAFE_MKDIR(cleanup, dir, 0744);
> @@ -99,6 +115,20 @@ int tst_fs_fill_subdirs(void (*cleanup) (void), const char *dir)
>  	if (!S_ISDIR(s.st_mode))
>  		tst_brkm(TBROK, cleanup, "%s is not directory", dir);
>  
> +	/* for current kernel, subdir limit is not availiable for all fs */
> +	fs_type = tst_fs_type(cleanup, dir);
> +
> +	whitelist_size = ARRAY_SIZE(subdir_limit_whitelist);

So you still use whitelist, I would have liked blacklist more because
if, by any change, new filesystem with this limit will be added to
kernel the test will be working as well, but either one needs to be
update under certain conditions, so in the end either one is fine.

> +	for (i = 0; i < whitelist_size; i++) {
> +		if (fs_type == subdir_limit_whitelist[i])
> +			break;
> +	}
> +	if (i == whitelist_size) {
> +		tst_resm(TINFO, "subdir limit is not availiable for "
> +			 "filesystem %s", tst_fs_type_name(fs_type));
                         ^
                         %s filesystem.
> +		return -1;
> +	}
> +
>  	for (i = 0; i < MAX_SANE_HARD_LINKS; i++) {
>  		sprintf(dirname, "%s/testdir%d", dir, i);
>  
> @@ -134,7 +164,8 @@ int tst_fs_fill_subdirs(void (*cleanup) (void), const char *dir)
>  
>  	}
>  
> -	tst_resm(TINFO, "Failed reach the subdirs limit");
> +	tst_resm(TINFO, "Failed reach the subdirs limit on filesystem %s",
                                                           ^
							on %s filesystem.
> +		 tst_fs_type_name(fs_type));

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list