[PATCH 1/2] btrfs-progs: Make option parsing more robust to code modifications

[PATCH 1/2] btrfs-progs: Make option parsing more robust to code modifications

[Hugo Mills]

The current approach to option parsing, where long-only options are
selected on the basis of their position in the long_options array is
fragile and painful to modify if options are to be inserted into the
list, rather than appended.

Instead, use the last field of struct option to return a value which
cannot be a char (and hence a short option), and simply switch on those
within the case statement.

Signed-off-by: Hugo Mills <hugo@carfax.org.uk>
---
 cmds-check.c | 49 +++++++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index f06e029..a1226c6 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -8403,13 +8403,15 @@ out:
 	return bad_roots;
 }
 
+enum { OPT_REPAIR = 257, OPT_INIT_CSUM, OPT_INIT_EXTENT, OPT_CHECK_CSUM };
+
 static struct option long_options[] = {
 	{ "super", 1, NULL, 's' },
-	{ "repair", 0, NULL, 0 },
-	{ "init-csum-tree", 0, NULL, 0 },
-	{ "init-extent-tree", 0, NULL, 0 },
-	{ "check-data-csum", 0, NULL, 0 },
-	{ "backup", 0, NULL, 0 },
+	{ "repair", 0, NULL, OPT_REPAIR },
+	{ "init-csum-tree", 0, NULL, OPT_INIT_CSUM },
+	{ "init-extent-tree", 0, NULL, OPT_INIT_EXTENT },
+	{ "check-data-csum", 0, NULL, OPT_CHECK_CSUM },
+	{ "backup", 0, NULL, 'b' },
 	{ "subvol-extents", 1, NULL, 'E' },
 	{ "qgroup-report", 0, NULL, 'Q' },
 	{ "tree-root", 1, NULL, 'r' },
@@ -8483,23 +8485,26 @@ int cmd_check(int argc, char **argv)
 			case '?':
 			case 'h':
 				usage(cmd_check_usage);
-		}
-		if (option_index == 1) {
-			printf("enabling repair mode\n");
-			repair = 1;
-			ctree_flags |= OPEN_CTREE_WRITES;
-		} else if (option_index == 2) {
-			printf("Creating a new CRC tree\n");
-			init_csum_tree = 1;
-			repair = 1;
-			ctree_flags |= OPEN_CTREE_WRITES;
-		} else if (option_index == 3) {
-			init_extent_tree = 1;
-			ctree_flags |= (OPEN_CTREE_WRITES |
-					OPEN_CTREE_NO_BLOCK_GROUPS);
-			repair = 1;
-		} else if (option_index == 4) {
-			check_data_csum = 1;
+			case OPT_REPAIR:
+				printf("enabling repair mode\n");
+				repair = 1;
+				ctree_flags |= OPEN_CTREE_WRITES;
+				break;
+			case OPT_INIT_CSUM:
+				printf("Creating a new CRC tree\n");
+				init_csum_tree = 1;
+				repair = 1;
+				ctree_flags |= OPEN_CTREE_WRITES;
+				break;
+			case OPT_INIT_EXTENT:
+				init_extent_tree = 1;
+				ctree_flags |= (OPEN_CTREE_WRITES |
+						OPEN_CTREE_NO_BLOCK_GROUPS);
+				repair = 1;
+				break;
+			case OPT_CHECK_CSUM:
+				check_data_csum = 1;
+				break;
 		}
 	}
 	argc = argc - optind;
-- 
2.1.4

[PATCH 2/2] btrfs-progs: Add --readonly flag

[Hugo Mills]

Particularly during support conversations, people get confused about
which options to use with btrfs check. Adding a flag, --readonly, which
implies the default read-only behaviour and which conflicts with the
read-write operations, should help make the behaviour of the tool clear.

Signed-off-by: Hugo Mills <hugo@carfax.org.uk>
---
 cmds-check.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index a1226c6..d4d2e73 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -8403,11 +8403,13 @@ out:
 	return bad_roots;
 }
 
