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

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

[Qu Wenruo]

[CHANGELOG]
v2:
- Fix the missing "f" for the subject of the first patch
- Add the missing unmount of the test case

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: fix 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     | 32 +++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh

--
2.43.0

[PATCH v2 1/2] btrfs-progs: cmd/subvolume: fix 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 v2 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     | 32 +++++++++++++++++++
 1 file changed, 32 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..fd074de113a5
--- /dev/null
+++ b/tests/cli-tests/025-subvolume-create-failures/test.sh
@@ -0,0 +1,32 @@
+#!/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"
+
+run_check_umount_test_dev
-- 
2.43.0