[PATCH 0/2] btrfs-progs: make sure "mkfs --rootdir" copies the xattr for the rootdir

[PATCH 0/2] btrfs-progs: make sure "mkfs --rootdir" copies the xattr for the rootdir

[Qu Wenruo]

We got a bug report that "mkfs.btrfs --rootdir" copies all the xattr but
the xattr of the source directory.

It turns out that we only do the regular xattr copy for all the child
inodes, not the source directory itself.

Fix it and create a test case for it.

Qu Wenruo (2):
  btrfs-progs: mkfs/rootdir: add the missing xattr for the rootdir inode
  btrfs-progs: tests/mkfs: make sure rootdir got its xattr copied

 mkfs/rootdir.c                             |  8 +++++
 tests/mkfs-tests/027-rootdir-xattr/test.sh | 40 ++++++++++++++++++++++
 2 files changed, 48 insertions(+)
 create mode 100755 tests/mkfs-tests/027-rootdir-xattr/test.sh

--
2.42.0

[PATCH 1/2] btrfs-progs: mkfs/rootdir: add the missing xattr for the rootdir inode

[Qu Wenruo]

[BUG]
When using "mkfs.btrfs" with "--rootdir" option, the top level inode
(rootdir) will not get the same xattr from the source dir:

  mkdir -p source_dir/
  touch source_dir/file
  setfattr -n user.rootdir_xattr source_dir/
  setfattr -n user.regular_xattr source_dir/file
  mkfs.btrfs -f --rootdir source_dir $dev
  mount $dev $mnt
  getfattr $mnt
  # Nothing <<<
  getfattr $mnt/file
  # file: $mnt/file
  user.regular_xattr <<<

[CAUSE]
In function traverse_directory(), we only call add_xattr_item() for all
the child inodes, not really for the rootdir inode itself, leading to
the missing xattr items.

[FIX]
Also call add_xattr_item() for the rootdir inode.

Issue: #688
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 mkfs/rootdir.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
index a413a31eb2d6..f6328c9df2ec 100644
--- a/mkfs/rootdir.c
+++ b/mkfs/rootdir.c
@@ -466,6 +466,14 @@ static int traverse_directory(struct btrfs_trans_handle *trans,
 	dir_entry->inum = parent_inum;
 	list_add_tail(&dir_entry->list, &dir_head->list);
 
+	ret = add_xattr_item(trans, root, btrfs_root_dirid(&root->root_item),
+			     dir_name);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to add xattr item for the top level inode: %m");
+		goto fail_no_dir;
+	}
+
 	root_dir_key.objectid = btrfs_root_dirid(&root->root_item);
 	root_dir_key.offset = 0;
 	root_dir_key.type = BTRFS_INODE_ITEM_KEY;
-- 
2.42.0

[PATCH 2/2] btrfs-progs: tests/mkfs: make sure rootdir got its xattr copied

[Qu Wenruo]

The new test case would:

- Create a dir as source dir for --rootdir
- Add xattr for that source dir
- Create a file inside that source dir
- Add xattr for that file
- Run "mkfs.btrfs --rootdir" with that source dir
- Mount the created fs
- Make sure the following xattr exists:
  * Xattr for the rootdir
  * Xattr for that file inside the mount point

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/mkfs-tests/027-rootdir-xattr/test.sh | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100755 tests/mkfs-tests/027-rootdir-xattr/test.sh

diff --git a/tests/mkfs-tests/027-rootdir-xattr/test.sh b/tests/mkfs-tests/027-rootdir-xattr/test.sh
new file mode 100755
index 000000000000..bff9dc71d8e0
--- /dev/null
+++ b/tests/mkfs-tests/027-rootdir-xattr/test.sh
@@ -0,0 +1,40 @@
+#!/bin/bash
+# Test if the source dir has xattr, "mkfs.btrfs --rootdir" would properly copy
+# that xattr to the rootdir inode.
+
+source "$TEST_TOP/common" || exit
+
+setup_root_helper
+prepare_test_dev
+
+check_global_prereq setfattr
+check_global_prereq getfattr
+
+# Here we don't want to use /tmp, as it's pretty common /tmp is tmpfs, which
+# doesn't support xattr.
+# Instead we go $TEST_TOP/btrfs-progs-mkfs-tests-027.XXXXXX/ instead.
+src_dir=$(mktemp -d --tmpdir=$TEST_TOP btrfs-progs-mkfs-tests-027.XXXXXX)
+run_mayfail setfattr -n user.rootdir "$src_dir"
+if [ $? -ne 0 ]; then
+	rm -rf -- "$src_dir"
+	_not_run "unable to set xattr to temporaray directory"
+fi
+run_check touch "$src_dir/foobar"
+run_check setfattr -n user.foobar "$src_dir/foobar"
+
+run_check_mkfs_test_dev --rootdir "$src_dir"
+run_check_mount_test_dev
+
+attr=$(run_check_stdout getfattr -n user.rootdir --absolute-names "$src_dir")
+if ! echo $attr | grep -q "user.rootdir" ; then
+	rm -rf -- "$src_dir"
+	_fail "no rootdir xattr found"
+fi
+
+attr=$(run_check_stdout getfattr -n user.foobar --absolute-names "$src_dir/foobar")
+if ! echo $attr | grep -q "user.foobar" ; then
+	rm -rf -- "$src_dir"
+	_fail "no regular file xattr found"
+fi
+
+rm -rf -- "$src_dir"
-- 
2.42.0