-enum { OPT_REPAIR = 257, OPT_INIT_CSUM, OPT_INIT_EXTENT, OPT_CHECK_CSUM };
+enum { OPT_REPAIR = 257, OPT_READONLY, OPT_INIT_CSUM, OPT_INIT_EXTENT,
+	   OPT_CHECK_CSUM };
 
 static struct option long_options[] = {
 	{ "super", 1, NULL, 's' },
 	{ "repair", 0, NULL, OPT_REPAIR },
+	{ "readonly", 0, NULL, OPT_READONLY },
 	{ "init-csum-tree", 0, NULL, OPT_INIT_CSUM },
 	{ "init-extent-tree", 0, NULL, OPT_INIT_EXTENT },
 	{ "check-data-csum", 0, NULL, OPT_CHECK_CSUM },
@@ -8447,6 +8449,7 @@ int cmd_check(int argc, char **argv)
 	u64 num;
 	int option_index = 0;
 	int init_csum_tree = 0;
+	int readonly = 0;
 	int qgroup_report = 0;
 	enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE;
 
@@ -8490,6 +8493,9 @@ int cmd_check(int argc, char **argv)
 				repair = 1;
 				ctree_flags |= OPEN_CTREE_WRITES;
 				break;
+			case OPT_READONLY:
+				readonly = 1;
+				break;
 			case OPT_INIT_CSUM:
 				printf("Creating a new CRC tree\n");
 				init_csum_tree = 1;
@@ -8512,6 +8518,12 @@ int cmd_check(int argc, char **argv)
 	if (check_argc_exact(argc, 1))
 		usage(cmd_check_usage);
 
+	/* This check is the only reason for --readonly to exist */
+	if (readonly && repair) {
+		fprintf(stderr, "Repair options are not compatible with --readonly\n");
+		exit(1);
+	}
+
 	radix_tree_init();
 	cache_tree_init(&root_cache);
 
-- 
2.1.4

Re: [PATCH 1/2] btrfs-progs: Make option parsing more robust to code modifications

[David Sterba]

On Tue, Jan 27, 2015 at 03:05:52PM +0000, Hugo Mills wrote:
> The current approach to option parsing, where long-only options are
> selected on the basis of their position in the long_options array is
> fragile and painful to modify if options are to be inserted into the
> list, rather than appended.
> 
> Instead, use the last field of struct option to return a value which
> cannot be a char (and hence a short option), and simply switch on those
> within the case statement.
> 
> Signed-off-by: Hugo Mills <hugo@carfax.org.uk>

Both applied, thanks.

Re: [PATCH 1/2] btrfs-progs: Make option parsing more robust to code modifications

[Qu Wenruo]

-------- Original Message --------
Subject: [PATCH 1/2] btrfs-progs: Make option parsing more robust to 
code modifications
From: Hugo Mills <hugo@carfax.org.uk>
To: <linux-btrfs@vger.kernel.org>, <dsterba@suse.cz>
Date: 2015年01月27日 23:05
> The current approach to option parsing, where long-only options are
> selected on the basis of their position in the long_options array is
> fragile and painful to modify if options are to be inserted into the
> list, rather than appended.
>
> Instead, use the last field of struct option to return a value which
> cannot be a char (and hence a short option), and simply switch on those
> within the case statement.
This is much much better than original immediate number.
The original way always takes me extra time to count the number.
>
> Signed-off-by: Hugo Mills <hugo@carfax.org.uk>
> ---
>   cmds-check.c | 49 +++++++++++++++++++++++++++----------------------
>   1 file changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/cmds-check.c b/cmds-check.c
> index f06e029..a1226c6 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -8403,13 +8403,15 @@ out:
>   	return bad_roots;
>   }
>   
> +enum { OPT_REPAIR = 257, OPT_INIT_CSUM, OPT_INIT_EXTENT, OPT_CHECK_CSUM };
> +
I'm a little interested in why assigning 257 to OPT_REPAIR.
Does it have any extra meaning?
>   static struct option long_options[] = {
>   	{ "super", 1, NULL, 's' },
> -	{ "repair", 0, NULL, 0 },
> -	{ "init-csum-tree", 0, NULL, 0 },
> -	{ "init-extent-tree", 0, NULL, 0 },
> -	{ "check-data-csum", 0, NULL, 0 },
> -	{ "backup", 0, NULL, 0 },
> +	{ "repair", 0, NULL, OPT_REPAIR },
> +	{ "init-csum-tree", 0, NULL, OPT_INIT_CSUM },
> +	{ "init-extent-tree", 0, NULL, OPT_INIT_EXTENT },
> +	{ "check-data-csum", 0, NULL, OPT_CHECK_CSUM },
> +	{ "backup", 0, NULL, 'b' },
>   	{ "subvol-extents", 1, NULL, 'E' },
>   	{ "qgroup-report", 0, NULL, 'Q' },
>   	{ "tree-root", 1, NULL, 'r' },
> @@ -8483,23 +8485,26 @@ int cmd_check(int argc, char **argv)
>   			case '?':
>   			case 'h':
>   				usage(cmd_check_usage);
> -		}
> -		if (option_index == 1) {
> -			printf("enabling repair mode\n");
> -			repair = 1;
> -			ctree_flags |= OPEN_CTREE_WRITES;
> -		} else if (option_index == 2) {
> -			printf("Creating a new CRC tree\n");
> -			init_csum_tree = 1;
> -			repair = 1;
> -			ctree_flags |= OPEN_CTREE_WRITES;
> -		} else if (option_index == 3) {
> -			init_extent_tree = 1;
> -			ctree_flags |= (OPEN_CTREE_WRITES |
> -					OPEN_CTREE_NO_BLOCK_GROUPS);
> -			repair = 1;
> -		} else if (option_index == 4) {
> -			check_data_csum = 1;
> +			case OPT_REPAIR:
> +				printf("enabling repair mode\n");
> +				repair = 1;
> +				ctree_flags |= OPEN_CTREE_WRITES;
> +				break;
> +			case OPT_INIT_CSUM:
> +				printf("Creating a new CRC tree\n");
> +				init_csum_tree = 1;
> +				repair = 1;
> +				ctree_flags |= OPEN_CTREE_WRITES;
> +				break;
> +			case OPT_INIT_EXTENT:
> +				init_extent_tree = 1;
> +				ctree_flags |= (OPEN_CTREE_WRITES |
> +						OPEN_CTREE_NO_BLOCK_GROUPS);
> +				repair = 1;
> +				break;
> +			case OPT_CHECK_CSUM:
> +				check_data_csum = 1;
> +				break;
>   		}
>   	}
>   	argc = argc - optind;
The rest looks good to me.

