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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 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;
>   }