Re: [PATCH 2/2] fsck.f2fs: support -p without argument

[Sheng Yong <shengyong1@huawei.com>]

Hi Jaegeuk,

On 1/20/2017 5:47 PM, Jaegeuk Kim wrote:
> On 01/20, Sheng Yong wrote:
[..]
>>>  
>>>  	if (!strcmp("fsck.f2fs", prog)) {
>>> -		const char *option_string = "ad:fp:t";
>>> +		const char *option_string = ":a:d:f:p:t:";
>> I think there is no need to modify options -a/-f/-t, and option_string = ":ad:fp:t"
>> is enough to fix this.
> 
> The reason of these messy things was to detect arguments of -a/-f/-t.
> When I tested this in my ubnutu machine without this, fsck.f2fs would allow
> -a with an argument like -a 1.
> 
[...]
>> So we could keep these handlings of a/f/t/d as the original ones. And check
>> if argc > optind to detect if there are unhandled unknown options left at
>> last.
> 
> That will be handled by default case below?

But the argument of any option cannot be caught by default. I think this maybe
why you did not get error message when you try -a 1.

I also test the following modification:

>From e2647f5815718fad57940ae6a1f9536862d99cb1 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Fri, 20 Jan 2017 17:54:51 +0800
Subject: [PATCH] fsck.f2fs: support -p without argument

This patch allows fsck run -p without argument. So we could use -p as
-p, -p 0, -p 1. '-p' and '-p 0' have the same meaning as '-a'. '-p 1'
check more meta data than '-a'.

Reported-by: KARBOWSKI Piotr <piotr.karbowski@gmail.com>
Signed-off-by: Sheng Yong <shengyong1@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fsck/main.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 80 insertions(+), 19 deletions(-)

diff --git a/fsck/main.c b/fsck/main.c
index 64537cc..e7282e8 100644
--- a/fsck/main.c
+++ b/fsck/main.c
@@ -17,6 +17,7 @@
  */
 #include "fsck.h"
 #include <libgen.h>
+#include <ctype.h>

 struct f2fs_fsck gfsck;

@@ -77,13 +78,54 @@ void sload_usage()
 	exit(1);
 }

+static void __handle_fsck_args(int optopt)
+{
+	switch (optopt) {
+	case 'p':
+		MSG(0, "Info: Use default preen mode\n");
+		c.preen_mode = PREEN_MODE_0;
+		c.auto_fix = 1;
+		break;
+	default:
+		MSG(0, "\tError: Need argument for -%c\n", optopt);
+		fsck_usage();
+	}
+}
+
+static int is_digits(char *optarg)
+{
+	int i;
+
+	for (i = 0; i < strlen(optarg); i++)
+		if (!isdigit(optarg[i]))
+			break;
+	return i == strlen(optarg);
+}
+
 void f2fs_parse_options(int argc, char *argv[])
 {
 	int option = 0;
 	char *prog = basename(argv[0]);
+	int err = 0;
+
+	if (argc < 2) {
+		MSG(0, "\tError: Device not specified\n");
+		if (c.func == FSCK)
+			fsck_usage();
+		else if (c.func == DUMP)
+			dump_usage();
+		else if (c.func == DEFRAG)
+			defrag_usage();
+		else if (c.func == RESIZE)
+			resize_usage();
+		else if (c.func == SLOAD)
+			sload_usage();
+	}
+	c.device_name = strdup(argv[argc - 1]);
+	argv[argc-- - 1] = 0;

 	if (!strcmp("fsck.f2fs", prog)) {
-		const char *option_string = "ad:fp:t";
+		const char *option_string = ":ad:fp:t";

 		c.func = FSCK;
 		while ((option = getopt(argc, argv, option_string)) != EOF) {
@@ -97,6 +139,16 @@ void f2fs_parse_options(int argc, char *argv[])
 				 *  0: default level, the same as -a
 				 *  1: check meta
 				 */
+				if (optarg[0] == '-') {
+					c.preen_mode = PREEN_MODE_0;
+					optind--;
+					break;
+				} else if (!is_digits(optarg)) {
+					MSG(0, "\tError: Wrong option -%c %s\n",
+							option, optarg);
+					err = 1;
+					break;
+				}
 				c.preen_mode = atoi(optarg);
 				if (c.preen_mode < 0)
 					c.preen_mode = PREEN_MODE_0;
@@ -108,9 +160,19 @@ void f2fs_parse_options(int argc, char *argv[])
 					"preen mode %d\n", c.preen_mode);
 				break;
 			case 'd':
+				if (optarg[0] == '-') {
+					MSG(0, "\tError: Need argument for -%c\n",
+							option);
+					err = 1;
+					break;
+				} else if (!is_digits(optarg)) {
+					MSG(0, "\tError: Wrong option -%c %s\n",
+							option, optarg);
+					err = 1;
+					break;
+				}
 				c.dbg_lv = atoi(optarg);
-				MSG(0, "Info: Debug level = %d\n",
-							c.dbg_lv);
+				MSG(0, "Info: Debug level = %d\n", c.dbg_lv);
 				break;
 			case 'f':
 				c.fix_on = 1;
@@ -119,11 +181,25 @@ void f2fs_parse_options(int argc, char *argv[])
 			case 't':
 				c.dbg_lv = -1;
 				break;
+			case ':':
+				__handle_fsck_args(optopt);
+				break;
+			case '?':
+				MSG(0, "\tError: Unknown option %c\n", optopt);
+				err = 1;
+				break;
 			default:
 				MSG(0, "\tError: Unknown option %c\n", option);
-				fsck_usage();
+				err = 1;
 				break;
 			}
+			if (err)
+				fsck_usage();
+		}
+		if (argc > optind) {
+			c.dbg_lv = 0;
+			MSG(0, "\tError: Unknown argument %s\n", argv[optind]);
+			fsck_usage();
 		}
 	} else if (!strcmp("dump.f2fs", prog)) {
 		const char *option_string = "d:i:n:s:a:b:";
@@ -287,21 +363,6 @@ void f2fs_parse_options(int argc, char *argv[])
 			}
 		}
 	}