Thanks,
Qu

Re: [PATCH 1/2] btrfs-progs: Make option parsing more robust to code modifications

[Hugo Mills]

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

On Wed, Jan 28, 2015 at 09:15:42AM +0800, Qu Wenruo wrote:
> -------- Original Message --------
> Subject: [PATCH 1/2] btrfs-progs: Make option parsing more robust to
> code modifications
> From: Hugo Mills <hugo@carfax.org.uk>
> To: <linux-btrfs@vger.kernel.org>, <dsterba@suse.cz>
> Date: 2015年01月27日 23:05
> >The current approach to option parsing, where long-only options are
> >selected on the basis of their position in the long_options array is
> >fragile and painful to modify if options are to be inserted into the
> >list, rather than appended.
> >
> >Instead, use the last field of struct option to return a value which
> >cannot be a char (and hence a short option), and simply switch on those
> >within the case statement.
> This is much much better than original immediate number.
> The original way always takes me extra time to count the number.
> >
> >Signed-off-by: Hugo Mills <hugo@carfax.org.uk>
> >---
> >  cmds-check.c | 49 +++++++++++++++++++++++++++----------------------
> >  1 file changed, 27 insertions(+), 22 deletions(-)
> >
> >diff --git a/cmds-check.c b/cmds-check.c
> >index f06e029..a1226c6 100644
> >--- a/cmds-check.c
> >+++ b/cmds-check.c
> >@@ -8403,13 +8403,15 @@ out:
> >  	return bad_roots;
> >  }
> >+enum { OPT_REPAIR = 257, OPT_INIT_CSUM, OPT_INIT_EXTENT, OPT_CHECK_CSUM };
> >+
> I'm a little interested in why assigning 257 to OPT_REPAIR.
> Does it have any extra meaning?

   getopt_long returns the character code of the option, for short
options, and returns the struct option->val value for a long
option. If you want to avoid handling options twice (once for the
short, and once for the long), it makes sense to return the short
character code as the "val" for the long options, where one
exists. Where it doesn't exist, we need some other value which isn't
going to cause clashes with the single-character namespace. Hence
starting above the value range of char.

   In hindsight, starting at 256 would have been good, but it's
actually completely irrelevant, as long as the value is strictly
larger than 255.

   Hugo.

> >  static struct option long_options[] = {
> >  	{ "super", 1, NULL, 's' },
> >-	{ "repair", 0, NULL, 0 },
> >-	{ "init-csum-tree", 0, NULL, 0 },
> >-	{ "init-extent-tree", 0, NULL, 0 },
> >-	{ "check-data-csum", 0, NULL, 0 },
> >-	{ "backup", 0, NULL, 0 },
> >+	{ "repair", 0, NULL, OPT_REPAIR },
> >+	{ "init-csum-tree", 0, NULL, OPT_INIT_CSUM },
> >+	{ "init-extent-tree", 0, NULL, OPT_INIT_EXTENT },
> >+	{ "check-data-csum", 0, NULL, OPT_CHECK_CSUM },
> >+	{ "backup", 0, NULL, 'b' },
> >  	{ "subvol-extents", 1, NULL, 'E' },
> >  	{ "qgroup-report", 0, NULL, 'Q' },
> >  	{ "tree-root", 1, NULL, 'r' },
> >@@ -8483,23 +8485,26 @@ int cmd_check(int argc, char **argv)
> >  			case '?':
> >  			case 'h':
> >  				usage(cmd_check_usage);
> >-		}
> >-		if (option_index == 1) {
> >-			printf("enabling repair mode\n");
> >-			repair = 1;
> >-			ctree_flags |= OPEN_CTREE_WRITES;
> >-		} else if (option_index == 2) {
> >-			printf("Creating a new CRC tree\n");
> >-			init_csum_tree = 1;
> >-			repair = 1;
> >-			ctree_flags |= OPEN_CTREE_WRITES;
> >-		} else if (option_index == 3) {
> >-			init_extent_tree = 1;
> >-			ctree_flags |= (OPEN_CTREE_WRITES |
> >-					OPEN_CTREE_NO_BLOCK_GROUPS);
> >-			repair = 1;
> >-		} else if (option_index == 4) {
> >-			check_data_csum = 1;
> >+			case OPT_REPAIR:
> >+				printf("enabling repair mode\n");
> >+				repair = 1;
> >+				ctree_flags |= OPEN_CTREE_WRITES;
> >+				break;
> >+			case OPT_INIT_CSUM:
> >+				printf("Creating a new CRC tree\n");
> >+				init_csum_tree = 1;
> >+				repair = 1;
> >+				ctree_flags |= OPEN_CTREE_WRITES;
> >+				break;
> >+			case OPT_INIT_EXTENT:
> >+				init_extent_tree = 1;
> >+				ctree_flags |= (OPEN_CTREE_WRITES |
> >+						OPEN_CTREE_NO_BLOCK_GROUPS);
> >+				repair = 1;
> >+				break;
> >+			case OPT_CHECK_CSUM:
> >+				check_data_csum = 1;
> >+				break;
> >  		}
> >  	}
> >  	argc = argc - optind;
> The rest looks good to me.
> 
> Thanks,
> Qu

-- 
Hugo Mills             | "You know, the British have always been nice to mad
hugo@... carfax.org.uk | people."
http://carfax.org.uk/  |
PGP: 65E74AC0          |                         Laura Jesson, Brief Encounter

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