[PATCH v2] btrfs-progs: cmds: subvolume: add -p option on creating subvol

[PATCH v2] btrfs-progs: cmds: subvolume: add -p option on creating subvol

[Sidong Yang]

This patch resolves github issue #429. This patch adds an option that
create parent directories when it creates subvolumes on nonexisting
parents. This option tokenizes dstdir and checks each paths whether
it's existing directory and make directory for creating subvolume.

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
v2: fixed the case using realpath() that path nonexists, added
description on docs.
---
 Documentation/btrfs-subvolume.rst |  3 +++
 cmds/subvolume.c                  | 34 ++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/Documentation/btrfs-subvolume.rst b/Documentation/btrfs-subvolume.rst
index 4591d4bb..89809822 100644
--- a/Documentation/btrfs-subvolume.rst
+++ b/Documentation/btrfs-subvolume.rst
@@ -61,6 +61,9 @@ create [-i <qgroupid>] [<dest>/]<name>
                 Add the newly created subvolume to a qgroup. This option can be given multiple
                 times.
 
+        -p
+                Make any missing parent directories for each argument.
+
 delete [options] [<subvolume> [<subvolume>...]], delete -i|--subvolid <subvolid> <path>
         Delete the subvolume(s) from the filesystem.
 
diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index fbf56566..7df0d2ec 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -88,6 +88,7 @@ static const char * const cmd_subvol_create_usage[] = {
 	"",
 	"-i <qgroupid>  add the newly created subvolume to a qgroup. This",
 	"               option can be given multiple times.",
+	"-p             make any missing parent directories for each argument.",
 	HELPINFO_INSERT_GLOBALS,
 	HELPINFO_INSERT_QUIET,
 	NULL
@@ -105,10 +106,11 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
 	char	*dst;
 	struct btrfs_qgroup_inherit *inherit = NULL;
 	DIR	*dirstream = NULL;
+	bool create_parents = false;
 
 	optind = 0;
 	while (1) {
-		int c = getopt(argc, argv, "c:i:");
+		int c = getopt(argc, argv, "c:i:p");
 		if (c < 0)
 			break;
 
@@ -127,6 +129,9 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
 				goto out;
 			}
 			break;
+		case 'p':
+			create_parents = true;
+			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
@@ -167,6 +172,33 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
 		goto out;
 	}
 
+	if (create_parents) {
+		char p[PATH_MAX] = {0};
+		char dstdir_dup[PATH_MAX];
+		char *token;
+
+		strcpy(dstdir_dup, dstdir);
+		if (dstdir_dup[0] == '/')
+			strcat(p, "/");
+
+		token = strtok(dstdir_dup, "/");
+		while(token) {
+			strcat(p, token);
+			res = path_is_dir(p);
+			if (res == -ENOENT) {
+				if (mkdir(p, 0755)) {
+					error("failed to make dir: %s", p);
+					goto out;
+				}
+			} else if (res <= 0) {
+				error("failed to check dir: %s", p);				
+				goto out;
+			}
+			strcat(p, "/");
+			token = strtok(NULL, "/");
+		}
+	}
+
 	fddst = btrfs_open_dir(dstdir, &dirstream, 1);
 	if (fddst < 0)
 		goto out;
-- 
2.25.1

Re: [PATCH v2] btrfs-progs: cmds: subvolume: add -p option on creating subvol

[Graham Cobb]

On 19/02/2022 15:11, Sidong Yang wrote:
> This patch resolves github issue #429. This patch adds an option that
> create parent directories when it creates subvolumes on nonexisting
> parents. This option tokenizes dstdir and checks each paths whether
> it's existing directory and make directory for creating subvolume.
> 
> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> ---
> v2: fixed the case using realpath() that path nonexists, added
> description on docs.
> ---
>  Documentation/btrfs-subvolume.rst |  3 +++
>  cmds/subvolume.c                  | 34 ++++++++++++++++++++++++++++++-
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/btrfs-subvolume.rst b/Documentation/btrfs-subvolume.rst
> index 4591d4bb..89809822 100644
> --- a/Documentation/btrfs-subvolume.rst
> +++ b/Documentation/btrfs-subvolume.rst
> @@ -61,6 +61,9 @@ create [-i <qgroupid>] [<dest>/]<name>
>                  Add the newly created subvolume to a qgroup. This option can be given multiple
>                  times.
>  
> +        -p
> +                Make any missing parent directories for each argument.
> +
>  delete [options] [<subvolume> [<subvolume>...]], delete -i|--subvolid <subvolid> <path>
>          Delete the subvolume(s) from the filesystem.
>  
> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> index fbf56566..7df0d2ec 100644
> --- a/cmds/subvolume.c
> +++ b/cmds/subvolume.c
> @@ -88,6 +88,7 @@ static const char * const cmd_subvol_create_usage[] = {
>  	"",
>  	"-i <qgroupid>  add the newly created subvolume to a qgroup. This",
>  	"               option can be given multiple times.",
> +	"-p             make any missing parent directories for each argument.",
>  	HELPINFO_INSERT_GLOBALS,
>  	HELPINFO_INSERT_QUIET,
>  	NULL
> @@ -105,10 +106,11 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
>  	char	*dst;
>  	struct btrfs_qgroup_inherit *inherit = NULL;
>  	DIR	*dirstream = NULL;
> +	bool create_parents = false;
>  
>  	optind = 0;
>  	while (1) {
> -		int c = getopt(argc, argv, "c:i:");
> +		int c = getopt(argc, argv, "c:i:p");
>  		if (c < 0)
>  			break;
>  
> @@ -127,6 +129,9 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
>  				goto out;
>  			}
>  			break;
> +		case 'p':
> +			create_parents = true;
> +			break;
>  		default:
>  			usage_unknown_option(cmd, argv);
>  		}
> @@ -167,6 +172,33 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
>  		goto out;
>  	}
>  
> +	if (create_parents) {
> +		char p[PATH_MAX] = {0};

I'm not a fan of single letter variable names, especially for
substantive objects such as large character arrays (as opposed to a
pointer keeping track of a position in some other object). But maybe
that is just me.

> +		char dstdir_dup[PATH_MAX];
> +		char *token;
> +
> +		strcpy(dstdir_dup, dstdir);

Has dstdir already been checked its length is PATH_MAX-1 or less?
Wouldn't it be safer to at least use strncpy plus overwriting the final
byte with zero?

> +		if (dstdir_dup[0] == '/')
> +			strcat(p, "/");
> +
> +		token = strtok(dstdir_dup, "/");
> +		while(token) {
> +			strcat(p, token);
> +			res = path_is_dir(p);
> +			if (res == -ENOENT) {
> +				if (mkdir(p, 0755)) {

Why 0755? Personally I make use of groups and run with umask 0002. Why
not just use 0777? I realise this is unlikely to be significantly
important but I dislike programs overriding the user's choice of umask.
I assume the requestor's intention was that the behaviour should be the
same as "mkdir -p".

Graham

> +					error("failed to make dir: %s", p);
> +					goto out;
> +				}
> +			} else if (res <= 0) {
> +				error("failed to check dir: %s", p);				
> +				goto out;
> +			}
> +			strcat(p, "/");
> +			token = strtok(NULL, "/");
> +		}
> +	}
> +
>  	fddst = btrfs_open_dir(dstdir, &dirstream, 1);
>  	if (fddst < 0)
>  		goto out;

Re: [PATCH v2] btrfs-progs: cmds: subvolume: add -p option on creating subvol

[Sidong Yang]

On Sat, Feb 19, 2022 at 04:38:03PM +0000, Graham Cobb wrote:

Hi, Cobb.
Thanks for review.
> On 19/02/2022 15:11, Sidong Yang wrote:
> > This patch resolves github issue #429. This patch adds an option that
> > create parent directories when it creates subvolumes on nonexisting
> > parents. This option tokenizes dstdir and checks each paths whether
> > it's existing directory and make directory for creating subvolume.
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> > v2: fixed the case using realpath() that path nonexists, added
> > description on docs.
> > ---
> >  Documentation/btrfs-subvolume.rst |  3 +++
> >  cmds/subvolume.c                  | 34 ++++++++++++++++++++++++++++++-
> >  2 files changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/btrfs-subvolume.rst b/Documentation/btrfs-subvolume.rst
> > index 4591d4bb..89809822 100644
> > --- a/Documentation/btrfs-subvolume.rst
> > +++ b/Documentation/btrfs-subvolume.rst
> > @@ -61,6 +61,9 @@ create [-i <qgroupid>] [<dest>/]<name>
> >                  Add the newly created subvolume to a qgroup. This option can be given multiple
> >                  times.
> >  
> > +        -p
> > +                Make any missing parent directories for each argument.
> > +
> >  delete [options] [<subvolume> [<subvolume>...]], delete -i|--subvolid <subvolid> <path>
> >          Delete the subvolume(s) from the filesystem.
> >  
> > diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> > index fbf56566..7df0d2ec 100644
> > --- a/cmds/subvolume.c
> > +++ b/cmds/subvolume.c
> > @@ -88,6 +88,7 @@ static const char * const cmd_subvol_create_usage[] = {
> >  	"",
> >  	"-i <qgroupid>  add the newly created subvolume to a qgroup. This",
> >  	"               option can be given multiple times.",
> > +	"-p             make any missing parent directories for each argument.",
> >  	HELPINFO_INSERT_GLOBALS,
> >  	HELPINFO_INSERT_QUIET,
> >  	NULL
> > @@ -105,10 +106,11 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
> >  	char	*dst;
> >  	struct btrfs_qgroup_inherit *inherit = NULL;
> >  	DIR	*dirstream = NULL;
> > +	bool create_parents = false;
> >  
> >  	optind = 0;
> >  	while (1) {
> > -		int c = getopt(argc, argv, "c:i:");
> > +		int c = getopt(argc, argv, "c:i:p");
> >  		if (c < 0)
> >  			break;
> >  
> > @@ -127,6 +129,9 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
> >  				goto out;
> >  			}
> >  			break;
> > +		case 'p':
> > +			create_parents = true;
> > +			break;
> >  		default:
> >  			usage_unknown_option(cmd, argv);
> >  		}
> > @@ -167,6 +172,33 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
> >  		goto out;
> >  	}
> >  
> > +	if (create_parents) {
> > +		char p[PATH_MAX] = {0};
> 
> I'm not a fan of single letter variable names, especially for
> substantive objects such as large character arrays (as opposed to a
> pointer keeping track of a position in some other object). But maybe
> that is just me.
> 
I think it would be better that adding longopt "--parents" and it seems
that requestor wants it.

> > +		char dstdir_dup[PATH_MAX];
> > +		char *token;
> > +
> > +		strcpy(dstdir_dup, dstdir);
> 
> Has dstdir already been checked its length is PATH_MAX-1 or less?
> Wouldn't it be safer to at least use strncpy plus overwriting the final
> byte with zero?
> 
Agree, It's not guaranteed that strlen(dstdir) is less than PATH_MAX.
strncpy() is a safer way than just strcpy().

> > +		if (dstdir_dup[0] == '/')
> > +			strcat(p, "/");
> > +
> > +		token = strtok(dstdir_dup, "/");
> > +		while(token) {
> > +			strcat(p, token);
> > +			res = path_is_dir(p);
> > +			if (res == -ENOENT) {
> > +				if (mkdir(p, 0755)) {
> 
> Why 0755? Personally I make use of groups and run with umask 0002. Why
> not just use 0777? I realise this is unlikely to be significantly
> important but I dislike programs overriding the user's choice of umask.
> I assume the requestor's intention was that the behaviour should be the
> same as "mkdir -p".

Thanks, I didn't know that mode parameter in mkdir() works in (mode &
~umask & 0777). Now I realize that using 0777 is correct way to follow
user's choice. I'll fix this for next version.

Thanks,
Sidong
> 
> Graham
> 
> > +					error("failed to make dir: %s", p);
> > +					goto out;
> > +				}
> > +			} else if (res <= 0) {
> > +				error("failed to check dir: %s", p);				
> > +				goto out;
> > +			}
> > +			strcat(p, "/");
> > +			token = strtok(NULL, "/");
> > +		}
> > +	}
> > +
> >  	fddst = btrfs_open_dir(dstdir, &dirstream, 1);
> >  	if (fddst < 0)
> >  		goto out;
>