[PATCH v2 0/3] btrfs-progs: use libbtrfsutil for subvolume creation

[PATCH v2 0/3] btrfs-progs: use libbtrfsutil for subvolume creation

[Mark Harmstone]

These patches are a resending of Omar Sandoval's patch from 2018, which
appears to have been overlooked [0], split up and rebased against the
current code.

We change btrfs subvol create and btrfs subvol snapshot so that they use
libbtrfsutil rather than calling the ioctl directly.

[0] https://lore.kernel.org/linux-btrfs/ab09ba595157b7fb6606814730508cae4da48caf.1516991902.git.osandov@fb.com/

Changelog:
* Fixed deprecated function names
* Fixed test failures (now returns correct return value on failure)
* Fixed this breaking fstest btrfs/300 (thanks Boris)

Mark Harmstone (3):
  btrfs-progs: use libbtrfsutil for btrfs subvolume create
  btrfs-progs: use libbtrfsutil for btrfs subvolume snapshot
  btrfs-progs: remove unused qgroup functions

 cmds/qgroup.c    |  64 ----------------
 cmds/qgroup.h    |   2 -
 cmds/subvolume.c | 194 +++++++++++++++++++----------------------------
 3 files changed, 76 insertions(+), 184 deletions(-)

-- 
2.44.2

[PATCH v2 1/3] btrfs-progs: use libbtrfsutil for btrfs subvolume create

[Mark Harmstone]

From: Mark Harmstone <maharmstone@meta.com>

Call btrfs_util_subvolume_create in create_one_subvolume rather than
calling the ioctl directly.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
Co-authored-by: Omar Sandoval <osandov@fb.com>
---
 cmds/subvolume.c | 100 ++++++++++++++++++++---------------------------
 1 file changed, 43 insertions(+), 57 deletions(-)

diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index b4151af2..2a635fa2 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -46,6 +46,7 @@
 #include "common/units.h"
 #include "common/format-output.h"
 #include "common/tree-search.h"
+#include "common/parse-utils.h"
 #include "cmds/commands.h"
 #include "cmds/qgroup.h"
 
@@ -140,28 +141,15 @@ static const char * const cmd_subvolume_create_usage[] = {
 	NULL
 };
 
-static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *inherit,
+static int create_one_subvolume(const char *dst, struct btrfs_util_qgroup_inherit *inherit,
 				bool create_parents)
 {
 	int ret;
-	int len;
-	int	fddst = -1;
 	char	*dupname = NULL;
 	char	*dupdir = NULL;
 	const char *newname;
 	char	*dstdir;
-
-	ret = path_is_dir(dst);
-	if (ret < 0 && ret != -ENOENT) {
-		errno = -ret;
-		error("cannot access %s: %m", dst);
-		goto out;
-	}
-	if (ret >= 0) {
-		error("target path already exists: %s", dst);
-		ret = -EEXIST;
-		goto out;
-	}
+	enum btrfs_util_error err;
 
 	dupname = strdup(dst);
 	if (!dupname) {
@@ -179,19 +167,6 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
 	}
 	dstdir = path_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;
-	}
-
 	if (create_parents) {
 		char p[PATH_MAX] = { 0 };
 		char dstdir_dup[PATH_MAX];
@@ -223,47 +198,57 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
 		}
 	}
 
-	fddst = btrfs_open_dir(dstdir);
-	if (fddst < 0) {
-		ret = fddst;
+	err = btrfs_util_subvolume_create(dst, 0, NULL, inherit);
+	if (err) {
+		error_btrfs_util(err);
+		ret = 1;
 		goto out;
 	}
 
-	if (inherit) {
-		struct btrfs_ioctl_vol_args_v2	args;
+	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
 
-		memset(&args, 0, sizeof(args));
-		strncpy_null(args.name, newname, sizeof(args.name));
-		args.flags |= BTRFS_SUBVOL_QGROUP_INHERIT;
-		args.size = btrfs_qgroup_inherit_size(inherit);
-		args.qgroup_inherit = inherit;
+	ret = 0;
 
-		ret = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE_V2, &args);
-	} else {
-		struct btrfs_ioctl_vol_args	args;
+out:
+	free(dupname);
+	free(dupdir);
+
+	return ret;
+}
+
+static int qgroup_inherit_add_group(struct btrfs_util_qgroup_inherit **inherit,
+				    const char *arg)
+{
+	enum btrfs_util_error err;
+	u64 qgroupid;
 
-		memset(&args, 0, sizeof(args));
-		strncpy_null(args.name, newname, sizeof(args.name));
-		ret = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args);
+	if (!*inherit) {
+		err = btrfs_util_qgroup_inherit_create(0, inherit);
+		if (err) {
+			error_btrfs_util(err);
+			return -1;
+		}
 	}
 
