[v2,1/2] syscalls/fremovexattr: Add fremovexattr() tests

Fixes: #276

Following the same logic and tests used to test removexattr() syscall,
this commit implements tests for fremovexattr(). It only differs from
removexattr() on the given arguments: using a file descriptor instead of
the filename.

Kernel has different entry points for both, with slightly different
execution paths, mainly related to dealing with the passed file
descriptor.

Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
---
 runtest/syscalls                              |   3 +
 .../kernel/syscalls/fremovexattr/.gitignore   |   2 +
 .../kernel/syscalls/fremovexattr/Makefile     |   8 ++
 .../syscalls/fremovexattr/fremovexattr01.c    | 106 ++++++++++++++
 .../syscalls/fremovexattr/fremovexattr02.c    | 130 ++++++++++++++++++
 5 files changed, 249 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fremovexattr/.gitignore
 create mode 100644 testcases/kernel/syscalls/fremovexattr/Makefile
 create mode 100644 testcases/kernel/syscalls/fremovexattr/fremovexattr01.c
 create mode 100644 testcases/kernel/syscalls/fremovexattr/fremovexattr02.c

Hi!
> +/*
> + * Test Name: fremovexattr01
> + *
> + * Description:
> + * Like removexattr(2), fremovexattr(2) also removes an extended attribute,
> + * identified by a name, from a file but, instead of using a filename path, it
> + * uses a descriptor. This test verifies that a simple call to fremovexattr(2)
> + * removes, indeed, a previously set attribute key/value from a file.
> + */
> +
> +#include "config.h"
> +#include <errno.h>
> +#include <stdlib.h>
> +
> +#ifdef HAVE_SYS_XATTR_H
> +# include <sys/xattr.h>
> +#endif
> +
> +#include "tst_test.h"
> +
> +#ifdef HAVE_SYS_XATTR_H
> +
> +#define ENOATTR ENODATA
> +
> +#define XATTR_TEST_KEY "user.testkey"
> +#define XATTR_TEST_VALUE "this is a test value"
> +#define XATTR_TEST_VALUE_SIZE 20
> +
> +#define MNTPOINT "mntpoint"
> +#define FNAME MNTPOINT"/fremovexattr01testfile"
> +
> +static int fd = -1;
> +static char *got_value;

Why not just static char got_value[XATTR_TEST_VALUE_SIZE]?

