[PATCH v2] btrfs-progs: subvol: change subvol set-default to also accept subvol path

[PATCH v2] btrfs-progs: subvol: change subvol set-default to also accept subvol path

[Misono, Tomohiro]

This patch changes "subvol set-default" to also accept the subvolume path
for convenience.

This is the one of the issue on github: 
https://github.com/kdave/btrfs-progs/issues/35

If there are two args, they are assumed as subvol id and path to the fs
(the same as current behavior), and if there is only one arg, it is assumed
as the path to the subvolume. Therefore there is no ambiguity between subvol
id and subvol name, which is mentioned in the above issue page.

Only the absolute path to the subvolume is allowed, for the safety when
multiple filesystems are used.

subvol id is resolved by get_subvol_info() which is used by "subvol show".

change to v2:
restrict the path to only allow absolute path.
documents are updated accordingly.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 Documentation/btrfs-subvolume.asciidoc | 11 +++----
 cmds-subvolume.c                       | 53 +++++++++++++++++++++++++++-------
 2 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/Documentation/btrfs-subvolume.asciidoc b/Documentation/btrfs-subvolume.asciidoc
index 5cfe885..7973567 100644
--- a/Documentation/btrfs-subvolume.asciidoc
+++ b/Documentation/btrfs-subvolume.asciidoc
@@ -142,12 +142,13 @@ you can add \'\+' or \'-' in front of each items, \'+' means ascending,
 for --sort you can combine some items together by \',', just like
 --sort=+ogen,-gen,path,rootid.
 
-*set-default* <id> <path>::
-Set the subvolume of the filesystem <path> which is mounted as
-default.
+*set-default* [<path>|<id> <mountpoint>]::
+Set the subvolume of the filesystem which is mounted as default.
 +
