[PATCH 0/2] btrfs-progs: fix the return value of "btrfs

[PATCH 0/2] btrfs-progs: fix the return value of "btrfs

[Qu Wenruo]

There is a bug report ("https://github.com/kdave/btrfs-progs/issues/730")
that after commit 5aa959fb3440 ("btrfs-progs: subvolume create: accept
multiple arguments"), "btrfs subvolume create" no longer properly return
1 for error cases.

It turns out that the offending commit changed how we determine the
return code, thus for several error cases, we still return 0 for
create_one_subvolume().

Fix it and add a test case for it.

Qu Wenruo (2):
  btrfs-progs: cmd/subvolume: ix return value when the target exists
  btrfs-progs: cli-tests: add test case for return value of "btrfs
    subvlume create"

 cmds/subvolume.c                              | 11 ++++++-
 .../025-subvolume-create-failures/test.sh     | 30 +++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh

--
2.43.0

[PATCH 1/2] btrfs-progs: cmd/subvolume: ix return value when the target exists

[Qu Wenruo]

[BUG]
When try to create a subvolume where the target path already exists, the
"btrfs" comannd doesn't return error code correctly.

  # mkfs.btrfs -f $dev
  # mount $dev $mnt
  # touch $mnt/subv1
  # btrfs subvolume create $mnt/subv1
  ERROR: target path already exists: $mnt/subv1
  # echo $?
  0

[CAUSE]
The check on whether target exists is done by path_is_dir(), if it
return 0 or 1, it means there is something in that path already.

But unfortunately commit 5aa959fb3440 ("btrfs-progs: subvolume create:
accept multiple arguments") only changed the out tag, which would
directly return @ret, not updating the return value correct.

[FIX]
Make sure all error out branch has their @ret manually updated.

Fixes: 5aa959fb3440 ("btrfs-progs: subvolume create: accept multiple arguments")
Issue: #730
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/subvolume.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 57be9eac5e56..b01d5c80f63d 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -160,12 +160,14 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
 	}
 	if (ret >= 0) {
 		error("target path already exists: %s", dst);
+		ret = -EEXIST;
 		goto out;
 	}
 
 	dupname = strdup(dst);
 	if (!dupname) {
 		error_msg(ERROR_MSG_MEMORY, "duplicating %s", dst);
+		ret = -ENOMEM;
 		goto out;
 	}
 	newname = basename(dupname);
@@ -173,18 +175,21 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
 	dupdir = strdup(dst);
 	if (!dupdir) {
 		error_msg(ERROR_MSG_MEMORY, "duplicating %s", dst);
+		ret = -ENOMEM;
 		goto out;
 	}
 	dstdir = dirname(dupdir);
 
 	if (!test_issubvolname(newname)) {
 		error("invalid subvolume name: %s", newname);
+		ret = -EINVAL;
 		goto out;
 	}
 
 	len = strlen(newname);
 	if (len > BTRFS_VOL_NAME_MAX) {
 		error("subvolume name too long: %s", newname);
+		ret = -EINVAL;
 		goto out;
 	}
 
@@ -208,6 +213,8 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
 					goto out;
 				}
 			} else if (ret <= 0) {
+				if (ret == 0)
+					ret = -EEXIST;
 				errno = ret ;
 				error("failed to check directory %s before creation: %m", p);
 				goto out;
@@ -218,8 +225,10 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
 	}
 
 	fddst = btrfs_open_dir(dstdir, &dirstream, 1);
