| Message ID | 5008967a-9da9-ad91-65e9-bcfbfb07feb0@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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)
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 -- 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
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. -- 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
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? -- 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
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 -- 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
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)
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(-)