[PATCH 0/3] btrfs-progs: subvolume create: accept multiple arguments

[PATCH 0/3] btrfs-progs: subvolume create: accept multiple arguments

[Qu Wenruo]

This patchset adds the ability to accept multiple arguments for "btrfs
subvolume create" command, just like "mkdir".

And also we follow the error reporting part of "mkdir", any failure
would make the command to return 1 for error.

[PATCHSET STRUCTURE]
During the development, I found two missing error handling for strdup(),
thus here comes the first patch to fix them.

Then the 2nd patch implements the main part.

Finally the last patch is add the new test case for the error handling
part.

Qu Wenruo (3):
  btrfs-progs: subvolume create: handle failure for strdup()
  btrfs-progs: subvolume create: accept multiple arguments
  btrfs-progs: cli-tests: add test case for subvolume create multiple
    arguments

 Documentation/btrfs-subvolume.rst             |  11 +-
 cmds/subvolume.c                              | 223 ++++++++++--------
 .../021-subvolume-multiple-arguments/test.sh  |  37 +++
 3 files changed, 170 insertions(+), 101 deletions(-)
 create mode 100755 tests/cli-tests/021-subvolume-multiple-arguments/test.sh

--
2.42.0

[PATCH 1/3] btrfs-progs: subvolume create: handle failure for strdup()

[Qu Wenruo]

The function strdup() can return NULL if the system is out of memory,
thus we need to hanle the rare but possible -ENOMEM case.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/subvolume.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 8504c380c9ee..bccc4968dad3 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -194,8 +194,17 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
 	}
 
 	dupname = strdup(dst);
+	if (!dupname) {
+		error("out of memory when duplicating %s", dst);
+		goto out;
+	}
 	newname = basename(dupname);
+
 	dupdir = strdup(dst);