-The subvolume is identified by <id>, which is returned by the *subvolume list*
-command.
+Only absolute path is allowed to specify the subvolume.
+Alterantively, the pair of <id> and <mountpoint> can be used. In that
+case, the subvolume is identified by <id>, which is returned by the
+*subvolume list* command. The filesystem is specified by <mountpoint>.
 
 *show* <path>::
 Show information of a given subvolume in the <path>.
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 666f6e0..dda9e73 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -803,28 +803,59 @@ out:
 }
 
 static const char * const cmd_subvol_set_default_usage[] = {
-	"btrfs subvolume set-default <subvolid> <path>",
-	"Set the default subvolume of a filesystem",
+	"btrfs subvolume set-default [<path>|<id> <mountpoint>]",
+	"Set the default subvolume of the filesystem mounted as default.",
+	"The subvolume can be specified by its absolute path,",
+	"or the pair of subvolume id and mount point to the filesystem.",
 	NULL
 };
 
 static int cmd_subvol_set_default(int argc, char **argv)
 {
-	int	ret=0, fd, e;
-	u64	objectid;
-	char	*path;
-	char	*subvolid;
-	DIR	*dirstream = NULL;
+	int ret = 0;
+	int fd, e;
+	u64 objectid;
+	char *path;
+	char *subvolid;
+	DIR *dirstream = NULL;
 
 	clean_args_no_options(argc, argv, cmd_subvol_set_default_usage);
 
-	if (check_argc_exact(argc - optind, 2))
+	if (check_argc_min(argc - optind, 1) ||
+			check_argc_max(argc - optind, 2))
 		usage(cmd_subvol_set_default_usage);
 
-	subvolid = argv[optind];
-	path = argv[optind + 1];
+	if (argc - optind == 1) {
+		/* path to the subvolume is specified */
+		struct root_info ri;
+		char *fullpath;
 
-	objectid = arg_strtou64(subvolid);
+		path = argv[optind];
+		if (path[0] != '/') {
+			error("only absolute path is allowed");
+			return 1;
+		}
+
+		fullpath = realpath(path, NULL);
+		if (!fullpath) {
+			error("cannot find real path for '%s': %s",
+				path, strerror(errno));
+			return 1;
+		}
+
+		ret = get_subvol_info(fullpath, &ri);
+		free(fullpath);
+
+		if (ret)
+			return 1;
+
+		objectid = ri.root_id;
+	} else {
+		/* subvol id and path to the filesystem are specified */
+		subvolid = argv[optind];
+		path = argv[optind + 1];
+		objectid = arg_strtou64(subvolid);
+	}
 
 	fd = btrfs_open_dir(path, &dirstream, 1);
 	if (fd < 0)
-- 
2.9.5

Re: [PATCH v2] btrfs-progs: subvol: change subvol set-default to also accept subvol path

[Andrei Borzenkov]

On Mon, Oct 2, 2017 at 11:19 AM, Misono, Tomohiro
<misono.tomohiro@jp.fujitsu.com> wrote:
> This patch changes "subvol set-default" to also accept the subvolume path
> for convenience.
>
> This is the one of the issue on github:
> https://github.com/kdave/btrfs-progs/issues/35
>
> If there are two args, they are assumed as subvol id and path to the fs
> (the same as current behavior), and if there is only one arg, it is assumed
> as the path to the subvolume. Therefore there is no ambiguity between subvol
> id and subvol name, which is mentioned in the above issue page.
>
> Only the absolute path to the subvolume is allowed, for the safety when
> multiple filesystems are used.
>
> subvol id is resolved by get_subvol_info() which is used by "subvol show".
>
> change to v2:
> restrict the path to only allow absolute path.

This is absolutely arbitrary restriction. Why we can do "btrfs
subvolume create ./relative/path" but cannot do "btrfs subvolume
set-default ./relative/path"?

> documents are updated accordingly.
>
> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
> ---
>  Documentation/btrfs-subvolume.asciidoc | 11 +++----
>  cmds-subvolume.c                       | 53 +++++++++++++++++++++++++++-------
>  2 files changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/btrfs-subvolume.asciidoc b/Documentation/btrfs-subvolume.asciidoc
> index 5cfe885..7973567 100644
> --- a/Documentation/btrfs-subvolume.asciidoc
> +++ b/Documentation/btrfs-subvolume.asciidoc
> @@ -142,12 +142,13 @@ you can add \'\+' or \'-' in front of each items, \'+' means ascending,
>  for --sort you can combine some items together by \',', just like
>  --sort=+ogen,-gen,path,rootid.
>
> -*set-default* <id> <path>::
> -Set the subvolume of the filesystem <path> which is mounted as
> -default.
> +*set-default* [<path>|<id> <mountpoint>]::
> +Set the subvolume of the filesystem which is mounted as default.
>  +
> -The subvolume is identified by <id>, which is returned by the *subvolume list*
> -command.
> +Only absolute path is allowed to specify the subvolume.
> +Alterantively, the pair of <id> and <mountpoint> can be used. In that
> +case, the subvolume is identified by <id>, which is returned by the
> +*subvolume list* command. The filesystem is specified by <mountpoint>.
>
>  *show* <path>::
>  Show information of a given subvolume in the <path>.
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index 666f6e0..dda9e73 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -803,28 +803,59 @@ out:
>  }
>
>  static const char * const cmd_subvol_set_default_usage[] = {
> -       "btrfs subvolume set-default <subvolid> <path>",
> -       "Set the default subvolume of a filesystem",
> +       "btrfs subvolume set-default [<path>|<id> <mountpoint>]",
> +       "Set the default subvolume of the filesystem mounted as default.",
> +       "The subvolume can be specified by its absolute path,",
> +       "or the pair of subvolume id and mount point to the filesystem.",
>         NULL
>  };
>
>  static int cmd_subvol_set_default(int argc, char **argv)
>  {
> -       int     ret=0, fd, e;
> -       u64     objectid;
> -       char    *path;
> -       char    *subvolid;
> -       DIR     *dirstream = NULL;
> +       int ret = 0;
> +       int fd, e;
> +       u64 objectid;
> +       char *path;
> +       char *subvolid;
> +       DIR *dirstream = NULL;
>
>         clean_args_no_options(argc, argv, cmd_subvol_set_default_usage);
>
> -       if (check_argc_exact(argc - optind, 2))
> +       if (check_argc_min(argc - optind, 1) ||
> +                       check_argc_max(argc - optind, 2))
>                 usage(cmd_subvol_set_default_usage);
>
> -       subvolid = argv[optind];
> -       path = argv[optind + 1];
> +       if (argc - optind == 1) {
> +               /* path to the subvolume is specified */
> +               struct root_info ri;
> +               char *fullpath;
>
> -       objectid = arg_strtou64(subvolid);
> +               path = argv[optind];
> +               if (path[0] != '/') {
> +                       error("only absolute path is allowed");
> +                       return 1;
> +               }
> +
> +               fullpath = realpath(path, NULL);
> +               if (!fullpath) {
> +                       error("cannot find real path for '%s': %s",
> +                               path, strerror(errno));
> +                       return 1;
> +               }
> +
> +               ret = get_subvol_info(fullpath, &ri);
> +               free(fullpath);
> +
> +               if (ret)
> +                       return 1;
> +
> +               objectid = ri.root_id;
> +       } else {
> +               /* subvol id and path to the filesystem are specified */
> +               subvolid = argv[optind];
> +               path = argv[optind + 1];
> +               objectid = arg_strtou64(subvolid);
> +       }
>
>         fd = btrfs_open_dir(path, &dirstream, 1);
>         if (fd < 0)
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] btrfs-progs: subvol: change subvol set-default to also accept subvol path

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 5720 bytes --]