> +static void verify_fremovexattr(void)
> +{
> +	SAFE_FSETXATTR(fd, XATTR_TEST_KEY, XATTR_TEST_VALUE,
> +			XATTR_TEST_VALUE_SIZE, XATTR_CREATE);
> +
> +	TEST(fremovexattr(fd, XATTR_TEST_KEY));
> +
> +	if (TST_RET != 0) {
> +		tst_res(TFAIL | TTERRNO, "fremovexattr(2) failed");
> +		return;
> +	}
> +
> +	memset(got_value, 0, XATTR_TEST_VALUE_SIZE);
> +
> +	TEST(fgetxattr(fd, XATTR_TEST_KEY, got_value, XATTR_TEST_VALUE_SIZE));
> +
> +	if (TST_RET >= 0) {
> +		tst_res(TFAIL, "fremovexattr(2) did not remove attribute");
> +		return;
> +	}
> +
> +	if (TST_RET < 0 && TST_ERR != ENOATTR) {
> +		tst_brk(TBROK, "fremovexattr(2) could not verify removal");
> +		return;
> +	}
> +
> +	tst_res(TPASS, "fremovexattr(2) removed attribute as expected");
> +}
> +
> +static void cleanup(void)
> +{
> +	free(got_value);
> +
> +	close(fd);
        ^
	if (fd > 0)
		SAFE_CLOSE(fd);

	would be slightly better here as it would emit
	warning if the close() has failed.

> +}
> +
> +static void setup(void)
> +{
> +	SAFE_TOUCH(FNAME, 0644, NULL);
> +
> +	fd = SAFE_OPEN(FNAME, O_RDWR, NULL);
                                      ^
				      open() is a variable argument
				      function and the third argument is
				      an integer but only in a case that
				      we passed O_CREAT in the flags, if
				      we are not creating the file
				      it should be omitted

So if we want to create the file, we don't have to call the SAFE_TOUCH()
but pass O_CREAT in the flags and pass the mode as third argument here
as:

	fd = SAFE_OPEN(FNAME, O_RDWR | O_CREAT, 0644);


> +	TEST(fremovexattr(fd, XATTR_TEST_KEY));
> +
> +	if (TST_RET == -1 && TST_ERR == EOPNOTSUPP) {
> +		tst_brk(TCONF, "fremovexattr(2) not supported");
> +		return;
                ^
		No need for the return here.

> +	}
> +
> +	got_value = SAFE_MALLOC(XATTR_TEST_VALUE_SIZE);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test_all = verify_fremovexattr,
> +	.cleanup = cleanup,
> +	.mntpoint = MNTPOINT,
> +	.mount_device = 1,
> +	.all_filesystems = 1,
> +	.needs_tmpdir = 1,
> +	.needs_root = 1,
> +};
> +
> +#else /* HAVE_SYS_XATTR_H */
> +TST_TEST_TCONF("<sys/xattr.h> does not exist");
> +#endif
> diff --git a/testcases/kernel/syscalls/fremovexattr/fremovexattr02.c b/testcases/kernel/syscalls/fremovexattr/fremovexattr02.c
> new file mode 100644
> index 000000000..5585b6abe
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fremovexattr/fremovexattr02.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2018 Linaro Limited. All rights reserved.
> + * Author: Rafael David Tinoco <rafael.tinoco@linaro.org>
> + */
> +
> +/*
> + * Test Name: fremovexattr02
> + *
> + * Test cases::
> + * 1) fremovexattr(2) fails if the named attribute does not exist.
> + * 2) fremovexattr(2) fails if file descriptor is not valid.
> + * 3) fremovexattr(2) fails if named attribute has an invalid address.
> + *
> + * Expected Results:
> + * fremovexattr(2) should return -1 and set errno to ENODATA.
> + * fremovexattr(2) should return -1 and set errno to EBADF.
> + * fremovexattr(2) should return -1 and set errno to EFAULT.
> + */
> +
> +#include "config.h"
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +
> +#ifdef HAVE_SYS_XATTR_H
> +# include <sys/xattr.h>
> +#endif
> +
> +#include "tst_test.h"
> +
> +#ifdef HAVE_SYS_XATTR_H
> +
> +#define XATTR_TEST_KEY "user.testkey"
> +
> +#define MNTPOINT "mntpoint"
> +#define FNAME MNTPOINT"/fremovexattr02testfile"
> +
> +static int fd = -1;
> +
> +struct test_case {
> +	int fd;
> +	char *key;
> +	char *value;
> +	char *gotvalue;
> +	int size;
> +	int exp_ret;
> +	int exp_err;
> +};
> +
> +struct test_case tc[] = {
> +	{				/* case 1: attribute does not exist */
> +	 .key = XATTR_TEST_KEY,
> +	 .exp_ret = -1,
> +	 .exp_err = ENODATA,
> +	 },
> +	{				/* case 2: file descriptor is invalid */
> +	 .fd = -1,
> +	 .key = XATTR_TEST_KEY,
> +	 .exp_ret = -1,
> +	 .exp_err = EBADF,
> +	 },
> +	{				/* case 3: bad name attribute */
> +	 .exp_ret = -1,
> +	 .exp_err = EFAULT,
> +	},
> +};
> +
> +static void verify_fremovexattr(unsigned int i)
> +{
> +	TEST(fremovexattr(tc[i].fd, tc[i].key));
> +
> +	if (TST_RET == -1 && TST_ERR == EOPNOTSUPP)
> +		tst_brk(TCONF, "fremovexattr(2) not supported");
> +
> +	if (tc[i].exp_ret == TST_RET) {
> +
> +		if (tc[i].exp_err) {
> +			if (tc[i].exp_err == TST_ERR) {
> +				tst_res(TPASS, "fremovexattr(2) passed");
> +				return;
> +			}
> +		} else  {
> +			tst_res(TPASS, "fremovexattr(2) passed");
> +			return;
> +		}
> +	}
> +
> +	tst_res(TFAIL | TTERRNO, "fremovexattr(2) failed");
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +}
> +
> +static void setup(void)
> +{
> +	size_t i = 0;
> +
> +	SAFE_TOUCH(FNAME, 0644, NULL);
> +	fd = SAFE_OPEN(FNAME, O_RDWR, NULL);

Here as well no need for the SAFE_TOUCH().

> +	for (i = 0; i < ARRAY_SIZE(tc); i++) {
> +
> +		if (tc[i].fd != -1)
> +			tc[i].fd = fd;
> +
> +		if (!tc[i].key && tc[i].exp_err == EFAULT)
> +			tc[i].key = tst_get_bad_addr(cleanup);
> +	}
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test = verify_fremovexattr,
> +	.cleanup = cleanup,
> +	.tcnt = ARRAY_SIZE(tc),
> +	.mntpoint = MNTPOINT,
> +	.mount_device = 1,
> +	.all_filesystems = 1,
> +	.needs_tmpdir = 1,
> +	.needs_root = 1,
> +};

I'm wondering if we need the all_filesystem here, I guess that the
ENODATA test will reach down to the filesystem driver, so it's probably
relevant here.

> +#else /* HAVE_SYS_XATTR_H */
> +TST_TEST_TCONF("<sys/xattr.h> does not exist");
> +#endif
> -- 
> 2.19.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

>> +static int fd = -1;
>> +static char *got_value;
>
> Why not just static char got_value[XATTR_TEST_VALUE_SIZE]?

Of course =o) !! Will do.

>> +static void cleanup(void)
>> +{
>> +     free(got_value);
>> +
>> +     close(fd);
>         ^
>         if (fd > 0)
>                 SAFE_CLOSE(fd);

Missed this.

>
>         would be slightly better here as it would emit
>         warning if the close() has failed.
>
>> +}
>> +
>> +static void setup(void)
>> +{
>> +     SAFE_TOUCH(FNAME, 0644, NULL);
>> +
>> +     fd = SAFE_OPEN(FNAME, O_RDWR, NULL);
>                                       ^
>                                       open() is a variable argument
>                                       function and the third argument is
>                                       an integer but only in a case that
>                                       we passed O_CREAT in the flags, if
>                                       we are not creating the file
>                                       it should be omitted

Sure! Auto mode, sorry.


>> +++ b/testcases/kernel/syscalls/fremovexattr/fremovexattr02.c
>> ...
>> +     SAFE_TOUCH(FNAME, 0644, NULL);
>> +     fd = SAFE_OPEN(FNAME, O_RDWR, NULL);
>
> Here as well no need for the SAFE_TOUCH().

Same.

>> +static struct tst_test test = {
>> +     .setup = setup,
>> +     .test = verify_fremovexattr,
>> +     .cleanup = cleanup,
>> +     .tcnt = ARRAY_SIZE(tc),
>> +     .mntpoint = MNTPOINT,
>> +     .mount_device = 1,
>> +     .all_filesystems = 1,
>> +     .needs_tmpdir = 1,
>> +     .needs_root = 1,
>> +};
>
> I'm wondering if we need the all_filesystem here, I guess that the
> ENODATA test will reach down to the filesystem driver, so it's probably
> relevant here.

Yep.. that is what I though.

Tks a lot Cyril, will send a v3.

o/