+	if (!dupdir) {
+		error("out of memory when duplicating %s", dst);
+		goto out;
+	}
 	dstdir = dirname(dupdir);
 
 	if (!test_issubvolname(newname)) {
-- 
2.42.0

[PATCH 2/3] btrfs-progs: subvolume create: accept multiple arguments

[Qu Wenruo]

This patch would make "btrfs subvolume create" to accept multiple
arguments, just like "mkdir".

The existing options like "-i <qgroupid>" and "-p" would all be applied
to all subvolume(s).

If one destination failed, the command would return 1, while still retry
the remaining destinations.

Issue: #695
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-subvolume.rst |  11 ++-
 cmds/subvolume.c                  | 150 +++++++++++++++++-------------
 2 files changed, 92 insertions(+), 69 deletions(-)

diff --git a/Documentation/btrfs-subvolume.rst b/Documentation/btrfs-subvolume.rst
index 6da00be8dc86..8b434475337b 100644
--- a/Documentation/btrfs-subvolume.rst
+++ b/Documentation/btrfs-subvolume.rst
@@ -49,12 +49,19 @@ do not affect the files in the original subvolume.
 SUBCOMMAND
 -----------
 
-create [options] [<dest>/]<name>
-        Create a subvolume *name* in *dest*.
+create [options] [<dest>/]<name> [[<dest2>/]<name2>] ...
+        Create subvolume(s) at the destination(s).
 
         If *dest* is not given, subvolume *name* will be created in the current
         directory.
 
+	If multiple desinations are given, then the options are applied to all
+	subvolumes.
+
+	If failure happened for any of the destinations, the command would
+	still retry the remaining destinations, but would return 1 to indicate
+	the failure.
+
         ``Options``
 
         -i <qgroupid>
diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index bccc4968dad3..7decaa1eb828 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -114,81 +114,38 @@ static const char * const subvolume_cmd_group_usage[] = {
 };
 
 static const char * const cmd_subvolume_create_usage[] = {
-	"btrfs subvolume create [options] [<dest>/]<name>",
-	"Create a subvolume",
-	"Create a subvolume <name> in <dest>.  If <dest> is not given",
+	"btrfs subvolume create [options] [<dest>/]<name> [[<dest2>/]<name2>] ...",
+	"Create subvolume(s)",
+	"Create subvolume(s) at specified destination.  If <dest> is not given",
 	"subvolume <name> will be created in the current directory.",
 	"",
-	OPTLINE("-i <qgroupid>", "add the newly created subvolume to a qgroup. This option can be given multiple times."),
+	OPTLINE("-i <qgroupid>", "add the newly created subvolume(s) to a qgroup. This option can be given multiple times."),
 	OPTLINE("-p|--parents", "create any missing parent directories for each argument (like mkdir -p)"),
 	HELPINFO_INSERT_GLOBALS,
 	HELPINFO_INSERT_QUIET,
 	NULL
 };
 
-static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **argv)
+static int create_one_subvolume(const char *dst,
+				struct btrfs_qgroup_inherit *inherit,
+				bool create_parents)
 {
-	int	retval, res, len;
+	int	ret;
+	int	len;
 	int	fddst = -1;
 	char	*dupname = NULL;
 	char	*dupdir = NULL;
 	char	*newname;
 	char	*dstdir;
-	char	*dst;
-	struct btrfs_qgroup_inherit *inherit = NULL;
 	DIR	*dirstream = NULL;
-	bool create_parents = false;
 
-	optind = 0;
-	while (1) {
-		int c;
-		static const struct option long_options[] = {
-			{ "parents", no_argument, NULL, 'p' },
-			{ NULL, 0, NULL, 0 }
-		};
-
-		c = getopt_long(argc, argv, "i:p", long_options, NULL);
-		if (c < 0)
-			break;
-
-		switch (c) {
-		case 'c':
-			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
-			if (res) {
-				retval = res;
-				goto out;
-			}
-			break;
-		case 'i':
-			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
-			if (res) {
-				retval = res;
-				goto out;
-			}
-			break;
-		case 'p':
-			create_parents = true;
-			break;
-		default:
-			usage_unknown_option(cmd, argv);
-		}
-	}
-
-	if (check_argc_exact(argc - optind, 1)) {
-		retval = 1;
-		goto out;
-	}
-
-	dst = argv[optind];
-
-	retval = 1;	/* failure */
-	res = path_is_dir(dst);
-	if (res < 0 && res != -ENOENT) {
-		errno = -res;
+	ret = path_is_dir(dst);
+	if (ret < 0 && ret != -ENOENT) {
+		errno = -ret;
 		error("cannot access %s: %m", dst);
 		goto out;
 	}
-	if (res >= 0) {
+	if (ret >= 0) {
 		error("target path already exists: %s", dst);
 		goto out;
 	}
@@ -230,15 +187,15 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
 		token = strtok(dstdir_dup, "/");
 		while (token) {
 			strcat(p, token);
-			res = path_is_dir(p);
-			if (res == -ENOENT) {
-				res = mkdir(p, 0777);
-				if (res < 0) {
+			ret = path_is_dir(p);
+			if (ret == -ENOENT) {
+				ret = mkdir(p, 0777);
+				if (ret < 0) {
 					error("failed to create directory %s: %m", p);
 					goto out;
 				}
-			} else if (res <= 0) {
-				errno = res;
+			} else if (ret <= 0) {
+				errno = ret ;
 				error("failed to check directory %s before creation: %m", p);
 				goto out;
 			}
@@ -261,28 +218,87 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
 		args.size = btrfs_qgroup_inherit_size(inherit);
 		args.qgroup_inherit = inherit;
 
-		res = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE_V2, &args);
+		ret = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE_V2, &args);
 	} else {
 		struct btrfs_ioctl_vol_args	args;
 
 		memset(&args, 0, sizeof(args));
 		strncpy_null(args.name, newname);
 
-		res = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args);
+		ret = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args);
 	}
 
-	if (res < 0) {
+	if (ret < 0) {
 		error("cannot create subvolume: %m");
 		goto out;
 	}
 
-	retval = 0;	/* success */
 out:
 	close_file_or_dir(fddst, dirstream);
-	free(inherit);
 	free(dupname);
 	free(dupdir);
 
+	return ret;
+}
+static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **argv)
+{
+	int	retval, res;
+	struct btrfs_qgroup_inherit *inherit = NULL;
+	bool has_error = false;
+	bool create_parents = false;
+
+	optind = 0;
+	while (1) {
+		int c;
+		static const struct option long_options[] = {
+			{ "parents", no_argument, NULL, 'p' },
+			{ NULL, 0, NULL, 0 }
+		};
+
+		c = getopt_long(argc, argv, "i:p", long_options, NULL);
+		if (c < 0)
+			break;
+
+		switch (c) {
+		case 'c':
+			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
+			if (res) {
+				retval = res;
+				goto out;
+			}
+			break;
+		case 'i':
+			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
+			if (res) {
+				retval = res;
+				goto out;
+			}
+			break;
+		case 'p':
+			create_parents = true;
+			break;
+		default:
+			usage_unknown_option(cmd, argv);
+		}
+	}
+
+	if (check_argc_min(argc - optind, 1)) {
+		retval = 1;
+		goto out;
+	}
+
+	retval = 1;
+
+	for (int i = optind; i < argc; i++) {
+		res = create_one_subvolume(argv[i], inherit, create_parents);
+		if (res < 0)
+			has_error = true;
+	}
+	if (!has_error)
+		retval = 0;
+out:
+	free(inherit);
+
 	return retval;
 }
 static DEFINE_SIMPLE_COMMAND(subvolume_create, "create");