-	if (ret < 0) {
-		error("cannot create subvolume: %m");
-		goto out;
+	qgroupid = parse_qgroupid_or_path(optarg);
+	if (qgroupid == 0) {
+		error("invalid qgroup specification, qgroupid must not be 0");
+		return -1;
 	}
-	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
 
-out:
-	close(fddst);
-	free(dupname);
-	free(dupdir);
+	err = btrfs_util_qgroup_inherit_add_group(inherit, qgroupid);
+	if (err) {
+		error_btrfs_util(err);
+		return -1;
+	}
 
-	return ret;
+	return 0;
 }
+
 static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **argv)
 {
 	int retval, ret;
-	struct btrfs_qgroup_inherit *inherit = NULL;
+	struct btrfs_util_qgroup_inherit *inherit = NULL;
 	bool has_error = false;
 	bool create_parents = false;
 
@@ -281,7 +266,7 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
 
 		switch (c) {
 		case 'i':
-			ret = btrfs_qgroup_inherit_add_group(&inherit, optarg);
+			ret = qgroup_inherit_add_group(&inherit, optarg);
 			if (ret) {
 				retval = ret;
 				goto out;
@@ -304,13 +289,14 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
 
 	for (int i = optind; i < argc; i++) {
 		ret = create_one_subvolume(argv[i], inherit, create_parents);
-		if (ret < 0)
+		if (ret)
 			has_error = true;
 	}
 	if (!has_error)
 		retval = 0;
 out:
-	free(inherit);
+	if (inherit)
+		btrfs_util_qgroup_inherit_destroy(inherit);
 
 	return retval;
 }
-- 
2.44.2

Re: [PATCH v2 1/3] btrfs-progs: use libbtrfsutil for btrfs subvolume create

[Qu Wenruo]

在 2024/8/2 20:57, Mark Harmstone 写道:
> From: Mark Harmstone <maharmstone@meta.com>
>
> Call btrfs_util_subvolume_create in create_one_subvolume rather than
> calling the ioctl directly.
>
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> Co-authored-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   cmds/subvolume.c | 100 ++++++++++++++++++++---------------------------
>   1 file changed, 43 insertions(+), 57 deletions(-)
>
> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> index b4151af2..2a635fa2 100644
> --- a/cmds/subvolume.c
> +++ b/cmds/subvolume.c
> @@ -46,6 +46,7 @@
>   #include "common/units.h"
>   #include "common/format-output.h"
>   #include "common/tree-search.h"
> +#include "common/parse-utils.h"
>   #include "cmds/commands.h"
>   #include "cmds/qgroup.h"
>
> @@ -140,28 +141,15 @@ static const char * const cmd_subvolume_create_usage[] = {
>   	NULL
>   };
>
> -static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *inherit,
> +static int create_one_subvolume(const char *dst, struct btrfs_util_qgroup_inherit *inherit,
>   				bool create_parents)
>   {
>   	int ret;
> -	int len;
> -	int	fddst = -1;
>   	char	*dupname = NULL;
>   	char	*dupdir = NULL;
>   	const char *newname;
>   	char	*dstdir;
> -
> -	ret = path_is_dir(dst);
> -	if (ret < 0 && ret != -ENOENT) {
> -		errno = -ret;
> -		error("cannot access %s: %m", dst);
> -		goto out;
> -	}
> -	if (ret >= 0) {
> -		error("target path already exists: %s", dst);
> -		ret = -EEXIST;
> -		goto out;
> -	}
> +	enum btrfs_util_error err;
>
>   	dupname = strdup(dst);
>   	if (!dupname) {
> @@ -179,19 +167,6 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
>   	}
>   	dstdir = path_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;
> -	}
> -
>   	if (create_parents) {
>   		char p[PATH_MAX] = { 0 };
>   		char dstdir_dup[PATH_MAX];
> @@ -223,47 +198,57 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
>   		}
>   	}
>
> -	fddst = btrfs_open_dir(dstdir);
> -	if (fddst < 0) {
> -		ret = fddst;
> +	err = btrfs_util_subvolume_create(dst, 0, NULL, inherit);
> +	if (err) {
> +		error_btrfs_util(err);
> +		ret = 1;
>   		goto out;
>   	}
>
> -	if (inherit) {
> -		struct btrfs_ioctl_vol_args_v2	args;
> +	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
>
> -		memset(&args, 0, sizeof(args));
> -		strncpy_null(args.name, newname, sizeof(args.name));
> -		args.flags |= BTRFS_SUBVOL_QGROUP_INHERIT;
> -		args.size = btrfs_qgroup_inherit_size(inherit);
> -		args.qgroup_inherit = inherit;
> +	ret = 0;
>
> -		ret = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE_V2, &args);
> -	} else {
> -		struct btrfs_ioctl_vol_args	args;
> +out:
> +	free(dupname);
> +	free(dupdir);
> +
> +	return ret;
> +}
> +
> +static int qgroup_inherit_add_group(struct btrfs_util_qgroup_inherit **inherit,
> +				    const char *arg)
> +{
> +	enum btrfs_util_error err;
> +	u64 qgroupid;
>
> -		memset(&args, 0, sizeof(args));
> -		strncpy_null(args.name, newname, sizeof(args.name));
> -		ret = ioctl(fddst, BTRFS_IOC_SUBVOL_CREATE, &args);
> +	if (!*inherit) {
> +		err = btrfs_util_qgroup_inherit_create(0, inherit);
> +		if (err) {
> +			error_btrfs_util(err);
> +			return -1;
> +		}
>   	}
>
> -	if (ret < 0) {
> -		error("cannot create subvolume: %m");
> -		goto out;
> +	qgroupid = parse_qgroupid_or_path(optarg);
> +	if (qgroupid == 0) {
> +		error("invalid qgroup specification, qgroupid must not be 0");
> +		return -1;
>   	}
> -	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
>
> -out:
> -	close(fddst);
> -	free(dupname);
> -	free(dupdir);
> +	err = btrfs_util_qgroup_inherit_add_group(inherit, qgroupid);
> +	if (err) {
> +		error_btrfs_util(err);
> +		return -1;
> +	}
>
> -	return ret;
> +	return 0;
>   }
> +
>   static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **argv)
>   {
>   	int retval, ret;
> -	struct btrfs_qgroup_inherit *inherit = NULL;
> +	struct btrfs_util_qgroup_inherit *inherit = NULL;
>   	bool has_error = false;
>   	bool create_parents = false;
>
> @@ -281,7 +266,7 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
>
>   		switch (c) {
>   		case 'i':
> -			ret = btrfs_qgroup_inherit_add_group(&inherit, optarg);
> +			ret = qgroup_inherit_add_group(&inherit, optarg);
>   			if (ret) {
>   				retval = ret;
>   				goto out;
> @@ -304,13 +289,14 @@ static int cmd_subvolume_create(const struct cmd_struct *cmd, int argc, char **a
>
>   	for (int i = optind; i < argc; i++) {
>   		ret = create_one_subvolume(argv[i], inherit, create_parents);
> -		if (ret < 0)
> +		if (ret)
>   			has_error = true;
>   	}
>   	if (!has_error)
>   		retval = 0;
>   out:
> -	free(inherit);
> +	if (inherit)
> +		btrfs_util_qgroup_inherit_destroy(inherit);
>
>   	return retval;
>   }

[PATCH v2 2/3] btrfs-progs: use libbtrfsutil for btrfs subvolume snapshot

[Mark Harmstone]

From: Mark Harmstone <maharmstone@meta.com>

Call btrfs_util_subvolume_snapshot in cmd_subvolume_snapshot rather than
calling the ioctl directly.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
Co-authored-by: Omar Sandoval <osandov@fb.com>
---
 cmds/subvolume.c | 94 +++++++++++++++++-------------------------------
 1 file changed, 33 insertions(+), 61 deletions(-)

diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 2a635fa2..a9664039 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -635,18 +635,11 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
 {
 	char	*subvol, *dst;
 	int	res, retval;
-	int	fd = -1, fddst = -1;
-	int	len;
-	bool readonly = false;
-	char	*dupname = NULL;
-	char	*dupdir = NULL;
-	const char *newname;
-	char	*dstdir;
+	char	*dstdir = NULL;
 	enum btrfs_util_error err;
-	struct btrfs_ioctl_vol_args_v2	args;
-	struct btrfs_qgroup_inherit *inherit = NULL;
+	struct btrfs_util_qgroup_inherit *inherit = NULL;
+	int flags = 0;
 
-	memset(&args, 0, sizeof(args));
 	optind = 0;
 	while (1) {
 		int c = getopt(argc, argv, "i:r");
@@ -655,14 +648,14 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
 
 		switch (c) {
 		case 'i':
-			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
+			res = qgroup_inherit_add_group(&inherit, optarg);
 			if (res) {
 				retval = res;
 				goto out;
 			}
 			break;
 		case 'r':
-			readonly = true;
+			flags |= BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY;
 			break;
 		default:
 			usage_unknown_option(cmd, argv);
@@ -696,72 +689,51 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
 	}
 
 	if (res > 0) {
+		char *dupname;
+		const char *newname;
+
 		dupname = strdup(subvol);
 		newname = path_basename(dupname);
-		dstdir = dst;
-	} else {
-		dupname = strdup(dst);
-		newname = path_basename(dupname);
-		dupdir = strdup(dst);
-		dstdir = path_dirname(dupdir);
-	}
-
-	if (!test_issubvolname(newname)) {
-		error("invalid snapshot name '%s'", newname);
-		goto out;
-	}
-
-	len = strlen(newname);
-	if (len > BTRFS_VOL_NAME_MAX) {
-		error("snapshot name too long '%s'", newname);
-		goto out;
-	}
 
-	fddst = btrfs_open_dir(dstdir);
-	if (fddst < 0)
-		goto out;
-
-	fd = btrfs_open_dir(subvol);
-	if (fd < 0)
-		goto out;
+		dstdir = malloc(strlen(dst) + 1 + strlen(newname) + 1);
+		if (!dstdir) {
+			error("out of memory");
+			free(dupname);
+			goto out;
+		}
 
-	if (readonly)
-		args.flags |= BTRFS_SUBVOL_RDONLY;
+		dstdir[0] = 0;
+		strcpy(dstdir, dst);
+		strcat(dstdir, "/");
+		strcat(dstdir, newname);
 
-	args.fd = fd;
-	if (inherit) {
-		args.flags |= BTRFS_SUBVOL_QGROUP_INHERIT;
-		args.size = btrfs_qgroup_inherit_size(inherit);
-		args.qgroup_inherit = inherit;
+		free(dupname);
+	} else {
+		dstdir = strdup(dst);
 	}
-	strncpy_null(args.name, newname, sizeof(args.name));
 
-	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
-	if (res < 0) {
-		if (errno == ETXTBSY)
-			error("cannot snapshot '%s': source subvolume contains an active swapfile (%m)", subvol);
-		else
-			error("cannot snapshot '%s': %m", subvol);
+	err = btrfs_util_subvolume_snapshot(subvol, dstdir, flags, NULL, inherit);
+	if (err) {
+		error_btrfs_util(err);
 		goto out;
 	}
 
 	retval = 0;	/* success */
 
-	if (readonly)
+	if (flags & BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY)
 		pr_verbose(LOG_DEFAULT,
-			   "Create readonly snapshot of '%s' in '%s/%s'\n",
-			   subvol, dstdir, newname);
+			   "Create readonly snapshot of '%s' in '%s'\n",
+			   subvol, dstdir);
 	else
 		pr_verbose(LOG_DEFAULT,
-			   "Create snapshot of '%s' in '%s/%s'\n",
-			   subvol, dstdir, newname);
+			   "Create snapshot of '%s' in '%s'\n",
+			   subvol, dstdir);
 
 out:
-	close(fddst);
-	close(fd);
-	free(inherit);
-	free(dupname);
-	free(dupdir);
+	free(dstdir);
+
+	if (inherit)
+		btrfs_util_qgroup_inherit_destroy(inherit);
 
 	return retval;
 }
-- 
2.44.2

Re: [PATCH v2 2/3] btrfs-progs: use libbtrfsutil for btrfs subvolume snapshot

[Qu Wenruo]

在 2024/8/2 20:57, Mark Harmstone 写道:
> From: Mark Harmstone <maharmstone@meta.com>
>
> Call btrfs_util_subvolume_snapshot in cmd_subvolume_snapshot rather than
> calling the ioctl directly.
>
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> Co-authored-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   cmds/subvolume.c | 94 +++++++++++++++++-------------------------------
>   1 file changed, 33 insertions(+), 61 deletions(-)
>
> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> index 2a635fa2..a9664039 100644
> --- a/cmds/subvolume.c
> +++ b/cmds/subvolume.c
> @@ -635,18 +635,11 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
>   {
>   	char	*subvol, *dst;
>   	int	res, retval;
> -	int	fd = -1, fddst = -1;
> -	int	len;
> -	bool readonly = false;
> -	char	*dupname = NULL;
> -	char	*dupdir = NULL;
> -	const char *newname;
> -	char	*dstdir;
> +	char	*dstdir = NULL;
>   	enum btrfs_util_error err;
> -	struct btrfs_ioctl_vol_args_v2	args;
> -	struct btrfs_qgroup_inherit *inherit = NULL;
> +	struct btrfs_util_qgroup_inherit *inherit = NULL;
> +	int flags = 0;
>
> -	memset(&args, 0, sizeof(args));
>   	optind = 0;
>   	while (1) {
>   		int c = getopt(argc, argv, "i:r");
> @@ -655,14 +648,14 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
>
>   		switch (c) {
>   		case 'i':
> -			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
> +			res = qgroup_inherit_add_group(&inherit, optarg);
>   			if (res) {
>   				retval = res;
>   				goto out;
>   			}
>   			break;
>   		case 'r':
> -			readonly = true;
> +			flags |= BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY;
>   			break;
>   		default:
>   			usage_unknown_option(cmd, argv);
> @@ -696,72 +689,51 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
>   	}
>
>   	if (res > 0) {
> +		char *dupname;
> +		const char *newname;
> +
>   		dupname = strdup(subvol);
>   		newname = path_basename(dupname);
> -		dstdir = dst;
> -	} else {
> -		dupname = strdup(dst);
> -		newname = path_basename(dupname);
> -		dupdir = strdup(dst);
> -		dstdir = path_dirname(dupdir);
> -	}
> -
> -	if (!test_issubvolname(newname)) {
> -		error("invalid snapshot name '%s'", newname);
> -		goto out;
> -	}
> -
> -	len = strlen(newname);
> -	if (len > BTRFS_VOL_NAME_MAX) {
> -		error("snapshot name too long '%s'", newname);
> -		goto out;
> -	}
>
> -	fddst = btrfs_open_dir(dstdir);
> -	if (fddst < 0)
> -		goto out;
> -
> -	fd = btrfs_open_dir(subvol);
> -	if (fd < 0)
> -		goto out;
> +		dstdir = malloc(strlen(dst) + 1 + strlen(newname) + 1);
> +		if (!dstdir) {
> +			error("out of memory");
> +			free(dupname);
> +			goto out;
> +		}
>
> -	if (readonly)
> -		args.flags |= BTRFS_SUBVOL_RDONLY;
> +		dstdir[0] = 0;
> +		strcpy(dstdir, dst);
> +		strcat(dstdir, "/");
> +		strcat(dstdir, newname);
>
> -	args.fd = fd;
> -	if (inherit) {
> -		args.flags |= BTRFS_SUBVOL_QGROUP_INHERIT;
> -		args.size = btrfs_qgroup_inherit_size(inherit);
> -		args.qgroup_inherit = inherit;
> +		free(dupname);
> +	} else {
> +		dstdir = strdup(dst);
>   	}
> -	strncpy_null(args.name, newname, sizeof(args.name));
>
> -	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
> -	if (res < 0) {
> -		if (errno == ETXTBSY)
> -			error("cannot snapshot '%s': source subvolume contains an active swapfile (%m)", subvol);
> -		else
> -			error("cannot snapshot '%s': %m", subvol);
> +	err = btrfs_util_subvolume_snapshot(subvol, dstdir, flags, NULL, inherit);
> +	if (err) {
> +		error_btrfs_util(err);
>   		goto out;
>   	}
>
>   	retval = 0;	/* success */
>
> -	if (readonly)
> +	if (flags & BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY)
>   		pr_verbose(LOG_DEFAULT,
> -			   "Create readonly snapshot of '%s' in '%s/%s'\n",
> -			   subvol, dstdir, newname);
> +			   "Create readonly snapshot of '%s' in '%s'\n",
> +			   subvol, dstdir);
>   	else
>   		pr_verbose(LOG_DEFAULT,
> -			   "Create snapshot of '%s' in '%s/%s'\n",
> -			   subvol, dstdir, newname);
> +			   "Create snapshot of '%s' in '%s'\n",
> +			   subvol, dstdir);
>
>   out:
> -	close(fddst);
> -	close(fd);
> -	free(inherit);
> -	free(dupname);
> -	free(dupdir);
> +	free(dstdir);
> +
> +	if (inherit)
> +		btrfs_util_qgroup_inherit_destroy(inherit);
>
>   	return retval;
>   }

[PATCH v2 3/3] btrfs-progs: remove unused qgroup functions

[Mark Harmstone]

From: Omar Sandoval <osandov@fb.com>

Remove functions that after the previous two patches are no longer
referenced.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
Co-authored-by: Omar Sandoval <osandov@fb.com>
---
 cmds/qgroup.c | 64 ---------------------------------------------------
 cmds/qgroup.h |  2 --
 2 files changed, 66 deletions(-)

diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index 20b97f7a..57052861 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -1688,70 +1688,6 @@ out:
 	return ret;
 }
 
-int btrfs_qgroup_inherit_size(struct btrfs_qgroup_inherit *p)
-{
-	return sizeof(*p) + sizeof(p->qgroups[0]) *
-			    (p->num_qgroups + 2 * p->num_ref_copies +
-			     2 * p->num_excl_copies);
-}
-
-static int qgroup_inherit_realloc(struct btrfs_qgroup_inherit **inherit, int n,
-		int pos)
-{
-	struct btrfs_qgroup_inherit *out;
-	int nitems = 0;
-
-	if (*inherit) {
-		nitems = (*inherit)->num_qgroups +
-			 (*inherit)->num_ref_copies +
-			 (*inherit)->num_excl_copies;
-	}
-
-	out = calloc(1, sizeof(*out) + sizeof(out->qgroups[0]) * (nitems + n));
-	if (out == NULL) {
-		error_msg(ERROR_MSG_MEMORY, NULL);
-		return -ENOMEM;
-	}
-
-	if (*inherit) {
-		struct btrfs_qgroup_inherit *i = *inherit;
-		int s = sizeof(out->qgroups[0]);
-
-		out->num_qgroups = i->num_qgroups;
-		out->num_ref_copies = i->num_ref_copies;
-		out->num_excl_copies = i->num_excl_copies;
-		memcpy(out->qgroups, i->qgroups, pos * s);
-		memcpy(out->qgroups + pos + n, i->qgroups + pos,
-		       (nitems - pos) * s);
-	}
-	free(*inherit);
-	*inherit = out;
-
-	return 0;
-}
-
-int btrfs_qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg)
-{
-	int ret;
-	u64 qgroupid = parse_qgroupid_or_path(arg);
-	int pos = 0;
-
-	if (qgroupid == 0) {
-		error("invalid qgroup specification, qgroupid must not 0");
-		return -EINVAL;
-	}
-
-	if (*inherit)
-		pos = (*inherit)->num_qgroups;
-	ret = qgroup_inherit_realloc(inherit, 1, pos);
-	if (ret)
-		return ret;
-
-	(*inherit)->qgroups[(*inherit)->num_qgroups++] = qgroupid;
-
-	return 0;
-}
-
 static const char * const qgroup_cmd_group_usage[] = {
 	"btrfs qgroup <command> [options] <path>",
 	NULL
diff --git a/cmds/qgroup.h b/cmds/qgroup.h
index 1fc10722..32309ce4 100644
--- a/cmds/qgroup.h
+++ b/cmds/qgroup.h
@@ -36,8 +36,6 @@ struct btrfs_qgroup_stats {
 	struct btrfs_qgroup_limit limit;
 };
 
-int btrfs_qgroup_inherit_size(struct btrfs_qgroup_inherit *p);
-int btrfs_qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg);
 int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats);
 
 #endif
-- 
2.44.2

Re: [PATCH v2 3/3] btrfs-progs: remove unused qgroup functions

[Qu Wenruo]

在 2024/8/2 20:57, Mark Harmstone 写道:
> From: Omar Sandoval <osandov@fb.com>
>
> Remove functions that after the previous two patches are no longer
> referenced.
>
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> Co-authored-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
qu
> ---
>   cmds/qgroup.c | 64 ---------------------------------------------------
>   cmds/qgroup.h |  2 --
>   2 files changed, 66 deletions(-)
>
> diff --git a/cmds/qgroup.c b/cmds/qgroup.c
> index 20b97f7a..57052861 100644
> --- a/cmds/qgroup.c
> +++ b/cmds/qgroup.c
> @@ -1688,70 +1688,6 @@ out:
>   	return ret;
>   }
>
> -int btrfs_qgroup_inherit_size(struct btrfs_qgroup_inherit *p)
> -{
> -	return sizeof(*p) + sizeof(p->qgroups[0]) *
> -			    (p->num_qgroups + 2 * p->num_ref_copies +
> -			     2 * p->num_excl_copies);
> -}
> -
> -static int qgroup_inherit_realloc(struct btrfs_qgroup_inherit **inherit, int n,
> -		int pos)
> -{
> -	struct btrfs_qgroup_inherit *out;
> -	int nitems = 0;
> -
> -	if (*inherit) {
> -		nitems = (*inherit)->num_qgroups +
> -			 (*inherit)->num_ref_copies +
> -			 (*inherit)->num_excl_copies;
> -	}
> -
> -	out = calloc(1, sizeof(*out) + sizeof(out->qgroups[0]) * (nitems + n));
> -	if (out == NULL) {
> -		error_msg(ERROR_MSG_MEMORY, NULL);
> -		return -ENOMEM;
> -	}
> -
> -	if (*inherit) {
> -		struct btrfs_qgroup_inherit *i = *inherit;
> -		int s = sizeof(out->qgroups[0]);
> -
> -		out->num_qgroups = i->num_qgroups;
> -		out->num_ref_copies = i->num_ref_copies;
> -		out->num_excl_copies = i->num_excl_copies;
> -		memcpy(out->qgroups, i->qgroups, pos * s);
> -		memcpy(out->qgroups + pos + n, i->qgroups + pos,
> -		       (nitems - pos) * s);
> -	}
> -	free(*inherit);
> -	*inherit = out;
> -
> -	return 0;
> -}
> -
> -int btrfs_qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg)
> -{
> -	int ret;
> -	u64 qgroupid = parse_qgroupid_or_path(arg);
> -	int pos = 0;
> -
> -	if (qgroupid == 0) {
> -		error("invalid qgroup specification, qgroupid must not 0");
> -		return -EINVAL;
> -	}
> -
> -	if (*inherit)
> -		pos = (*inherit)->num_qgroups;
> -	ret = qgroup_inherit_realloc(inherit, 1, pos);
> -	if (ret)
> -		return ret;
> -
> -	(*inherit)->qgroups[(*inherit)->num_qgroups++] = qgroupid;
> -
> -	return 0;
> -}
> -
>   static const char * const qgroup_cmd_group_usage[] = {
>   	"btrfs qgroup <command> [options] <path>",
>   	NULL
> diff --git a/cmds/qgroup.h b/cmds/qgroup.h
> index 1fc10722..32309ce4 100644
> --- a/cmds/qgroup.h
> +++ b/cmds/qgroup.h
> @@ -36,8 +36,6 @@ struct btrfs_qgroup_stats {
>   	struct btrfs_qgroup_limit limit;
>   };
>
> -int btrfs_qgroup_inherit_size(struct btrfs_qgroup_inherit *p);
> -int btrfs_qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg);
>   int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats);
>
>   #endif

