Re: [LTP] [PATCH v2] getxattr01: Convert to new API

[Petr Vorel <pvorel@suse.cz>]

> ---
> Changes since v1:
> 1. removed comments above tcases;
> 2. simplified check logic;
> 3. replaced close() with SAFE_CLOSE();
> 4. Description: merged the 4th point into the 3rd one according to
>    the check logic;

...

#ifdef HAVE_SYS_XATTR_H
Please remove this check, we always have <sys/xattr.h>.

> +#define XATTR_TEST_NOKEY	"user.nosuchkey"
> +#define XATTR_TEST_KEY		"user.testkey"
> +#define XATTR_TEST_VALUE	"this is a test value"
> +#define XATTR_TEST_VALUE_SIZE	20
> +#define BUFFSIZE		64

> -char filename[BUFSIZ];
> +static char filename[BUFSIZ];

> -struct test_case {
> +static struct tcase {
>  	char *fname;
>  	char *key;
>  	char *value;
>  	size_t size;
>  	int exp_err;
> +} tcases[] = {
> +	{ filename, XATTR_TEST_NOKEY, NULL, BUFFSIZE - 1, ENODATA },
> +	{ filename, XATTR_TEST_KEY, NULL, 1, ERANGE },
> +	{ filename, XATTR_TEST_KEY, NULL, BUFFSIZE - 1, 0 },

If .fname is always filename, why to pass it via test struct?
The same applies for .value (always NULL).

NOTE, this is ok, but if there were a lot of more struct members and some of
them would be 0 or NULL, we'd prefer to initialize it as:

{ .key = XATTR_TEST_NOKEY, .size = BUFFSIZE - 1, .exp_err = ENODATA },
{ .key = XATTR_TEST_KEY, .size = 1, .exp_err = ERANGE },
{ .key = XATTR_TEST_KEY, .size = BUFFSIZE - 1 }

This way it would be possible to avoid many NULL or 0.

...
> +	struct tcase *tc = &tcases[n];
> +
> +	if (tc->exp_err == 0) {
> +		TST_EXP_VAL(getxattr(tc->fname, tc->key, tc->value, tc->size),
> +			    XATTR_TEST_VALUE_SIZE);

> +		if (memcmp(tc->value, XATTR_TEST_VALUE, XATTR_TEST_VALUE_SIZE))
> +			tst_res(TFAIL, "Wrong value, expect \"%s\" got \"%s\"",
instead of \", more readable is to use '.

> +				XATTR_TEST_VALUE, tc->value);
>  		else
> -			tst_resm(TPASS, "Got the right value");
> +			tst_res(TPASS, "getxattr() retrieved expected value");
> +	} else {
> +		TST_EXP_FAIL(getxattr(tc->fname, tc->key, tc->value, tc->size),
> +			     tc->exp_err);
>  	}

>  static void setup(void)
> @@ -144,41 +69,36 @@ static void setup(void)
>  	int fd;
>  	unsigned int i;

> -	tst_require_root();
> -
> -	tst_tmpdir();
> -
>  	/* Create test file and setup initial xattr */
>  	snprintf(filename, BUFSIZ, "getxattr01testfile");
> -	fd = SAFE_CREAT(cleanup, filename, 0644);
> -	close(fd);
> -	if (setxattr(filename, XATTR_TEST_KEY, XATTR_TEST_VALUE,
> -		     strlen(XATTR_TEST_VALUE), XATTR_CREATE) == -1) {
> -		if (errno == ENOTSUP) {
> -			tst_brkm(TCONF, cleanup, "No xattr support in fs or "
> -				 "mount without user_xattr option");
> -		}
> -	}
> +	fd = SAFE_CREAT(filename, 0644);
> +	SAFE_CLOSE(fd);

...
> +	for (i = 0; i < ARRAY_SIZE(tcases); i++)
> +		tcases[i].value = SAFE_MALLOC(BUFFSIZE);
>  }

>  static void cleanup(void)
>  {
> -	tst_rmdir();
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tcases); i++)
> +		if (tcases[i].value != NULL)
Shouldn't be free() unconditional (always)?

> +			free(tcases[i].value);
>  }
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.needs_root = 1,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = run,
> +};
> +
>  #else /* HAVE_SYS_XATTR_H */
> -int main(int argc, char *argv[])
> -{
> -	tst_brkm(TCONF, NULL, "<sys/xattr.h> does not exist.");
> -}
> +	TST_TEST_TCONF("<sys/xattr.h> does not exist.");
This is to be removed (as noted up).
>  #endif

Kind regards,
Petr

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