-- 
2.42.0

[PATCH 3/3] btrfs-progs: cli-tests: add test case for subvolume create multiple arguments

[Qu Wenruo]

The test case would verify the following behaviors:

- Partial failure
  Should return 1, but the remaining valid destinations would still be
  created.

- All success
  That's as usual.

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

diff --git a/tests/cli-tests/021-subvolume-multiple-arguments/test.sh b/tests/cli-tests/021-subvolume-multiple-arguments/test.sh
new file mode 100755
index 000000000000..f86763829d31
--- /dev/null
+++ b/tests/cli-tests/021-subvolume-multiple-arguments/test.sh
@@ -0,0 +1,37 @@
+#!/bin/bash
+# Create subvolume with multiple destinations
+
+source "$TEST_TOP/common" || exit
+
+setup_root_helper
+prepare_test_dev
+
+run_check_mkfs_test_dev
+run_check_mount_test_dev
+
+# Create one invalid subvolume with 2 valid ones.
+# The command should return 1 but the 2 valid ones should be created.
+run_mustfail "should report error for any failed subvolume creation" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume create \
+	"$TEST_MNT/non-exist-dir/subv0" \
+	"$TEST_MNT/subv1" \
+	"$TEST_MNT/subv2"
+
+run_check $SUDO_HELPER stat "$TEST_MNT/subv1"
+run_check $SUDO_HELPER stat "$TEST_MNT/subv2"
+
+# Create multiple subvolumes with parent
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create -p \
+	"$TEST_MNT/non-exist-dir/subv0" \
+	"$TEST_MNT/subv1/subv3" \
+	"$TEST_MNT/subv4" \
+
+run_check $SUDO_HELPER stat "$TEST_MNT/non-exist-dir/subv0"
+run_check $SUDO_HELPER stat "$TEST_MNT/subv1/subv3"
+run_check $SUDO_HELPER stat "$TEST_MNT/subv4"
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create -p "$TEST_MNT/dir3/dir1/./..//.///subv3//////"
+run_check $SUDO_HELPER stat "$TEST_MNT/dir3/dir1"
+run_check $SUDO_HELPER stat "$TEST_MNT/dir3/subv3"
+run_check find "$TEST_MNT" -ls
+run_check_umount_test_dev
-- 
2.42.0

Re: [PATCH 1/3] btrfs-progs: subvolume create: handle failure for strdup()

[David Sterba]

On Thu, Nov 02, 2023 at 04:03:48PM +1030, Qu Wenruo wrote:
> The function strdup() can return NULL if the system is out of memory,
> thus we need to hanle the rare but possible -ENOMEM case.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  cmds/subvolume.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> index 8504c380c9ee..bccc4968dad3 100644
> --- a/cmds/subvolume.c
> +++ b/cmds/subvolume.c
> @@ -194,8 +194,17 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
>  	}
>  
>  	dupname = strdup(dst);
> +	if (!dupname) {
> +		error("out of memory when duplicating %s", dst);

There are message templats for the most common errors, so this is now

error_msg(ERROR_MSG_MEMORY, "duplicating %s", dst);

Re: [PATCH 0/3] btrfs-progs: subvolume create: accept multiple arguments

[David Sterba]

On Thu, Nov 02, 2023 at 04:03:47PM +1030, Qu Wenruo wrote:
> This patchset adds the ability to accept multiple arguments for "btrfs
> subvolume create" command, just like "mkdir".
> 
> And also we follow the error reporting part of "mkdir", any failure
> would make the command to return 1 for error.
> 
> [PATCHSET STRUCTURE]
> During the development, I found two missing error handling for strdup(),
> thus here comes the first patch to fix them.
> 
> Then the 2nd patch implements the main part.
> 
> Finally the last patch is add the new test case for the error handling
> part.
> 
> Qu Wenruo (3):
>   btrfs-progs: subvolume create: handle failure for strdup()
>   btrfs-progs: subvolume create: accept multiple arguments
>   btrfs-progs: cli-tests: add test case for subvolume create multiple
>     arguments

Added to devel, thanks.