Re: [PATCH v2 0/3] btrfs-progs: use libbtrfsutil for subvolume creation

[Neal Gompa]

On Fri, Aug 2, 2024 at 7:27 AM Mark Harmstone <maharmstone@fb.com> wrote:
>
> These patches are a resending of Omar Sandoval's patch from 2018, which
> appears to have been overlooked [0], split up and rebased against the
> current code.
>
> We change btrfs subvol create and btrfs subvol snapshot so that they use
> libbtrfsutil rather than calling the ioctl directly.
>
> [0] https://lore.kernel.org/linux-btrfs/ab09ba595157b7fb6606814730508cae4da48caf.1516991902.git.osandov@fb.com/
>
> Changelog:
> * Fixed deprecated function names
> * Fixed test failures (now returns correct return value on failure)
> * Fixed this breaking fstest btrfs/300 (thanks Boris)
>
> Mark Harmstone (3):
>   btrfs-progs: use libbtrfsutil for btrfs subvolume create
>   btrfs-progs: use libbtrfsutil for btrfs subvolume snapshot
>   btrfs-progs: remove unused qgroup functions
>
>  cmds/qgroup.c    |  64 ----------------
>  cmds/qgroup.h    |   2 -
>  cmds/subvolume.c | 194 +++++++++++++++++++----------------------------
>  3 files changed, 76 insertions(+), 184 deletions(-)
>
> --
> 2.44.2
>
>