-
-	if ((optind + 1) != argc) {
-		MSG(0, "\tError: Device not specified\n");
-		if (c.func == FSCK)
-			fsck_usage();
-		else if (c.func == DUMP)
-			dump_usage();
-		else if (c.func == DEFRAG)
-			defrag_usage();
-		else if (c.func == RESIZE)
-			resize_usage();
-		else if (c.func == SLOAD)
-			sload_usage();
-	}
-	c.device_name = argv[optind];
 }

 static void do_fsck(struct f2fs_sb_info *sbi)
-- 
2.10.1

.

> 
>>>  				break;
>>>  			case 'p':
>>>  				/* preen mode has different levels:
>>>  				 *  0: default level, the same as -a
>>>  				 *  1: check meta
>>>  				 */
>>> +				if (optarg[0] == '-') {
>>> +					c.preen_mode = PREEN_MODE_0;
>>> +					optind--;
>>> +					break;
>>> +				} else if (!is_digits(optarg)) {
>>> +					MSG(0, "\tError: Wrong option -%c %s\n",
>>> +							option, optarg);
>>> +					err = 1;
>>> +					break;
>>> +				}
>>>  				c.preen_mode = atoi(optarg);
>>>  				if (c.preen_mode < 0)
>>>  					c.preen_mode = PREEN_MODE_0;
>>>  				else if (c.preen_mode >= PREEN_MODE_MAX)
>>> -					c.preen_mode = PREEN_MODE_MAX - 1;
>>> +					c.preen_mode =
>>> +						PREEN_MODE_MAX - 1;
>>>  				if (c.preen_mode == PREEN_MODE_0)
>>>  					c.auto_fix = 1;
>>> -				MSG(0, "Info: Fix the reported corruption in "
>>> -					"preen mode %d\n", c.preen_mode);
>>> +				MSG(0, "Info: Fix the reported "
>>> +						"corruption in preen mode %d\n",
>>> +						c.preen_mode);
>>>  				break;
>>>  			case 'd':
>>> +				if (optarg[0] == '-') {
>>> +					printf("\tError: Need argument for -%c\n",
>>> +							option);
>> In this case, we cannot use -1 as the argument of -d. And a trivial fix: use MSG
>> instead of printf would be better.
> 
> Yup, it doesn't need to allow -1, and I'll change to use MSG.
> 
>>> +					err = 1;
>>> +					break;
>>> +				} else if (!is_digits(optarg)) {
>>> +					MSG(0, "\tError: Wrong option -%c %s\n",
>>> +							option, optarg);
>>> +					err = 1;
>>> +					break;
>>> +				}
>>>  				c.dbg_lv = atoi(optarg);
>>> -				MSG(0, "Info: Debug level = %d\n",
>>> -							c.dbg_lv);
>>> +				MSG(0, "Info: Debug level = %d\n", c.dbg_lv);
>>>  				break;
>>> -			case 'f':
>>> -				c.fix_on = 1;
>>> -				MSG(0, "Info: Force to fix corruption\n");
>>> +			case ':':
>>> +				__handle_fsck_args(optopt);
>>>  				break;
>>> -			case 't':
>>> -				c.dbg_lv = -1;
>>> +			case '?':
>>> +				MSG(0, "\tError: Unknown option %c\n", optopt);
>>> +				err = 1;
>>>  				break;
>>>  			default:
>>>  				MSG(0, "\tError: Unknown option %c\n", option);
>>> -				fsck_usage();
>>> +				err = 1;
>>>  				break;
>>
>> We could integrate '?' and default together, and remove '%c' int the output
>> message, since it is always '?'.
> 
> There-in optopt and option can show different failed option strings.
> 
>>
>> thanks,
>> Sheng
>>>  			}
>>> +			if (err)
>>> +				fsck_usage();
>>>  		}
>>>  	} else if (!strcmp("dump.f2fs", prog)) {
>>>  		const char *option_string = "d:i:n:s:a:b:";
>>> @@ -287,21 +373,6 @@ void f2fs_parse_options(int argc, char *argv[])
>>>  			}
>>>  		}
>>>  	}
>>> -
>>> -	if ((optind + 1) != argc) {
>>> -		MSG(0, "\tError: Device not specified\n");
>>> -		if (c.func == FSCK)
>>> -			fsck_usage();
>>> -		else if (c.func == DUMP)
>>> -			dump_usage();
>>> -		else if (c.func == DEFRAG)
>>> -			defrag_usage();
>>> -		else if (c.func == RESIZE)
>>> -			resize_usage();
>>> -		else if (c.func == SLOAD)
>>> -			sload_usage();
>>> -	}
>>> -	c.devices[0].path = strdup(argv[optind]);
BTW, some a patch which modifies c.device_name seems missing here.

thanks,
Sheng
>>>  }
>>>  
>>>  static void do_fsck(struct f2fs_sb_info *sbi)
>>>
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot