Re: [PATCH] generic: Test whether xattrs work on unlinked files

[Eryu Guan <guaneryu@gmail.com>]

On Sun, Sep 16, 2018 at 11:50:11PM +0200, Richard Weinberger wrote:
> On UBIFS we had an embarrassing bug where xattr operations
> worked only for files with nlink > 0.
> Make sure this will never happen again.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> Hi!
> 
> I'm not sure whether this is worth a new test, maybe we can add it
> to an existing test? What about generic/020?

Like Amir mentioned, we usually don't add new tests to existing cases. A
new test is totally fine and recommended.

> Since xfs_io does not support xattrs, we need a new C test helper.

Looks like you only need to create an open-but-unlinked file? Maybe you
could take a look at src/multi_open_unlinked.c and its usage and see if
it meets your need.

> 
> Thanks,
> //richard
> ---
>  src/Makefile             |  2 +-
>  src/attr_unlinked_test.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++

If a new C helper is still needed, please don't forget add an entry for
it in .gitignore.

>  tests/generic/505        | 46 +++++++++++++++++++++++++++++++++++++++++
>  tests/generic/505.out    |  2 ++
>  tests/generic/group      |  1 +
>  5 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 src/attr_unlinked_test.c
>  create mode 100755 tests/generic/505
>  create mode 100644 tests/generic/505.out
> 
> diff --git a/src/Makefile b/src/Makefile
> index 41826585..433fa517 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -27,7 +27,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	renameat2 t_getcwd e4compact test-nextquota punch-alternating \
>  	attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
>  	dio-invalidate-cache stat_test t_encrypted_d_revalidate \
> -	attr_replace_test swapon mkswap
> +	attr_replace_test swapon mkswap attr_unlinked_test
>  
>  SUBDIRS = log-writes perf
>  
> diff --git a/src/attr_unlinked_test.c b/src/attr_unlinked_test.c
> new file mode 100644
> index 00000000..f3ce6d84
> --- /dev/null
> +++ b/src/attr_unlinked_test.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test whether xattr operations work on unlinked files.
> + * based on attr_replace_test.c
> + */
> +#include <stdio.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/xattr.h>
> +#include <sys/stat.h>
> +
> +#define die() do { perror(""); \
> +fprintf(stderr, "error at line %d\n", __LINE__); \
> +exit(1); } while (0)
> +
> +#define fail(...) do { \
> +fprintf(stderr, __VA_ARGS__); exit (1); \
> +} while (0)
> +
> +int main(int argc, char *argv[])
> +{
> +	int ret;
> +	int fd;
> +	char *name = "user.world";
> +	char value[] = "hello";
> +
> +	if (argc != 2)
> +		fail("Usage: %s <file>\n", argv[0]);
> +
> +	fd = open(argv[1], O_RDWR | O_DIRECTORY | O_TMPFILE, S_IRUSR | S_IWUSR);
> +	if (fd < 0)
> +		die();
> +
> +	ret = fsetxattr(fd, name, value, sizeof(value), XATTR_CREATE);
> +	if (ret < 0)
> +		die();
> +
> +	ret = fsetxattr(fd, name, value, sizeof(value), XATTR_REPLACE);
> +	if (ret < 0)
> +		die();
> +
> +	ret = fsetxattr(fd, name, NULL, 0, 0);
> +	if (ret < 0)
> +		die();
> +
> +	close(fd);
> +
> +	return 0;
> +}
> diff --git a/tests/generic/505 b/tests/generic/505
> new file mode 100755
> index 00000000..6e8bb512
> --- /dev/null
> +++ b/tests/generic/505
> @@ -0,0 +1,46 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# FS QA Test No. 505
> +#
> +# Ensure that xattr operations work also on unlinked files.
> +# UBIFS wrongly assumed that xattr operations are only valid for nlink > 0.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15

Please use "./new generic" to generate new test template (and don't
remove the pre-defined variables if you did :-)), it also has "here" and
"tmp" defined. (Even if your test doesn't take use of the variables, but
the common helper functions do).

> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $testfile
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/attr
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs generic
> +_supported_os Linux
> +_require_test_program "attr_unlinked_test"
> +_require_attrs
> +_require_scratch

Seems like test could be done directly on TEST_DEV/TEST_DIR.

Thanks,
Eryu

> +
> +rm -f $seqres.full
> +_scratch_mkfs >>$seqres.full 2>&1
> +_scratch_mount >>$seqres.full 2>&1
> +
> +./src/attr_unlinked_test $SCRATCH_MNT/
> +
> +echo "Silence is golden"
> +
> +status=0
> +exit
> diff --git a/tests/generic/505.out b/tests/generic/505.out
> new file mode 100644
> index 00000000..80e3bd1d
> --- /dev/null
> +++ b/tests/generic/505.out
> @@ -0,0 +1,2 @@
> +QA output created by 505
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index 55155de8..75770f7a 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -507,3 +507,4 @@
>  502 auto quick log
>  503 auto quick dax punch collapse zero
>  504 auto quick locks
> +505 auto quick attr
> -- 
> 2.11.0
>