-	if (fddst < 0)
+	if (fddst < 0) {
+		ret = fddst;
 		goto out;
+	}
 
 	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
 	if (inherit) {
-- 
2.43.0

[PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create"

[Qu Wenruo]

The test case would check if "btrfs subvolume create":

- Report error on an existing path
- Still report error if mulitple paths are given and one of them already
  exists
- For above case, still created a subvolume for the good parameter

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../025-subvolume-create-failures/test.sh     | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh

diff --git a/tests/cli-tests/025-subvolume-create-failures/test.sh b/tests/cli-tests/025-subvolume-create-failures/test.sh
new file mode 100755
index 000000000000..b268a069ba37
--- /dev/null
+++ b/tests/cli-tests/025-subvolume-create-failures/test.sh
@@ -0,0 +1,30 @@
+#!/bin/bash
+# Create subvolume failure cases to make sure the return value is correct
+
+source "$TEST_TOP/common" || exit
+
+setup_root_helper
+prepare_test_dev
+
+run_check_mkfs_test_dev
+run_check_mount_test_dev
+
+# Create one subvolume and one file as place holder for later subvolume
+# creation to fail.
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
+run_check $SUDO_HELPER touch "$TEST_MNT/subv2"
+
+# Using existing path to create a subvolume must fail
+run_mustfail "should report error when target path already exists" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
+
+run_mustfail "should report error when target path already exists" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv2"
+
+# Using multiple subvolumes in one create go, the good one "subv3" should be
+# created
+run_mustfail "should report error when target path already exists" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume create \
+	"$TEST_MNT/subv1" "$TEST_MNT/subv2" "$TEST_MNT/subv3"
+
+run_check $SUDO_HELPER stat "$TEST_MNT/subv3"
-- 
2.43.0

Re: [PATCH 0/2] btrfs-progs: fix the return value of "btrfs

[David Sterba]

On Wed, Jan 10, 2024 at 01:23:30PM +1030, Qu Wenruo wrote:
> There is a bug report ("https://github.com/kdave/btrfs-progs/issues/730")
> that after commit 5aa959fb3440 ("btrfs-progs: subvolume create: accept
> multiple arguments"), "btrfs subvolume create" no longer properly return
> 1 for error cases.
> 
> It turns out that the offending commit changed how we determine the
> return code, thus for several error cases, we still return 0 for
> create_one_subvolume().
> 
> Fix it and add a test case for it.
> 
> Qu Wenruo (2):
>   btrfs-progs: cmd/subvolume: ix return value when the target exists
>   btrfs-progs: cli-tests: add test case for return value of "btrfs
>     subvlume create"

Added to devel, thanks.

Re: [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create"

[Qu Wenruo]

[-- Attachment #1.1.1: Type: text/plain, Size: 2143 bytes --]



On 2024/1/10 13:23, Qu Wenruo wrote:
> The test case would check if "btrfs subvolume create":
> 
> - Report error on an existing path
> - Still report error if mulitple paths are given and one of them already
>    exists
> - For above case, still created a subvolume for the good parameter
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   .../025-subvolume-create-failures/test.sh     | 30 +++++++++++++++++++
>   1 file changed, 30 insertions(+)
>   create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh
> 
> diff --git a/tests/cli-tests/025-subvolume-create-failures/test.sh b/tests/cli-tests/025-subvolume-create-failures/test.sh
> new file mode 100755
> index 000000000000..b268a069ba37
> --- /dev/null
> +++ b/tests/cli-tests/025-subvolume-create-failures/test.sh
> @@ -0,0 +1,30 @@
> +#!/bin/bash
> +# Create subvolume failure cases to make sure the return value is correct
> +
> +source "$TEST_TOP/common" || exit
> +
> +setup_root_helper
> +prepare_test_dev
> +
> +run_check_mkfs_test_dev
> +run_check_mount_test_dev
> +
> +# Create one subvolume and one file as place holder for later subvolume
> +# creation to fail.
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
> +run_check $SUDO_HELPER touch "$TEST_MNT/subv2"
> +
> +# Using existing path to create a subvolume must fail
> +run_mustfail "should report error when target path already exists" \
> +	$SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
> +
> +run_mustfail "should report error when target path already exists" \
> +	$SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv2"
> +
> +# Using multiple subvolumes in one create go, the good one "subv3" should be
> +# created
> +run_mustfail "should report error when target path already exists" \
> +	$SUDO_HELPER "$TOP/btrfs" subvolume create \
> +	"$TEST_MNT/subv1" "$TEST_MNT/subv2" "$TEST_MNT/subv3"
> +
> +run_check $SUDO_HELPER stat "$TEST_MNT/subv3"

My bad, I forgot to unmount the test dev

David, mind to add the following line to clean it up?

run_check_umount_test_dev

Thanks,
Qu

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7027 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create"

[David Sterba]

On Wed, Jan 10, 2024 at 05:27:17PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/10 13:23, Qu Wenruo wrote:
> > The test case would check if "btrfs subvolume create":
> > 
> > - Report error on an existing path
> > - Still report error if mulitple paths are given and one of them already
> >    exists
> > - For above case, still created a subvolume for the good parameter
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >   .../025-subvolume-create-failures/test.sh     | 30 +++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> >   create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh
> > 
> > diff --git a/tests/cli-tests/025-subvolume-create-failures/test.sh b/tests/cli-tests/025-subvolume-create-failures/test.sh
> > new file mode 100755
> > index 000000000000..b268a069ba37
> > --- /dev/null
> > +++ b/tests/cli-tests/025-subvolume-create-failures/test.sh
> > @@ -0,0 +1,30 @@
> > +#!/bin/bash
> > +# Create subvolume failure cases to make sure the return value is correct
> > +
> > +source "$TEST_TOP/common" || exit
> > +
> > +setup_root_helper
> > +prepare_test_dev
> > +
> > +run_check_mkfs_test_dev
> > +run_check_mount_test_dev
> > +
> > +# Create one subvolume and one file as place holder for later subvolume
> > +# creation to fail.
> > +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
> > +run_check $SUDO_HELPER touch "$TEST_MNT/subv2"
> > +
> > +# Using existing path to create a subvolume must fail
> > +run_mustfail "should report error when target path already exists" \
> > +	$SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
> > +
> > +run_mustfail "should report error when target path already exists" \
> > +	$SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv2"
> > +
> > +# Using multiple subvolumes in one create go, the good one "subv3" should be
> > +# created
> > +run_mustfail "should report error when target path already exists" \
> > +	$SUDO_HELPER "$TOP/btrfs" subvolume create \
> > +	"$TEST_MNT/subv1" "$TEST_MNT/subv2" "$TEST_MNT/subv3"
> > +
> > +run_check $SUDO_HELPER stat "$TEST_MNT/subv3"
> 
> My bad, I forgot to unmount the test dev
> 
> David, mind to add the following line to clean it up?
> 
> run_check_umount_test_dev

Updated, thanks.