On Mon, Oct 02, 2017 at 11:39:05AM +0300, Andrei Borzenkov wrote:
> On Mon, Oct 2, 2017 at 11:19 AM, Misono, Tomohiro
> <misono.tomohiro@jp.fujitsu.com> wrote:
> > This patch changes "subvol set-default" to also accept the subvolume path
> > for convenience.
> >
> > This is the one of the issue on github:
> > https://github.com/kdave/btrfs-progs/issues/35
> >
> > If there are two args, they are assumed as subvol id and path to the fs
> > (the same as current behavior), and if there is only one arg, it is assumed
> > as the path to the subvolume. Therefore there is no ambiguity between subvol
> > id and subvol name, which is mentioned in the above issue page.
> >
> > Only the absolute path to the subvolume is allowed, for the safety when
> > multiple filesystems are used.
> >
> > subvol id is resolved by get_subvol_info() which is used by "subvol show".
> >
> > change to v2:
> > restrict the path to only allow absolute path.
> 
> This is absolutely arbitrary restriction. Why we can do "btrfs
> subvolume create ./relative/path" but cannot do "btrfs subvolume
> set-default ./relative/path"?

   Indeed. In fact, it's precisely the _opposite_ of the way that
every other command works -- you provide the path to the subvolume in
the *current namespace*.

   This approach would be just a major misfeature at this point.

   Hugo.

> > documents are updated accordingly.
> >
> > Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
> > ---
> >  Documentation/btrfs-subvolume.asciidoc | 11 +++----
> >  cmds-subvolume.c                       | 53 +++++++++++++++++++++++++++-------
> >  2 files changed, 48 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/btrfs-subvolume.asciidoc b/Documentation/btrfs-subvolume.asciidoc
> > index 5cfe885..7973567 100644
> > --- a/Documentation/btrfs-subvolume.asciidoc
> > +++ b/Documentation/btrfs-subvolume.asciidoc
> > @@ -142,12 +142,13 @@ you can add \'\+' or \'-' in front of each items, \'+' means ascending,
> >  for --sort you can combine some items together by \',', just like
> >  --sort=+ogen,-gen,path,rootid.
> >
> > -*set-default* <id> <path>::
> > -Set the subvolume of the filesystem <path> which is mounted as
> > -default.
> > +*set-default* [<path>|<id> <mountpoint>]::
> > +Set the subvolume of the filesystem which is mounted as default.
> >  +
> > -The subvolume is identified by <id>, which is returned by the *subvolume list*
> > -command.
> > +Only absolute path is allowed to specify the subvolume.
> > +Alterantively, the pair of <id> and <mountpoint> can be used. In that
> > +case, the subvolume is identified by <id>, which is returned by the
> > +*subvolume list* command. The filesystem is specified by <mountpoint>.
> >
> >  *show* <path>::
> >  Show information of a given subvolume in the <path>.
> > diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> > index 666f6e0..dda9e73 100644
> > --- a/cmds-subvolume.c
> > +++ b/cmds-subvolume.c
> > @@ -803,28 +803,59 @@ out:
> >  }
> >
> >  static const char * const cmd_subvol_set_default_usage[] = {
> > -       "btrfs subvolume set-default <subvolid> <path>",
> > -       "Set the default subvolume of a filesystem",
> > +       "btrfs subvolume set-default [<path>|<id> <mountpoint>]",
> > +       "Set the default subvolume of the filesystem mounted as default.",
> > +       "The subvolume can be specified by its absolute path,",
> > +       "or the pair of subvolume id and mount point to the filesystem.",
> >         NULL
> >  };
> >
> >  static int cmd_subvol_set_default(int argc, char **argv)
> >  {
> > -       int     ret=0, fd, e;
> > -       u64     objectid;
> > -       char    *path;
> > -       char    *subvolid;
> > -       DIR     *dirstream = NULL;
> > +       int ret = 0;
> > +       int fd, e;
> > +       u64 objectid;
> > +       char *path;
> > +       char *subvolid;
> > +       DIR *dirstream = NULL;
> >
> >         clean_args_no_options(argc, argv, cmd_subvol_set_default_usage);
> >
> > -       if (check_argc_exact(argc - optind, 2))
> > +       if (check_argc_min(argc - optind, 1) ||
> > +                       check_argc_max(argc - optind, 2))
> >                 usage(cmd_subvol_set_default_usage);
> >
> > -       subvolid = argv[optind];
> > -       path = argv[optind + 1];
> > +       if (argc - optind == 1) {
> > +               /* path to the subvolume is specified */
> > +               struct root_info ri;
> > +               char *fullpath;
> >
> > -       objectid = arg_strtou64(subvolid);
> > +               path = argv[optind];
> > +               if (path[0] != '/') {
> > +                       error("only absolute path is allowed");
> > +                       return 1;
> > +               }
> > +
> > +               fullpath = realpath(path, NULL);
> > +               if (!fullpath) {
> > +                       error("cannot find real path for '%s': %s",
> > +                               path, strerror(errno));
> > +                       return 1;
> > +               }
> > +
> > +               ret = get_subvol_info(fullpath, &ri);
> > +               free(fullpath);
> > +
> > +               if (ret)
> > +                       return 1;
> > +
> > +               objectid = ri.root_id;
> > +       } else {
> > +               /* subvol id and path to the filesystem are specified */
> > +               subvolid = argv[optind];
> > +               path = argv[optind + 1];
> > +               objectid = arg_strtou64(subvolid);
> > +       }
> >
> >         fd = btrfs_open_dir(path, &dirstream, 1);
> >         if (fd < 0)

-- 
Hugo Mills             | Great oxymorons of the world, no. 4:
hugo@... carfax.org.uk | Future Perfect
http://carfax.org.uk/  |
PGP: E2AB1DE4          |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] btrfs-progs: subvol: change subvol set-default to also accept subvol path

