[2/2] btrfs-progs: refine btrfs-debug-tree error prompt when a mount point given
diff mbox

Message ID 1419470195-2889-2-git-send-email-guihc.fnst@cn.fujitsu.com
State Accepted
Headers show

Commit Message

Gui Hecheng Dec. 25, 2014, 1:16 a.m. UTC
Now, if exec:
	# btrfs-debug-tree <mount_point>
it echos:
	: Superblock bytenr is larger than device size

But it is quite misleading, because it is a valid btrfs.
In this case, we should tell the developer to provide a block device.

After apply:
	: '<mount_point>' is not a block device
	: 'usage: btrfs-debug-tree [options] device

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-debug-tree.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Satoru Takeuchi Dec. 25, 2014, 6:49 a.m. UTC | #1
On 2014/12/25 10:16, Gui Hecheng wrote:
> Now, if exec:
> 	# btrfs-debug-tree <mount_point>
> it echos:
> 	: Superblock bytenr is larger than device size
> 
> But it is quite misleading, because it is a valid btrfs.
> In this case, we should tell the developer to provide a block device.
> 
> After apply:
> 	: '<mount_point>' is not a block device
> 	: 'usage: btrfs-debug-tree [options] device
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>   btrfs-debug-tree.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/btrfs-debug-tree.c b/btrfs-debug-tree.c
> index e46500d..7f079a9 100644
> --- a/btrfs-debug-tree.c
> +++ b/btrfs-debug-tree.c
> @@ -179,6 +179,12 @@ int main(int ac, char **av)
>   	if (check_argc_exact(ac, 1))
>   		print_usage();
>   
> +	ret = check_arg_type(av[optind]);
> +	if (ret != BTRFS_ARG_BLKDEV) {
> +		fprintf(stderr, "'%s' is not a block device\n", av[optind]);

fprintf(stderr, "ERROR: '%s' is ...)" is better since
other error messages in btrfs-progs have this convention.

Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

Thanks,
Satoru

> +		print_usage();
> +	}
> +
>   	info = open_ctree_fs_info(av[optind], 0, 0, OPEN_CTREE_PARTIAL);
>   	if (!info) {
>   		fprintf(stderr, "unable to open %s\n", av[optind]);
> 

--
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
Gui Hecheng Dec. 25, 2014, 7:23 a.m. UTC | #2
On Thu, 2014-12-25 at 15:49 +0900, Satoru Takeuchi wrote:
> On 2014/12/25 10:16, Gui Hecheng wrote:
> > Now, if exec:
> > 	# btrfs-debug-tree <mount_point>
> > it echos:
> > 	: Superblock bytenr is larger than device size
> > 
> > But it is quite misleading, because it is a valid btrfs.
> > In this case, we should tell the developer to provide a block device.
> > 
> > After apply:
> > 	: '<mount_point>' is not a block device
> > 	: 'usage: btrfs-debug-tree [options] device
> > 
> > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> > ---
> >   btrfs-debug-tree.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/btrfs-debug-tree.c b/btrfs-debug-tree.c
> > index e46500d..7f079a9 100644
> > --- a/btrfs-debug-tree.c
> > +++ b/btrfs-debug-tree.c
> > @@ -179,6 +179,12 @@ int main(int ac, char **av)
> >   	if (check_argc_exact(ac, 1))
> >   		print_usage();
> >   
> > +	ret = check_arg_type(av[optind]);
> > +	if (ret != BTRFS_ARG_BLKDEV) {
> > +		fprintf(stderr, "'%s' is not a block device\n", av[optind]);
> 
> fprintf(stderr, "ERROR: '%s' is ...)" is better since
> other error messages in btrfs-progs have this convention.

Yes, it is good to make it clear about the "ERROR", but since there is
no "ERROR" word throughout the source file, I think I would make all
error prompts to start with the "ERROR" word globally in the progs in
another patch.

Thanks,
Gui

> Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> 
> Thanks,
> Satoru
> 
> > +		print_usage();
> > +	}
> > +
> >   	info = open_ctree_fs_info(av[optind], 0, 0, OPEN_CTREE_PARTIAL);
> >   	if (!info) {
> >   		fprintf(stderr, "unable to open %s\n", av[optind]);
> > 
> 


--
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
David Sterba Dec. 29, 2014, 4:29 p.m. UTC | #3
On Thu, Dec 25, 2014 at 09:16:35AM +0800, Gui Hecheng wrote:
> Now, if exec:
> 	# btrfs-debug-tree <mount_point>
> it echos:
> 	: Superblock bytenr is larger than device size
> 
> But it is quite misleading, because it is a valid btrfs.
> In this case, we should tell the developer to provide a block device.
> 
> After apply:
> 	: '<mount_point>' is not a block device
> 	: 'usage: btrfs-debug-tree [options] device
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>  btrfs-debug-tree.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/btrfs-debug-tree.c b/btrfs-debug-tree.c
> index e46500d..7f079a9 100644
> --- a/btrfs-debug-tree.c
> +++ b/btrfs-debug-tree.c
> @@ -179,6 +179,12 @@ int main(int ac, char **av)
>  	if (check_argc_exact(ac, 1))
>  		print_usage();
>  
> +	ret = check_arg_type(av[optind]);
> +	if (ret != BTRFS_ARG_BLKDEV) {
> +		fprintf(stderr, "'%s' is not a block device\n", av[optind]);
> +		print_usage();

The current widespread pattern is to print_usage() after most errors in
commandline arguments but I find it quite annoying. The help is always
available under --help for each command. As the bugfix is good I'm going
to apply it and replace it with exit().

We can start removing misues of print_usage() from the codebase in the
next dev cycle.
--
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

Patch
diff mbox

diff --git a/btrfs-debug-tree.c b/btrfs-debug-tree.c
index e46500d..7f079a9 100644
--- a/btrfs-debug-tree.c
+++ b/btrfs-debug-tree.c
@@ -179,6 +179,12 @@  int main(int ac, char **av)
 	if (check_argc_exact(ac, 1))
 		print_usage();
 
+	ret = check_arg_type(av[optind]);
+	if (ret != BTRFS_ARG_BLKDEV) {
+		fprintf(stderr, "'%s' is not a block device\n", av[optind]);
+		print_usage();
+	}
+
 	info = open_ctree_fs_info(av[optind], 0, 0, OPEN_CTREE_PARTIAL);
 	if (!info) {
 		fprintf(stderr, "unable to open %s\n", av[optind]);