[PATCH 0/4] Btrfs-progs: cleanups: new helper for parsing string to u64

[PATCH 0/4] Btrfs-progs: cleanups: new helper for parsing string to u64

[Wang Shilong]

There are many places that need parse string to u64 for btrfs commands,
in fact, we do such things *too casually*, using atoi/atol/atoll..is not
right at all, and even we don't check whether it is a valid string.

Let's do everything more gracefully, we introduce a new helper
btrfs_strtoull() which will do all the necessary checks.If we fail to
parse string to u64, we will output message and exit directly, this is
something like what usage() is doing. It is ok to not return erro to
it's caller, because this function should be called when parsing arg
(just like usage!)

I convert most places to btrfs_strtoull, test patches with xfstests.
Feel free to review and comment.

Wang Shilong (4):
  Btrfs-progs: new helper to parse string to u64 for btrfs
  Btrfs-progs: switch to btrfs_strtoull() part1
  Btrfs-progs: switch to btrfs_strtoull() part2
  Btrfs-progs: switch to btrfs_strtoull() part3

 btrfs-corrupt-block.c | 38 +++++++++-----------------------------
 btrfs-debug-tree.c    |  2 +-
 btrfs-find-root.c     | 23 +++--------------------
 btrfs-image.c         |  8 ++++----
 btrfs-list.c          | 14 +++-----------
 btrfs-map-logical.c   | 26 ++++++--------------------
 btrfs-show-super.c    |  7 ++++---
 btrfstune.c           |  4 ++--
 cmds-inspect.c        |  8 ++++----
 cmds-replace.c        |  7 +------
 cmds-restore.c        | 20 ++++----------------
 cmds-subvolume.c      |  8 ++------
 utils.c               | 21 ++++++++++++++++++++-
 utils.h               |  1 +
 14 files changed, 64 insertions(+), 123 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Wang Shilong]

There are many places that need parse string to u64 for btrfs commands,
in fact, we do such things *too casually*, using atoi/atol/atoll..is not
right at all, and even we don't check whether it is a valid string.

Let's do everything more gracefully, we introduce a new helper
btrfs_strtoull() which will do all the necessary checks.If we fail to
parse string to u64, we will output message and exit directly, this is
something like what usage() is doing. It is ok to not return erro to
it's caller, because this function should be called when parsing arg
(just like usage!)

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 utils.c | 19 +++++++++++++++++++
 utils.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/utils.c b/utils.c
index 97e23d5..0698d8d 100644
--- a/utils.c
+++ b/utils.c
@@ -1520,6 +1520,25 @@ scan_again:
 	return 0;
 }
 
+u64 btrfs_strtoull(char *str, int base)
+{
+	u64 value;
+	char *ptr_parse_end = NULL;
+	char *ptr_str_end = str + strlen(str);
+
+	value = strtoull(str, &ptr_parse_end, base);
+	if (ptr_parse_end != ptr_str_end) {
+		fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n",
+				str);
+		exit(1);
+	}
+	if (value == ULONG_MAX) {
+		fprintf(stderr, "ERROR: %s is out of range.\n", str);
+		exit(1);
+	}
+	return value;
+}
+
 u64 parse_size(char *s)
 {
 	int i;
diff --git a/utils.h b/utils.h
index 04b8c45..094f41d 100644
--- a/utils.h
+++ b/utils.h
@@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
 int get_mountpt(char *dev, char *mntpt, size_t size);
 int btrfs_scan_block_devices(int run_ioctl);
 u64 parse_size(char *s);
+u64 btrfs_strtoull(char *str, int base);
 int open_file_or_dir(const char *fname, DIR **dirstream);
 void close_file_or_dir(int fd, DIR *dirstream);
 int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
-- 
1.8.3.1

[PATCH 2/4] Btrfs-progs: switch to btrfs_strtoull() part1

[Wang Shilong]

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 btrfs-find-root.c | 23 +++--------------------
 btrfs-list.c      | 14 +++-----------
 cmds-restore.c    | 20 ++++----------------
 utils.c           |  2 +-
 4 files changed, 11 insertions(+), 48 deletions(-)

diff --git a/btrfs-find-root.c b/btrfs-find-root.c
index 0ba4c57..045eab6 100644
--- a/btrfs-find-root.c
+++ b/btrfs-find-root.c
@@ -289,30 +289,13 @@ int main(int argc, char **argv)
 		switch(opt) {
 			errno = 0;
 			case 'o':
-				search_objectid = (u64)strtoll(optarg, NULL,
-							       10);
-				if (errno) {
-					fprintf(stderr, "Error parsing "
-						"objectid\n");
-					exit(1);
-				}
+				search_objectid = btrfs_strtoull(optarg, 10);
 				break;
 			case 'g':
-				search_generation = (u64)strtoll(optarg, NULL,
-							       10);
-				if (errno) {
-					fprintf(stderr, "Error parsing "
-						"generation\n");
-					exit(1);
-				}
+				search_generation = btrfs_strtoull(optarg, 10);
 				break;
 			case 'l':
-				search_level = strtol(optarg, NULL, 10);
-				if (errno) {
-					fprintf(stderr, "Error parsing "
-						"level\n");
-					exit(1);
-				}
+				search_level = btrfs_strtoull(optarg, 10);
 				break;
 			default:
 				usage();
diff --git a/btrfs-list.c b/btrfs-list.c
index 9effb27..1c457ae 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -1854,32 +1854,24 @@ int btrfs_list_parse_filter_string(char *opt_arg,
 {
 
 	u64 arg;
-	char *ptr_parse_end = NULL;
-	char *ptr_opt_arg_end = opt_arg + strlen(opt_arg);
 
 	switch (*(opt_arg++)) {
 	case '+':
-		arg = (u64)strtol(opt_arg, &ptr_parse_end, 10);
+		arg = btrfs_strtoull(opt_arg, 10);
 		type += 2;
-		if (ptr_parse_end != ptr_opt_arg_end)
-			return -1;
 
 		btrfs_list_setup_filter(filters, type, arg);
 		break;
 	case '-':
-		arg = (u64)strtoll(opt_arg, &ptr_parse_end, 10);
+		arg = btrfs_strtoull(opt_arg, 10);
 		type += 1;
-		if (ptr_parse_end != ptr_opt_arg_end)
-			return -1;
 
 		btrfs_list_setup_filter(filters, type, arg);
 		break;
 	default:
 		opt_arg--;
-		arg = (u64)strtoll(opt_arg, &ptr_parse_end, 10);
+		arg = btrfs_strtoull(opt_arg, 10);
 
-		if (ptr_parse_end != ptr_opt_arg_end)
-			return -1;
 		btrfs_list_setup_filter(filters, type, arg);
 		break;
 	}
diff --git a/cmds-restore.c b/cmds-restore.c
index fd533ce..233d538 100644
--- a/cmds-restore.c
+++ b/cmds-restore.c
@@ -1161,23 +1161,15 @@ int cmd_restore(int argc, char **argv)
 				break;
 			case 't':
 				errno = 0;
-				tree_location = (u64)strtoll(optarg, NULL, 10);
-				if (errno != 0) {
-					fprintf(stderr, "Tree location not valid\n");
-					exit(1);
-				}
+				tree_location = btrfs_strtoull(optarg, 10);
 				break;
 			case 'f':
 				errno = 0;
-				fs_location = (u64)strtoll(optarg, NULL, 10);
-				if (errno != 0) {
-					fprintf(stderr, "Fs location not valid\n");
-					exit(1);
-				}
+				fs_location = btrfs_strtoull(optarg, 10);
 				break;
 			case 'u':
 				errno = 0;
-				super_mirror = (int)strtol(optarg, NULL, 10);
+				super_mirror = btrfs_strtoull(optarg, 10);
 				if (errno != 0 ||
 				    super_mirror >= BTRFS_SUPER_MIRROR_MAX) {
 					fprintf(stderr, "Super mirror not "
@@ -1190,11 +1182,7 @@ int cmd_restore(int argc, char **argv)
 				break;
 			case 'r':
 				errno = 0;
-				root_objectid = (u64)strtoll(optarg, NULL, 10);
-				if (errno != 0) {
-					fprintf(stderr, "Root objectid not valid\n");
-					exit(1);
-				}
+				root_objectid = btrfs_strtoull(optarg, 10);
 				break;
 			case 'l':
 				list_roots = 1;
diff --git a/utils.c b/utils.c
index 0698d8d..ea73984 100644
--- a/utils.c
+++ b/utils.c
@@ -1586,7 +1586,7 @@ u64 parse_size(char *s)
 			s[i+1]);
 		exit(51);
 	}
-	return strtoull(s, NULL, 10) * mult;
+	return btrfs_strtoull(s, 10) * mult;
 }
 
 int open_file_or_dir(const char *fname, DIR **dirstream)
-- 
1.8.3.1

[PATCH 3/4] Btrfs-progs: switch to btrfs_strtoull() part2

[Wang Shilong]

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 btrfs-corrupt-block.c | 38 +++++++++-----------------------------
 btrfs-debug-tree.c    |  2 +-
 btrfs-image.c         |  8 ++++----
 btrfs-map-logical.c   | 26 ++++++--------------------
 cmds-inspect.c        |  8 ++++----
 5 files changed, 24 insertions(+), 58 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 10cae00..b54b45f 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -36,7 +36,7 @@
 #define FIELD_BUF_LEN 80
 
 struct extent_buffer *debug_corrupt_block(struct btrfs_root *root, u64 bytenr,
-				     u32 blocksize, int copy)
+				     u32 blocksize, u64 copy)
 {
 	int ret;
 	struct extent_buffer *eb;
@@ -165,7 +165,7 @@ static int corrupt_keys_in_block(struct btrfs_root *root, u64 bytenr)
 }
 
 static int corrupt_extent(struct btrfs_trans_handle *trans,
-			  struct btrfs_root *root, u64 bytenr, int copy)
+			  struct btrfs_root *root, u64 bytenr, u64 copy)
 {
 	struct btrfs_key key;
 	struct extent_buffer *leaf;
@@ -792,7 +792,7 @@ int main(int ac, char **av)
 	u64 logical = (u64)-1;
 	int ret = 0;
 	int option_index = 0;
-	int copy = 0;
+	u64 copy = 0;
 	u64 bytes = 4096;
 	int extent_rec = 0;
 	int extent_tree = 0;
@@ -816,23 +816,13 @@ int main(int ac, char **av)
 			break;
 		switch(c) {
 			case 'l':
-				logical = atoll(optarg);
+				logical = btrfs_strtoull(optarg);
 				break;
 			case 'c':
-				copy = atoi(optarg);
-				if (copy <= 0) {
-					fprintf(stderr,
-						"invalid copy number\n");
-					print_usage();
-				}
+				copy = btrfs_strtoull(optarg, 10);
 				break;
 			case 'b':
-				bytes = atoll(optarg);
-				if (bytes == 0) {
-					fprintf(stderr,
-						"invalid byte count\n");
-					print_usage();
-				}
+				bytes = btrfs_strtoull(optarg, 10);
 				break;
 			case 'e':
 				extent_rec = 1;
@@ -849,28 +839,18 @@ int main(int ac, char **av)
 			case 'U':
 				chunk_tree = 1;
 			case 'i':
-				inode = atoll(optarg);
-				if (inode == 0) {
-					fprintf(stderr,
-						"invalid inode number\n");
-					print_usage();
-				}
+				inode = btrfs_strtoull(optarg, 10);
 				break;
 			case 'f':
 				strncpy(field, optarg, FIELD_BUF_LEN);
 				break;
 			case 'x':
 				errno = 0;
-				file_extent = atoll(optarg);
-				if (errno) {
-					fprintf(stderr, "error converting "
-						"%d\n", errno);
-					print_usage();
-				}
+				file_extent = btrfs_strtoull(optarg, 10);
 				break;
 			case 'm':
 				errno = 0;
-				metadata_block = atoll(optarg);
+				metadata_block = btrfs_strtoull(optarg, 10);
 				if (errno) {
 					fprintf(stderr, "error converting "
 						"%d\n", errno);
diff --git a/btrfs-debug-tree.c b/btrfs-debug-tree.c
index f37de9d..b4cd137 100644
--- a/btrfs-debug-tree.c
+++ b/btrfs-debug-tree.c
@@ -162,7 +162,7 @@ int main(int ac, char **av)
 				root_backups = 1;
 				break;
 			case 'b':
-				block_only = atoll(optarg);
+				block_only = btrfs_strtoull(optarg, 10);
 				break;
 			default:
 				print_usage();
diff --git a/btrfs-image.c b/btrfs-image.c
index 1b2831a..4397738 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2463,8 +2463,8 @@ int main(int argc, char *argv[])
 {
 	char *source;
 	char *target;
-	int num_threads = 0;
-	int compress_level = 0;
+	u64 num_threads = 0;
+	u64 compress_level = 0;
 	int create = 1;
 	int old_restore = 0;
 	int walk_trees = 0;
@@ -2483,12 +2483,12 @@ int main(int argc, char *argv[])
 			create = 0;
 			break;
 		case 't':
-			num_threads = atoi(optarg);
+			num_threads = btrfs_strtoull(optarg, 10);
 			if (num_threads <= 0 || num_threads > 32)
 				print_usage();
 			break;
 		case 'c':
-			compress_level = atoi(optarg);
+			compress_level = btrfs_strtoull(optarg, 10);
 			if (compress_level < 0 || compress_level > 9)
 				print_usage();
 			break;
diff --git a/btrfs-map-logical.c b/btrfs-map-logical.c
index 51179a3..e48c588 100644
--- a/btrfs-map-logical.c
+++ b/btrfs-map-logical.c
@@ -31,6 +31,7 @@
 #include "transaction.h"
 #include "list.h"
 #include "version.h"
+#include "utils.h"
 
 /* we write the mirror info to stdout unless they are dumping the data
  * to stdout
@@ -38,7 +39,7 @@
 static FILE *info_file;
 
 static struct extent_buffer * debug_read_block(struct btrfs_root *root,
-		u64 bytenr, u32 blocksize, int copy)
+		u64 bytenr, u32 blocksize, u64 copy)
 {
 	int ret;
 	struct extent_buffer *eb;
@@ -120,7 +121,7 @@ int main(int ac, char **av)
 	u64 logical = 0;
 	int ret = 0;
 	int option_index = 0;
-	int copy = 0;
+	u64 copy = 0;
 	u64 bytes = 0;
 	int out_fd = 0;
 
@@ -132,28 +133,13 @@ int main(int ac, char **av)
 			break;
 		switch(c) {
 			case 'l':
-				logical = atoll(optarg);
-				if (logical == 0) {
-					fprintf(stderr,
-						"invalid extent number\n");
-					print_usage();
-				}
+				logical = btrfs_strtoull(optarg, 10);
 				break;
 			case 'c':
-				copy = atoi(optarg);
-				if (copy == 0) {
-					fprintf(stderr,
-						"invalid copy number\n");
-					print_usage();
-				}
+				copy = btrfs_strtoull(optarg, 10);
 				break;
 			case 'b':
-				bytes = atoll(optarg);
-				if (bytes == 0) {
-					fprintf(stderr,
-						"invalid byte count\n");
-					print_usage();
-				}
+				bytes = btrfs_strtoull(optarg, 10);
 				break;
 			case 'o':
 				output_file = strdup(optarg);
diff --git a/cmds-inspect.c b/cmds-inspect.c
index f0c8e3d..b2d58e8 100644
--- a/cmds-inspect.c
+++ b/cmds-inspect.c
@@ -120,7 +120,7 @@ static int cmd_inode_resolve(int argc, char **argv)
 		return 1;
 	}
 
-	ret = __ino_to_path_fd(atoll(argv[optind]), fd, verbose,
+	ret = __ino_to_path_fd(btrfs_strtoull(argv[optind], 10), fd, verbose,
 			       argv[optind+1]);
 	close_file_or_dir(fd, dirstream);
 	return !!ret;
@@ -167,7 +167,7 @@ static int cmd_logical_resolve(int argc, char **argv)
 			verbose = 1;
 			break;
 		case 's':
-			size = atoll(optarg);
+			size = btrfs_strtoull(optarg, 10);
 			break;
 		default:
 			usage(cmd_logical_resolve_usage);
@@ -183,7 +183,7 @@ static int cmd_logical_resolve(int argc, char **argv)
 		return 1;
 
 	memset(inodes, 0, sizeof(*inodes));
-	loi.logical = atoll(argv[optind]);
+	loi.logical = btrfs_strtoull(argv[optind], 10);
 	loi.size = size;
 	loi.inodes = (uintptr_t)inodes;
 
@@ -283,7 +283,7 @@ static int cmd_subvolid_resolve(int argc, char **argv)
 		goto out;
 	}
 
-	subvol_id = atoll(argv[1]);
+	subvol_id = btrfs_strtoull(argv[1], 10);
 	ret = btrfs_subvolid_resolve(fd, path, sizeof(path), subvol_id);
 
 	if (ret) {
-- 
1.8.3.1

[PATCH 4/4] Btrfs-progs: switch to btrfs_strtoull() part3

[Wang Shilong]

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 btrfs-show-super.c | 7 ++++---
 btrfstune.c        | 4 ++--
 cmds-replace.c     | 7 +------
 cmds-subvolume.c   | 8 ++------
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/btrfs-show-super.c b/btrfs-show-super.c
index b87f16a..378c7d2 100644
--- a/btrfs-show-super.c
+++ b/btrfs-show-super.c
@@ -59,17 +59,18 @@ int main(int argc, char **argv)
 	int all = 0;
 	char *filename;
 	int fd = -1;
-	int arg, i;
+	int i;
+	u64 arg;
 	u64 sb_bytenr = btrfs_sb_offset(0);
 
 	while ((opt = getopt(argc, argv, "ai:")) != -1) {
 		switch (opt) {
 		case 'i':
-			arg = atoi(optarg);
+			arg = btrfs_strtoull(optarg, 10);
 
 			if (arg < 0 || arg >= BTRFS_SUPER_MIRROR_MAX) {
 				fprintf(stderr,
-					"Illegal super_mirror %d\n",
+					"Illegal super_mirror %llu\n",
 					arg);
 				print_usage();
 				exit(1);
diff --git a/btrfstune.c b/btrfstune.c
index da82f36..744dec5 100644
--- a/btrfstune.c
+++ b/btrfstune.c
@@ -111,7 +111,7 @@ int main(int argc, char *argv[])
 	int success = 0;
 	int extrefs_flag = 0;
 	int seeding_flag = 0;
-	int seeding_value = 0;
+	u64 seeding_value = 0;
 	int skinny_flag = 0;
 	int ret;
 
@@ -123,7 +123,7 @@ int main(int argc, char *argv[])
 		switch(c) {
 		case 'S':
 			seeding_flag = 1;
-			seeding_value = atoi(optarg);
+			seeding_value = btrfs_strtoull(optarg, 10);
 			break;
 		case 'r':
 			extrefs_flag = 1;
diff --git a/cmds-replace.c b/cmds-replace.c
index c683d6c..3dc6c49 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -210,12 +210,7 @@ static int cmd_start_replace(int argc, char **argv)
 		struct btrfs_ioctl_fs_info_args fi_args;
 		struct btrfs_ioctl_dev_info_args *di_args = NULL;
 
-		if (atoi(srcdev) == 0) {
-			fprintf(stderr, "Error: Failed to parse the numerical devid value '%s'\n",
-				srcdev);
-			goto leave_with_error;
-		}
-		start_args.start.srcdevid = (__u64)atoi(srcdev);
+		start_args.start.srcdevid = btrfs_strtoull(srcdev, 10);
 
 		ret = get_fs_info(path, &fi_args, &di_args);
 		if (ret) {
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 0bd76f2..98470f7 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -820,11 +820,7 @@ static int cmd_subvol_set_default(int argc, char **argv)
 	subvolid = argv[1];
 	path = argv[2];
 
-	objectid = (unsigned long long)strtoll(subvolid, NULL, 0);
-	if (errno == ERANGE) {
-		fprintf(stderr, "ERROR: invalid tree id (%s)\n", subvolid);
-		return 1;
-	}
+	objectid = btrfs_strtoull(subvolid, 10);
 
 	fd = open_file_or_dir(path, &dirstream);
 	if (fd < 0) {
@@ -861,7 +857,7 @@ static int cmd_find_new(int argc, char **argv)
 		usage(cmd_find_new_usage);
 
 	subvol = argv[1];
-	last_gen = atoll(argv[2]);
+	last_gen = btrfs_strtoull(argv[2], 10);
 
 	ret = test_issubvolume(subvol);
 	if (ret < 0) {
-- 
1.8.3.1

Re: [PATCH 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Stefan Behrens]

On Wed, 19 Feb 2014 19:17:51 +0800, Wang Shilong wrote:
> There are many places that need parse string to u64 for btrfs commands,
> in fact, we do such things *too casually*, using atoi/atol/atoll..is not
> right at all, and even we don't check whether it is a valid string.
> 
> Let's do everything more gracefully, we introduce a new helper
> btrfs_strtoull() which will do all the necessary checks.If we fail to
> parse string to u64, we will output message and exit directly, this is
> something like what usage() is doing. It is ok to not return erro to
> it's caller, because this function should be called when parsing arg
> (just like usage!)
> 
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
>  utils.c | 19 +++++++++++++++++++
>  utils.h |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/utils.c b/utils.c
> index 97e23d5..0698d8d 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1520,6 +1520,25 @@ scan_again:
>  	return 0;
>  }
>  
> +u64 btrfs_strtoull(char *str, int base)
> +{
> +	u64 value;
> +	char *ptr_parse_end = NULL;
> +	char *ptr_str_end = str + strlen(str);
> +
> +	value = strtoull(str, &ptr_parse_end, base);
> +	if (ptr_parse_end != ptr_str_end) {
> +		fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n",

"not invalid" :)

> +				str);
> +		exit(1);
> +	}
> +	if (value == ULONG_MAX) {

ULLONG_MAX or {errno = 0; value = strtoull(...); if (errno == ERANGE)...}


> +		fprintf(stderr, "ERROR: %s is out of range.\n", str);
> +		exit(1);
> +	}

base = 0 is best BTW since it is able to read hex and octal numbers as
well. I'd remove the base parameter to btrfs_strtoull() and always use 0.

Re: [PATCH 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Wang Shilong]

Hello Stefan,

> On Wed, 19 Feb 2014 19:17:51 +0800, Wang Shilong wrote:
>> There are many places that need parse string to u64 for btrfs commands,
>> in fact, we do such things *too casually*, using atoi/atol/atoll..is not
>> right at all, and even we don't check whether it is a valid string.
>> 
>> Let's do everything more gracefully, we introduce a new helper
>> btrfs_strtoull() which will do all the necessary checks.If we fail to
>> parse string to u64, we will output message and exit directly, this is
>> something like what usage() is doing. It is ok to not return erro to
>> it's caller, because this function should be called when parsing arg
>> (just like usage!)
>> 
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> ---
>> utils.c | 19 +++++++++++++++++++
>> utils.h |  1 +
>> 2 files changed, 20 insertions(+)
>> 
>> diff --git a/utils.c b/utils.c
>> index 97e23d5..0698d8d 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -1520,6 +1520,25 @@ scan_again:
>> 	return 0;
>> }
>> 
>> +u64 btrfs_strtoull(char *str, int base)
>> +{
>> +	u64 value;
>> +	char *ptr_parse_end = NULL;
>> +	char *ptr_str_end = str + strlen(str);
>> +
>> +	value = strtoull(str, &ptr_parse_end, base);
>> +	if (ptr_parse_end != ptr_str_end) {
>> +		fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n",
> 
> "not invalid" :)
> 
oops

>> +				str);
>> +		exit(1);
>> +	}
>> +	if (value == ULONG_MAX) {
> 
> ULLONG_MAX or {errno = 0; value = strtoull(...); if (errno == ERANGE)…}

Yeah, i will use ULLONG_MAX instead ULONG  to check^_^..

> 
> 
>> +		fprintf(stderr, "ERROR: %s is out of range.\n", str);
>> +		exit(1);
>> +	}
> 
> base = 0 is best BTW since it is able to read hex and octal numbers as
> well. I'd remove the base parameter to btrfs_strtoull() and always use 0.

Right, all comments addressed, thanks for your review.^_^

Thanks,
Wang
> 
> --
> 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 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Goffredo Baroncelli]

Hi Wang,

On 02/19/2014 12:17 PM, Wang Shilong wrote:
> There are many places that need parse string to u64 for btrfs commands,
> in fact, we do such things *too casually*, using atoi/atol/atoll..is not
> right at all, and even we don't check whether it is a valid string.
> 
> Let's do everything more gracefully, we introduce a new helper
> btrfs_strtoull() which will do all the necessary checks.If we fail to
> parse string to u64, we will output message and exit directly, this is
> something like what usage() is doing. It is ok to not return erro to
> it's caller, because this function should be called when parsing arg
> (just like usage!)

Please don't do that !

The error reporting of btrfs_strtoull is insufficient.
In case of invalid value the user is not able to 
understand what is the the wrong parameter. This because the error
reporting is handled by the function itself. We should improve the error 
messages, not hide them.


>From my point of view, you have only two choices:
1) change the api as
	u64 btrfs_strtoull(char *str, int *error)
   or 
	int btrfs_strtoull(char *str, u64 *value)

and let the function to return the error code in case of parsing error.
The caller has the responsibility to inform the user of the error.

2) change the api as

	u64 btrfs_strtoull(char *str, char *parameter_name)

so the function in case of error, is able to tell which parameters is wrong.

I prefer the #1.

BR
G.Baroncelli

> 
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
>  utils.c | 19 +++++++++++++++++++
>  utils.h |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/utils.c b/utils.c
> index 97e23d5..0698d8d 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1520,6 +1520,25 @@ scan_again:
>  	return 0;
>  }
>  
> +u64 btrfs_strtoull(char *str, int base)
> +{
> +	u64 value;
> +	char *ptr_parse_end = NULL;
> +	char *ptr_str_end = str + strlen(str);
> +
> +	value = strtoull(str, &ptr_parse_end, base);
> +	if (ptr_parse_end != ptr_str_end) {
> +		fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n",
> +				str);
> +		exit(1);
> +	}
> +	if (value == ULONG_MAX) {
> +		fprintf(stderr, "ERROR: %s is out of range.\n", str);
> +		exit(1);
> +	}
> +	return value;
> +}
> +
>  u64 parse_size(char *s)
>  {
>  	int i;
> diff --git a/utils.h b/utils.h
> index 04b8c45..094f41d 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
>  int get_mountpt(char *dev, char *mntpt, size_t size);
>  int btrfs_scan_block_devices(int run_ioctl);
>  u64 parse_size(char *s);
> +u64 btrfs_strtoull(char *str, int base);
>  int open_file_or_dir(const char *fname, DIR **dirstream);
>  void close_file_or_dir(int fd, DIR *dirstream);
>  int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Wang Shilong]

Hi Goffredo,

> Hi Wang,
> 
> On 02/19/2014 12:17 PM, Wang Shilong wrote:
>> There are many places that need parse string to u64 for btrfs commands,
>> in fact, we do such things *too casually*, using atoi/atol/atoll..is not
>> right at all, and even we don't check whether it is a valid string.
>> 
>> Let's do everything more gracefully, we introduce a new helper
>> btrfs_strtoull() which will do all the necessary checks.If we fail to
>> parse string to u64, we will output message and exit directly, this is
>> something like what usage() is doing. It is ok to not return erro to
>> it's caller, because this function should be called when parsing arg
>> (just like usage!)
> 
> Please don't do that !
> 
> The error reporting of btrfs_strtoull is insufficient.
> In case of invalid value the user is not able to 
> understand what is the the wrong parameter. This because the error
> reporting is handled by the function itself. We should improve the error 
> messages, not hide them.
> 
> 
> From my point of view, you have only two choices:
> 1) change the api as
> 	u64 btrfs_strtoull(char *str, int *error)
>   or 
> 	int btrfs_strtoull(char *str, u64 *value)
> 
> and let the function to return the error code in case of parsing error.
> The caller has the responsibility to inform the user of the error.
> 
> 2) change the api as
> 
> 	u64 btrfs_strtoull(char *str, char *parameter_name)
> 
> so the function in case of error, is able to tell which parameters is wrong.
> 
> I prefer the #1.

I know what you are considering here, i was thinking the way you talked about.
I chose this way for three reasons:

#1 btrfs_strtoul() itself  would output message why we fail before exit.
#2 btrfs_strtoull() is called when arg parsing just like we use the function usage() which will call exit(1).
#3 if we return error message to the caller, just think there are many caller that will deal the same thing, check
     and output error messages….which is a little polluted information….

So i think it is ok that we output message in btrfs_strtoull() itself and return directly.(It is ok because during
arg parsing we don't involve memory allocation etc…)

I understand your suggestions is more common,  but for this case, I am more inclined to the current way
to deal with the issue.^_^

Thanks,
Wang
> 
> BR
> G.Baroncelli
> 
>> 
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> ---
>> utils.c | 19 +++++++++++++++++++
>> utils.h |  1 +
>> 2 files changed, 20 insertions(+)
>> 
>> diff --git a/utils.c b/utils.c
>> index 97e23d5..0698d8d 100644
>> --- a/utils.c
>> +++ b/utils.c
>> @@ -1520,6 +1520,25 @@ scan_again:
>> 	return 0;
>> }
>> 
>> +u64 btrfs_strtoull(char *str, int base)
>> +{
>> +	u64 value;
>> +	char *ptr_parse_end = NULL;
>> +	char *ptr_str_end = str + strlen(str);
>> +
>> +	value = strtoull(str, &ptr_parse_end, base);
>> +	if (ptr_parse_end != ptr_str_end) {
>> +		fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n",
>> +				str);
>> +		exit(1);
>> +	}
>> +	if (value == ULONG_MAX) {
>> +		fprintf(stderr, "ERROR: %s is out of range.\n", str);
>> +		exit(1);
>> +	}
>> +	return value;
>> +}
>> +
>> u64 parse_size(char *s)
>> {
>> 	int i;
>> diff --git a/utils.h b/utils.h
>> index 04b8c45..094f41d 100644
>> --- a/utils.h
>> +++ b/utils.h
>> @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
>> int get_mountpt(char *dev, char *mntpt, size_t size);
>> int btrfs_scan_block_devices(int run_ioctl);
>> u64 parse_size(char *s);
>> +u64 btrfs_strtoull(char *str, int base);
>> int open_file_or_dir(const char *fname, DIR **dirstream);
>> void close_file_or_dir(int fd, DIR *dirstream);
>> int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>> 
> 
> 
> -- 
> gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
> Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
> --
> 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 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Goffredo Baroncelli]

Hi Wang,
On 02/19/2014 05:08 PM, Wang Shilong wrote:
> Hi Goffredo,
> 
>> Hi Wang,
>>
>> On 02/19/2014 12:17 PM, Wang Shilong wrote:
>>> There are many places that need parse string to u64 for btrfs commands,
>>> in fact, we do such things *too casually*, using atoi/atol/atoll..is not
>>> right at all, and even we don't check whether it is a valid string.
>>>
>>> Let's do everything more gracefully, we introduce a new helper
>>> btrfs_strtoull() which will do all the necessary checks.If we fail to
>>> parse string to u64, we will output message and exit directly, this is
>>> something like what usage() is doing. It is ok to not return erro to
>>> it's caller, because this function should be called when parsing arg
>>> (just like usage!)
>>
>> Please don't do that !
>>
>> The error reporting of btrfs_strtoull is insufficient.
>> In case of invalid value the user is not able to 
>> understand what is the the wrong parameter. This because the error
>> reporting is handled by the function itself. We should improve the error 
>> messages, not hide them.
>>
>>
>> From my point of view, you have only two choices:
>> 1) change the api as
>> 	u64 btrfs_strtoull(char *str, int *error)
>>   or 
>> 	int btrfs_strtoull(char *str, u64 *value)
>>
>> and let the function to return the error code in case of parsing error.
>> The caller has the responsibility to inform the user of the error.
>>
>> 2) change the api as
>>
>> 	u64 btrfs_strtoull(char *str, char *parameter_name)
>>
>> so the function in case of error, is able to tell which parameters is wrong.
>>
>> I prefer the #1.
> 
> I know what you are considering here, i was thinking the way you talked about.
> I chose this way for three reasons:
> 
> #1 btrfs_strtoul() itself would output message why we fail before
> exit. 
The error message says that the value is out of range. But doesn't tell which
is the parameter involved.
If you have a command which has several arguments, and the user pass a string
instead of a number, the error returned doesn't tell which argument is wrong.
This is the reason of my complaint. 

At least add a fourth parameter which contains the name of the parameter 
parsed in order to improve the error message.

I.E.

	subvol_id = btrfs_strtoull(argv[i], 10, "subvolume ID");

If the user pass a path instead of a number the error message would be

  ERROR: xxxx is not a valid unsigned long long integer for the parameter 'subvolume ID'.

Or something like that.



> #2 btrfs_strtoull() is called when arg parsing just like we use
> the function usage() which will call exit(1). 
Yes this could be a reasonable tread off, even I would prefer a more
explicit name of the function (like argv_strtoull) in order to highlight
that it is a special function which could exit.

> #3 if we return error
> message to the caller, just think there are many caller that will
> deal the same thing, check and output error messages….which is a
> little polluted information….

> 
> So i think it is ok that we output message in btrfs_strtoull() itself
> and return directly.(It is ok because during arg parsing we don't
> involve memory allocation etc…)
> 
> I understand your suggestions is more common,  but for this case, I
> am more inclined to the current way to deal with the issue.^_^> 
> Thanks,
> Wang
>>
>> BR
>> G.Baroncelli
>>
>>>
>>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>> ---
>>> utils.c | 19 +++++++++++++++++++
>>> utils.h |  1 +
>>> 2 files changed, 20 insertions(+)
>>>
>>> diff --git a/utils.c b/utils.c
>>> index 97e23d5..0698d8d 100644
>>> --- a/utils.c
>>> +++ b/utils.c
>>> @@ -1520,6 +1520,25 @@ scan_again:
>>> 	return 0;
>>> }
>>>
>>> +u64 btrfs_strtoull(char *str, int base)
>>> +{
>>> +	u64 value;
>>> +	char *ptr_parse_end = NULL;
>>> +	char *ptr_str_end = str + strlen(str);
>>> +
>>> +	value = strtoull(str, &ptr_parse_end, base);
>>> +	if (ptr_parse_end != ptr_str_end) {
>>> +		fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n",
>>> +				str);
>>> +		exit(1);
>>> +	}
>>> +	if (value == ULONG_MAX) {
>>> +		fprintf(stderr, "ERROR: %s is out of range.\n", str);
>>> +		exit(1);
>>> +	}
>>> +	return value;
>>> +}
>>> +
>>> u64 parse_size(char *s)
>>> {
>>> 	int i;
>>> diff --git a/utils.h b/utils.h
>>> index 04b8c45..094f41d 100644
>>> --- a/utils.h
>>> +++ b/utils.h
>>> @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
>>> int get_mountpt(char *dev, char *mntpt, size_t size);
>>> int btrfs_scan_block_devices(int run_ioctl);
>>> u64 parse_size(char *s);
>>> +u64 btrfs_strtoull(char *str, int base);
>>> int open_file_or_dir(const char *fname, DIR **dirstream);
>>> void close_file_or_dir(int fd, DIR *dirstream);
>>> int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>>>
>>
>>
>> -- 
>> gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
>> Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
>> --
>> 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
> 
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Wang Shilong]

Hi Goffredo,

> Hi Wang,
> On 02/19/2014 05:08 PM, Wang Shilong wrote:
>> Hi Goffredo,
>> 
>>> Hi Wang,
>>> 
>>> On 02/19/2014 12:17 PM, Wang Shilong wrote:
>>>> There are many places that need parse string to u64 for btrfs commands,
>>>> in fact, we do such things *too casually*, using atoi/atol/atoll..is not
>>>> right at all, and even we don't check whether it is a valid string.
>>>> 
>>>> Let's do everything more gracefully, we introduce a new helper
>>>> btrfs_strtoull() which will do all the necessary checks.If we fail to
>>>> parse string to u64, we will output message and exit directly, this is
>>>> something like what usage() is doing. It is ok to not return erro to
>>>> it's caller, because this function should be called when parsing arg
>>>> (just like usage!)
>>> 
>>> Please don't do that !
>>> 
>>> The error reporting of btrfs_strtoull is insufficient.
>>> In case of invalid value the user is not able to 
>>> understand what is the the wrong parameter. This because the error
>>> reporting is handled by the function itself. We should improve the error 
>>> messages, not hide them.
>>> 
>>> 
>>> From my point of view, you have only two choices:
>>> 1) change the api as
>>> 	u64 btrfs_strtoull(char *str, int *error)
>>>  or 
>>> 	int btrfs_strtoull(char *str, u64 *value)
>>> 
>>> and let the function to return the error code in case of parsing error.
>>> The caller has the responsibility to inform the user of the error.
>>> 
>>> 2) change the api as
>>> 
>>> 	u64 btrfs_strtoull(char *str, char *parameter_name)
>>> 
>>> so the function in case of error, is able to tell which parameters is wrong.
>>> 
>>> I prefer the #1.
>> 
>> I know what you are considering here, i was thinking the way you talked about.
>> I chose this way for three reasons:
>> 
>> #1 btrfs_strtoul() itself would output message why we fail before
>> exit. 
> The error message says that the value is out of range. But doesn't tell which
> is the parameter involved.
> If you have a command which has several arguments, and the user pass a string
> instead of a number, the error returned doesn't tell which argument is wrong.
> This is the reason of my complaint. 
> 
> At least add a fourth parameter which contains the name of the parameter 
> parsed in order to improve the error message.
> 
> I.E.
> 
> 	subvol_id = btrfs_strtoull(argv[i], 10, "subvolume ID");
> 
> If the user pass a path instead of a number the error message would be
> 
>  ERROR: xxxx is not a valid unsigned long long integer for the parameter 'subvolume ID'.

This is more friendly.^_^

> 
> Or something like that.
> 
> 
> 
>> #2 btrfs_strtoull() is called when arg parsing just like we use
>> the function usage() which will call exit(1). 
> Yes this could be a reasonable tread off, even I would prefer a more
> explicit name of the function (like argv_strtoull) in order to highlight
> that it is a special function which could exit.
Fair enough!

So  we reach an agreement, new helper will be something like the  following.

u64 argv_strtoull(const char *arg, const char * argv_name);

Regards,
Wang
> 
>> #3 if we return error
>> message to the caller, just think there are many caller that will
>> deal the same thing, check and output error messages….which is a
>> little polluted information….
> 
>> 
>> So i think it is ok that we output message in btrfs_strtoull() itself
>> and return directly.(It is ok because during arg parsing we don't
>> involve memory allocation etc…)
>> 
>> I understand your suggestions is more common,  but for this case, I
>> am more inclined to the current way to deal with the issue.^_^> 
>> Thanks,
>> Wang
>>> 
>>> BR
>>> G.Baroncelli
>>> 
>>>> 
>>>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>>> ---
>>>> utils.c | 19 +++++++++++++++++++
>>>> utils.h |  1 +
>>>> 2 files changed, 20 insertions(+)
>>>> 
>>>> diff --git a/utils.c b/utils.c
>>>> index 97e23d5..0698d8d 100644
>>>> --- a/utils.c
>>>> +++ b/utils.c
>>>> @@ -1520,6 +1520,25 @@ scan_again:
>>>> 	return 0;
>>>> }
>>>> 
>>>> +u64 btrfs_strtoull(char *str, int base)
>>>> +{
>>>> +	u64 value;
>>>> +	char *ptr_parse_end = NULL;
>>>> +	char *ptr_str_end = str + strlen(str);
>>>> +
>>>> +	value = strtoull(str, &ptr_parse_end, base);
>>>> +	if (ptr_parse_end != ptr_str_end) {
>>>> +		fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n",
>>>> +				str);
>>>> +		exit(1);
>>>> +	}
>>>> +	if (value == ULONG_MAX) {
>>>> +		fprintf(stderr, "ERROR: %s is out of range.\n", str);
>>>> +		exit(1);
>>>> +	}
>>>> +	return value;
>>>> +}
>>>> +
>>>> u64 parse_size(char *s)
>>>> {
>>>> 	int i;
>>>> diff --git a/utils.h b/utils.h
>>>> index 04b8c45..094f41d 100644
>>>> --- a/utils.h
>>>> +++ b/utils.h
>>>> @@ -71,6 +71,7 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_bytes);
>>>> int get_mountpt(char *dev, char *mntpt, size_t size);
>>>> int btrfs_scan_block_devices(int run_ioctl);
>>>> u64 parse_size(char *s);
>>>> +u64 btrfs_strtoull(char *str, int base);
>>>> int open_file_or_dir(const char *fname, DIR **dirstream);
>>>> void close_file_or_dir(int fd, DIR *dirstream);
>>>> int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>>>> 
>>> 
>>> 
>>> -- 
>>> gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
>>> Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
>>> --
>>> 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
>> 
>> 
> 
> 
> -- 
> gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
> Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Eric Sandeen]

On 2/19/14, 10:31 AM, Goffredo Baroncelli wrote:
...

 
> The error message says that the value is out of range. But doesn't tell which
> is the parameter involved.
> If you have a command which has several arguments, and the user pass a string
> instead of a number, the error returned doesn't tell which argument is wrong.
> This is the reason of my complaint. 
> 
> At least add a fourth parameter which contains the name of the parameter 
> parsed in order to improve the error message.
> 
> I.E.
> 
> 	subvol_id = btrfs_strtoull(argv[i], 10, "subvolume ID");
> 
> If the user pass a path instead of a number the error message would be
> 
>   ERROR: xxxx is not a valid unsigned long long integer for the parameter 'subvolume ID'.
> 
> Or something like that.

I'm not so sure that this is needed.  Adding it makes the code a little more complicated,
and requires each caller to send in a string which may get out of sync with the actual
argument name (or the manpage, or the help/usage text).

The user has *just* typed it in, so it won't be too hard to see which one
was wrong even without naming the parameter, i.e. -

# btrfs foo-command 12345 0 123notanumber
Error: 123notanumber is not a valid numeric value

or

# btrfs baz-command 0xFFFFFFFFFFFFFFFF 12345 0
Error: numeric value 0xFFFFFFFFFFFFFFFF is too large.

would probably suffice, without bothering to name the parameter.

I'd also suggest not referring to "unsigned long long" or "out of range" which
might not mean much to people who aren't programmers; "not a valid value"
or "too large" might be clearer.

Also, what if someone enters a negative number?  Right now it happily
accepts it and converts it to an unsigned value:

# ./test-64bit -123
Parsed as 18446744073709551493

but an inadvertent negative sign might lead to highly unexpected results.

Just my $0.02.

-Eric

Re: [PATCH 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[Wang Shilong]

On 02/20/2014 01:23 AM, Eric Sandeen wrote:
> On 2/19/14, 10:31 AM, Goffredo Baroncelli wrote:
> ...
>
>   
>> The error message says that the value is out of range. But doesn't tell which
>> is the parameter involved.
>> If you have a command which has several arguments, and the user pass a string
>> instead of a number, the error returned doesn't tell which argument is wrong.
>> This is the reason of my complaint.
>>
>> At least add a fourth parameter which contains the name of the parameter
>> parsed in order to improve the error message.
>>
>> I.E.
>>
>> 	subvol_id = btrfs_strtoull(argv[i], 10, "subvolume ID");
>>
>> If the user pass a path instead of a number the error message would be
>>
>>    ERROR: xxxx is not a valid unsigned long long integer for the parameter 'subvolume ID'.
>>
>> Or something like that.
> I'm not so sure that this is needed.  Adding it makes the code a little more complicated,
> and requires each caller to send in a string which may get out of sync with the actual
> argument name (or the manpage, or the help/usage text).
>
> The user has *just* typed it in, so it won't be too hard to see which one
> was wrong even without naming the parameter, i.e. -
>
> # btrfs foo-command 12345 0 123notanumber
> Error: 123notanumber is not a valid numeric value
>
> or
>
> # btrfs baz-command 0xFFFFFFFFFFFFFFFF 12345 0
> Error: numeric value 0xFFFFFFFFFFFFFFFF is too large.
>
> would probably suffice, without bothering to name the parameter.
Sounds more reasonable.:-)
>
> I'd also suggest not referring to "unsigned long long" or "out of range" which
> might not mean much to people who aren't programmers; "not a valid value"
> or "too large" might be clearer.
Will update it.
>
> Also, what if someone enters a negative number?  Right now it happily
> accepts it and converts it to an unsigned value:
>
> # ./test-64bit -123
> Parsed as 18446744073709551493
So Let's add a '-' check before calling strtoull. something like:

if (str[0] == '-')
     fprintf(stderr, "ERROR: this may be a negative integer: %s\n", str);

Thanks,
Wang
>
> but an inadvertent negative sign might lead to highly unexpected results.
>
> Just my $0.02.
>
> -Eric
> --
> 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 1/4] Btrfs-progs: new helper to parse string to u64 for btrfs

[David Sterba]

On Wed, Feb 19, 2014 at 07:17:51PM +0800, Wang Shilong wrote:
> +u64 btrfs_strtoull(char *str, int base)
> +{
> +	u64 value;
> +	char *ptr_parse_end = NULL;
> +	char *ptr_str_end = str + strlen(str);
> +
> +	value = strtoull(str, &ptr_parse_end, base);
> +	if (ptr_parse_end != ptr_str_end) {
> +		fprintf(stderr, "ERROR: %s is not an invalid unsigned long long integer.\n",
> +				str);
> +		exit(1);

Calling exit() in a helper function makes is unsuitable as a generic
helper or for use from any library function, though all the places where
it's used are in main() so it's similar to usage(), and makes handling
errors in command line arguments easier.

I'm still concerned about the generic function name, but don't have an
idea how to fix it atm.

> +	}
> +	if (value == ULONG_MAX) {
> +		fprintf(stderr, "ERROR: %s is out of range.\n", str);
> +		exit(1);
> +	}
> +	return value;
> +}