[Misono, Tomohiro]

On 2017/10/02 18:01, Hugo Mills wrote:
> On Mon, Oct 02, 2017 at 11:39:05AM +0300, Andrei Borzenkov wrote:
>> On Mon, Oct 2, 2017 at 11:19 AM, Misono, Tomohiro
>> <misono.tomohiro@jp.fujitsu.com> wrote:
>>> This patch changes "subvol set-default" to also accept the subvolume path
>>> for convenience.
>>>
>>> This is the one of the issue on github:
>>> https://github.com/kdave/btrfs-progs/issues/35
>>>
>>> If there are two args, they are assumed as subvol id and path to the fs
>>> (the same as current behavior), and if there is only one arg, it is assumed
>>> as the path to the subvolume. Therefore there is no ambiguity between subvol
>>> id and subvol name, which is mentioned in the above issue page.
>>>
>>> Only the absolute path to the subvolume is allowed, for the safety when
>>> multiple filesystems are used.
>>>
>>> subvol id is resolved by get_subvol_info() which is used by "subvol show".
>>>
>>> change to v2:
>>> restrict the path to only allow absolute path.
>>
>> This is absolutely arbitrary restriction. Why we can do "btrfs
>> subvolume create ./relative/path" but cannot do "btrfs subvolume
>> set-default ./relative/path"?
> 
>    Indeed. In fact, it's precisely the _opposite_ of the way that
> every other command works -- you provide the path to the subvolume in
> the *current namespace*.
> 
>    This approach would be just a major misfeature at this point.
> 
>    Hugo.
> 

Ok, I understood the point and want to revert this.
Please review the first version if possible:
https://mail-archive.com/linux-btrfs@vger.kernel.org/msg68486.html

Thanks,
Tomohiro

Re: [PATCH v2] btrfs-progs: subvol: change subvol set-default to also accept subvol path

[David Sterba]

On Tue, Oct 03, 2017 at 08:57:52AM +0900, Misono, Tomohiro wrote:
> On 2017/10/02 18:01, Hugo Mills wrote:
> > On Mon, Oct 02, 2017 at 11:39:05AM +0300, Andrei Borzenkov wrote:
> >> On Mon, Oct 2, 2017 at 11:19 AM, Misono, Tomohiro
> >> <misono.tomohiro@jp.fujitsu.com> wrote:
> >>> This patch changes "subvol set-default" to also accept the subvolume path
> >>> for convenience.
> >>>
> >>> This is the one of the issue on github:
> >>> https://github.com/kdave/btrfs-progs/issues/35
> >>>
> >>> If there are two args, they are assumed as subvol id and path to the fs
> >>> (the same as current behavior), and if there is only one arg, it is assumed
> >>> as the path to the subvolume. Therefore there is no ambiguity between subvol
> >>> id and subvol name, which is mentioned in the above issue page.
> >>>
> >>> Only the absolute path to the subvolume is allowed, for the safety when
> >>> multiple filesystems are used.
> >>>
> >>> subvol id is resolved by get_subvol_info() which is used by "subvol show".
> >>>
> >>> change to v2:
> >>> restrict the path to only allow absolute path.
> >>
> >> This is absolutely arbitrary restriction. Why we can do "btrfs
> >> subvolume create ./relative/path" but cannot do "btrfs subvolume
> >> set-default ./relative/path"?
> > 
> >    Indeed. In fact, it's precisely the _opposite_ of the way that
> > every other command works -- you provide the path to the subvolume in
> > the *current namespace*.
> > 
> >    This approach would be just a major misfeature at this point.
> Ok, I understood the point and want to revert this.
> Please review the first version if possible:
> https://mail-archive.com/linux-btrfs@vger.kernel.org/msg68486.html