Re: [PATCH 2/2] btrfs-progs: tests/mkfs: make sure rootdir got its xattr copied

[David Sterba]

On Mon, Oct 09, 2023 at 02:04:09PM +1030, Qu Wenruo wrote:
> The new test case would:
> 
> - Create a dir as source dir for --rootdir
> - Add xattr for that source dir
> - Create a file inside that source dir
> - Add xattr for that file
> - Run "mkfs.btrfs --rootdir" with that source dir
> - Mount the created fs
> - Make sure the following xattr exists:
>   * Xattr for the rootdir
>   * Xattr for that file inside the mount point
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/mkfs-tests/027-rootdir-xattr/test.sh | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100755 tests/mkfs-tests/027-rootdir-xattr/test.sh
> 
> diff --git a/tests/mkfs-tests/027-rootdir-xattr/test.sh b/tests/mkfs-tests/027-rootdir-xattr/test.sh
> new file mode 100755
> index 000000000000..bff9dc71d8e0
> --- /dev/null
> +++ b/tests/mkfs-tests/027-rootdir-xattr/test.sh
> @@ -0,0 +1,40 @@
> +#!/bin/bash
> +# Test if the source dir has xattr, "mkfs.btrfs --rootdir" would properly copy
> +# that xattr to the rootdir inode.
> +
> +source "$TEST_TOP/common" || exit
> +
> +setup_root_helper
> +prepare_test_dev
> +
> +check_global_prereq setfattr
> +check_global_prereq getfattr
> +
> +# Here we don't want to use /tmp, as it's pretty common /tmp is tmpfs, which
> +# doesn't support xattr.
> +# Instead we go $TEST_TOP/btrfs-progs-mkfs-tests-027.XXXXXX/ instead.
> +src_dir=$(mktemp -d --tmpdir=$TEST_TOP btrfs-progs-mkfs-tests-027.XXXXXX)

In case we want to be sure that the underlying filesystem for temporary
files supports the feature we want it's better to create a temporary
btrfs filesystem, either inside the test directory o in /tmp.

Chosing TEST_TOP is IMO wrong because that's where the test related
scritps and logs are, this is not meant for temporary files at all.

Also please use one of the _mktemp helpers.

I'll apply the first patch with fix, please update the test, thanks.

Re: [PATCH 2/2] btrfs-progs: tests/mkfs: make sure rootdir got its xattr copied

[Qu Wenruo]

On 2023/10/10 00:28, David Sterba wrote:
> On Mon, Oct 09, 2023 at 02:04:09PM +1030, Qu Wenruo wrote:
>> The new test case would:
>>
>> - Create a dir as source dir for --rootdir
>> - Add xattr for that source dir
>> - Create a file inside that source dir
>> - Add xattr for that file
>> - Run "mkfs.btrfs --rootdir" with that source dir
>> - Mount the created fs
>> - Make sure the following xattr exists:
>>    * Xattr for the rootdir
>>    * Xattr for that file inside the mount point
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   tests/mkfs-tests/027-rootdir-xattr/test.sh | 40 ++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100755 tests/mkfs-tests/027-rootdir-xattr/test.sh
>>
>> diff --git a/tests/mkfs-tests/027-rootdir-xattr/test.sh b/tests/mkfs-tests/027-rootdir-xattr/test.sh
>> new file mode 100755
>> index 000000000000..bff9dc71d8e0
>> --- /dev/null
>> +++ b/tests/mkfs-tests/027-rootdir-xattr/test.sh
>> @@ -0,0 +1,40 @@
>> +#!/bin/bash
>> +# Test if the source dir has xattr, "mkfs.btrfs --rootdir" would properly copy
>> +# that xattr to the rootdir inode.
>> +
>> +source "$TEST_TOP/common" || exit
>> +
>> +setup_root_helper
>> +prepare_test_dev
>> +
>> +check_global_prereq setfattr
>> +check_global_prereq getfattr
>> +
>> +# Here we don't want to use /tmp, as it's pretty common /tmp is tmpfs, which
>> +# doesn't support xattr.
>> +# Instead we go $TEST_TOP/btrfs-progs-mkfs-tests-027.XXXXXX/ instead.
>> +src_dir=$(mktemp -d --tmpdir=$TEST_TOP btrfs-progs-mkfs-tests-027.XXXXXX)
> 
> In case we want to be sure that the underlying filesystem for temporary
> files supports the feature we want it's better to create a temporary
> btrfs filesystem, either inside the test directory o in /tmp.

Indeed, another btrfs looks much better.

Would update the patch soon.

Thanks,
Qu

> 
> Chosing TEST_TOP is IMO wrong because that's where the test related
> scritps and logs are, this is not meant for temporary files at all.
> 
> Also please use one of the _mktemp helpers.
> 
> I'll apply the first patch with fix, please update the test, thanks.