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

[Sidong Yang <realwakka@gmail.com>]

On Sat, Feb 19, 2022 at 03:11:36PM +0100, Goffredo Baroncelli wrote:

Hi, Thanks for review.

> On 19/02/2022 10.01, 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>
> > ---
> >   cmds/subvolume.c | 31 ++++++++++++++++++++++++++++++-
> >   1 file changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> > index fbf56566..1070c74a 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",
> 
> you should update the man page too

Thanks I'll do it for next version.
> 
> >   	HELPINFO_INSERT_GLOBALS,
> >   	HELPINFO_INSERT_QUIET,
> >   	NULL
> 
> [...]
> 
> > +	if (create_parents) {
> > +		char p[PATH_MAX];
> > +		char dstdir_real[PATH_MAX];
> > +		char *token;
> > +
> > +		realpath(dstdir, dstdir_real);
> 
> realpath(3) behaves strangely when the path doesn't exist: if exists only /tmp, realpath("/tmp/1/2/3/4/5") returns "/tmp/1" discarding 2/3/4....
> In fact my gcc warns:
> 
> $ make
>     [CC]     cmds/subvolume.o
> cmds/subvolume.c: In function ‘cmd_subvol_create’:
> cmds/subvolume.c:180:17: warning: ignoring return value of ‘realpath’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
>   180 |                 realpath(dstdir, dstdir_real);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> because realpath(3) returns an error if the path doesn't exist.
> 
> This causes:
> 
> # only /tmp/ exists
> btrfs sub cre -p /tmp/1/2/3/4/5/6
> ERROR: cannot access '/tmp/1/2/3/4/5': No such file or directory

Yes, It's completely wrong code. I'll correct it. For now, I think I
need to check whether the path is absolute or relative for checking
dstdir[0] == '/'.
> 
> 
> > +		token = strtok(dstdir_real, "/");
> > +		while(token) {
> > +			strcat(p, "/");
> 
> where is initialized p ?

I missed it. Thanks.
> 
> Unfortunately it works and the gcc doesn't warn, but I think only because in the stack there are zeros where p is allocated; but this is not guarantee.
> 
> > +			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;
> > +			}
> > +			token = strtok(NULL, "/");
> > +		}
> > +	}
> > +
> >   	fddst = btrfs_open_dir(dstdir, &dirstream, 1);
> >   	if (fddst < 0)
> >   		goto out;
> 
> 
> -- 
> gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
> Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5