I agree with Andrei and Hugo. We need to check that the subvolume path
belongs to the filesystem anyway. I don't see that in the first version,
so please fix it.

Re: [PATCH v2] btrfs-progs: subvol: change subvol set-default to also accept subvol path

[Misono, Tomohiro]

On 2017/10/05 3:07, David Sterba wrote:
> On Tue, Oct 03, 2017 at 08:57:52AM +0900, Misono, Tomohiro wrote:
>> On 2017/10/02 18:01, Hugo Mills wrote:
>>> On Mon, Oct 02, 2017 at 11:39:05AM +0300, Andrei Borzenkov wrote:
>>>> On Mon, Oct 2, 2017 at 11:19 AM, Misono, Tomohiro
>>>> <misono.tomohiro@jp.fujitsu.com> wrote:
>>>>> This patch changes "subvol set-default" to also accept the subvolume path
>>>>> for convenience.
>>>>>
>>>>> This is the one of the issue on github:
>>>>> https://github.com/kdave/btrfs-progs/issues/35
>>>>>
>>>>> If there are two args, they are assumed as subvol id and path to the fs
>>>>> (the same as current behavior), and if there is only one arg, it is assumed
>>>>> as the path to the subvolume. Therefore there is no ambiguity between subvol
>>>>> id and subvol name, which is mentioned in the above issue page.
>>>>>
>>>>> Only the absolute path to the subvolume is allowed, for the safety when
>>>>> multiple filesystems are used.
>>>>>
>>>>> subvol id is resolved by get_subvol_info() which is used by "subvol show".
>>>>>
>>>>> change to v2:
>>>>> restrict the path to only allow absolute path.
>>>>
>>>> This is absolutely arbitrary restriction. Why we can do "btrfs
>>>> subvolume create ./relative/path" but cannot do "btrfs subvolume
>>>> set-default ./relative/path"?
>>>
>>>    Indeed. In fact, it's precisely the _opposite_ of the way that
>>> every other command works -- you provide the path to the subvolume in
>>> the *current namespace*.
>>>
>>>    This approach would be just a major misfeature at this point.
>> Ok, I understood the point and want to revert this.
>> Please review the first version if possible:
>> https://mail-archive.com/linux-btrfs@vger.kernel.org/msg68486.html
> 
> I agree with Andrei and Hugo. We need to check that the subvolume path
> belongs to the filesystem anyway. I don't see that in the first version,
> so please fix it.
> 

I'm a bit confusing, but what is the meaning of "We need to check that the subvolume
path belongs to the filesystem"? Do you mean we should check whether the fs of current
directory and the fs of the subvolume specified are the same or not? However, current 
"set-default <id> <path>" does not check the <path> belong the same fs or not either.
Also, what if the subvolume is specified by absolute path?

Re: [PATCH v2] btrfs-progs: subvol: change subvol set-default to also accept subvol path

[David Sterba]

On Thu, Oct 05, 2017 at 09:57:25AM +0900, Misono, Tomohiro wrote:
> > I agree with Andrei and Hugo. We need to check that the subvolume path
> > belongs to the filesystem anyway. I don't see that in the first version,
> > so please fix it.
> > 
> 
> I'm a bit confusing, but what is the meaning of "We need to check that the subvolume
> path belongs to the filesystem"? Do you mean we should check whether the fs of current
> directory and the fs of the subvolume specified are the same or not? However, current 
> "set-default <id> <path>" does not check the <path> belong the same fs or not either.
> Also, what if the subvolume is specified by absolute path?

Ah my bad, specifying by path is enough of course. So the expected use
is:

filesystem mounted under /mnt

$ cd /mnt
$ btrfs subvol set-default ./subvol1

or 

$ cd /
$ btrfs subvol set-default /mnt/subvol1