Series looks good to me.

Reviewed-by: Neal Gompa <neal@gompa.dev>



-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH v2 0/3] btrfs-progs: use libbtrfsutil for subvolume creation

[Qu Wenruo]

在 2024/8/2 20:57, Mark Harmstone 写道:
> These patches are a resending of Omar Sandoval's patch from 2018, which
> appears to have been overlooked [0], split up and rebased against the
> current code.
>
> We change btrfs subvol create and btrfs subvol snapshot so that they use
> libbtrfsutil rather than calling the ioctl directly.
>
> [0] https://lore.kernel.org/linux-btrfs/ab09ba595157b7fb6606814730508cae4da48caf.1516991902.git.osandov@fb.com/

Since you're reviving the cleanups, you may also want to move some
btrfs-progs' ioctl related functions to libbtrfsutils.

One example is btrfs_lookup_uuid_subvol_item() and
btrfs_lookup_uuid_received_subvol_item().

With them moved to libbtrfsutils, we can mark
kernel-shared/uuid-tree.[ch] as fully cross-ported from kernel.

Thanks,
Qu
>
> Changelog:
> * Fixed deprecated function names
> * Fixed test failures (now returns correct return value on failure)
> * Fixed this breaking fstest btrfs/300 (thanks Boris)
>
> Mark Harmstone (3):
>    btrfs-progs: use libbtrfsutil for btrfs subvolume create
>    btrfs-progs: use libbtrfsutil for btrfs subvolume snapshot
>    btrfs-progs: remove unused qgroup functions
>
>   cmds/qgroup.c    |  64 ----------------
>   cmds/qgroup.h    |   2 -
>   cmds/subvolume.c | 194 +++++++++++++++++++----------------------------
>   3 files changed, 76 insertions(+), 184 deletions(-)
>

Re: [PATCH v2 0/3] btrfs-progs: use libbtrfsutil for subvolume creation

[Mark Harmstone]

Thanks Qu, I'll have a look at them.

Mark

On 5/8/24 01:46, Qu Wenruo wrote:
> Since you're reviving the cleanups, you may also want to move some
> btrfs-progs' ioctl related functions to libbtrfsutils.
> 
> One example is btrfs_lookup_uuid_subvol_item() and
> btrfs_lookup_uuid_received_subvol_item().
> 
> With them moved to libbtrfsutils, we can mark
> kernel-shared/uuid-tree.[ch] as fully cross-ported from kernel.