| Message ID | 20180916215011.31029-1-richard@nod.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic: Test whether xattrs work on unlinked files | expand |
On Mon, Sep 17, 2018 at 12:55 AM Richard Weinberger <richard@nod.at> 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? We usually add new tests, so existing test wont start failing for unpatched kernels. > Since xfs_io does not support xattrs, we need a new C test helper.t > I could just as well write: Since xfs_io does not support xattrs, we need to add xattrs support to xfs_io but I won't be the one to insist on that. What you did is you cloned attr_replace_test.c with the tiniest change and did not even bother to mention that. Please add -T flag support to attr_replace_test.c (following xfs_io -T) instead of cloning the helper and use that in your new test. Thanks, Amir.
On Mon, Sep 17, 2018 at 7:56 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Mon, Sep 17, 2018 at 12:55 AM Richard Weinberger <richard@nod.at> 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? > > We usually add new tests, so existing test wont start failing > for unpatched kernels. > > > Since xfs_io does not support xattrs, we need a new C test helper.t > > > I could just as well write: > > Since xfs_io does not support xattrs, we need to add xattrs support to xfs_io > > but I won't be the one to insist on that. > > What you did is you cloned attr_replace_test.c with the tiniest change > and did not even bother to mention that. > > Please add -T flag support to attr_replace_test.c (following xfs_io -T) > instead of cloning the helper and use that in your new test. > Yet simpler, if you don't really need an O_TMPFILE, you don't need a C helper: exec 5>x rm x setfattr -n user.test -v 1 /proc/self/fd/5 exec 5>&- Thanks, Amir.
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 >
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 + +_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 + +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
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? Since xfs_io does not support xattrs, we need a new C test helper. Thanks, //richard --- src/Makefile | 2 +- src/attr_unlinked_test.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ 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