[PATCH 1/3] mkfs.ubifs: check output first

[PATCH 1/3] mkfs.ubifs: check output first

[Shevchenko Andriy (EXT-Teleca/Helsinki)]

Signed-off-by: Andy Shevchenko <ext-andriy.shevchenko@nokia.com>
---
 mkfs.ubifs/mkfs.ubifs.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/mkfs.ubifs/mkfs.ubifs.c b/mkfs.ubifs/mkfs.ubifs.c
index e4b4e3c..9061453 100644
--- a/mkfs.ubifs/mkfs.ubifs.c
+++ b/mkfs.ubifs/mkfs.ubifs.c
@@ -647,8 +647,11 @@ static int get_options(int argc, char**argv)
 
 	if (optind != argc && !output)
 		output = strdup(argv[optind]);
-	if (output)
-		out_ubi = !open_ubi(output);
+
+	if (!output)
+		return err_msg("not output device or file specified");
+
+	out_ubi = !open_ubi(output);
 
 	if (out_ubi) {
 		c->min_io_size = c->di.min_io_size;
@@ -656,9 +659,6 @@ static int get_options(int argc, char**argv)
 		c->max_leb_cnt = c->vi.rsvd_lebs;
 	}
 
-	if (!output)
-		return err_msg("not output device or file specified");
-
 	if (c->min_io_size == -1)
 		return err_msg("min. I/O unit was not specified "
 			       "(use -h for help)");
-- 
1.6.3.3

[PATCH 2/3] flash_eraseall: move constants out of for loop

[Shevchenko Andriy (EXT-Teleca/Helsinki)]

Signed-off-by: Andy Shevchenko <ext-andriy.shevchenko@nokia.com>
---
 flash_eraseall.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/flash_eraseall.c b/flash_eraseall.c
index a22fc49..f6319d1 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -194,21 +194,21 @@ void process_options (int argc, char *argv[])
 {
 	int error = 0;
 
+	static const char *short_options = "jq";
+	static const struct option long_options[] = {
+		{"help", no_argument, 0, 0},
+		{"version", no_argument, 0, 0},
+		{"jffs2", no_argument, 0, 'j'},
+		{"quiet", no_argument, 0, 'q'},
+		{"silent", no_argument, 0, 'q'},
+
+		{0, 0, 0, 0},
+	};
+
 	exe_name = argv[0];
 
 	for (;;) {
 		int option_index = 0;
-		static const char *short_options = "jq";
-		static const struct option long_options[] = {
-			{"help", no_argument, 0, 0},
-			{"version", no_argument, 0, 0},
-			{"jffs2", no_argument, 0, 'j'},
-			{"quiet", no_argument, 0, 'q'},
-			{"silent", no_argument, 0, 'q'},
-
-			{0, 0, 0, 0},
-		};
-
 		int c = getopt_long(argc, argv, short_options,
 				long_options, &option_index);
 		if (c == EOF) {
-- 
1.6.3.3

[PATCH 3/3] flash_eraseall: make -? option the same as --help

[Shevchenko Andriy (EXT-Teleca/Helsinki)]

Signed-off-by: Andy Shevchenko <ext-andriy.shevchenko@nokia.com>
---
 flash_eraseall.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/flash_eraseall.c b/flash_eraseall.c
index f6319d1..ebd329b 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -192,8 +192,6 @@ int main (int argc, char *argv[])
 
 void process_options (int argc, char *argv[])
 {
-	int error = 0;
-
 	static const char *short_options = "jq";
 	static const struct option long_options[] = {
 		{"help", no_argument, 0, 0},
@@ -233,15 +231,13 @@ void process_options (int argc, char *argv[])
 				jffs2 = 1;
 				break;
 			case '?':
-				error = 1;
+				display_help();
 				break;
 		}
 	}
+
 	if (optind == argc) {
 		fprintf(stderr, "%s: no MTD device specified\n", exe_name);
-		error = 1;
-	}
-	if (error) {
 		fprintf(stderr, "Try `%s --help' for more information.\n",
 				exe_name);
 		exit(1);
-- 
1.6.3.3

Re: [PATCH 3/3] flash_eraseall: make -? option the same as --help

[Mike Frysinger]

On Thu, Jun 17, 2010 at 04:42, Shevchenko Andriy wrote:
> --- a/flash_eraseall.c
> +++ b/flash_eraseall.c
> @@ -192,8 +192,6 @@ int main (int argc, char *argv[])
>
>  void process_options (int argc, char *argv[])
>  {
> -       int error = 0;
> -
>        static const char *short_options = "jq";
>        static const struct option long_options[] = {
>                {"help", no_argument, 0, 0},
> @@ -233,15 +231,13 @@ void process_options (int argc, char *argv[])
>                                jffs2 = 1;
>                                break;
>                        case '?':
> -                               error = 1;
> +                               display_help();
>                                break;
>                }
>        }

NAK.  this isnt "-?", that's the error case of getopt.  look at the
invocation of getopt here ... ? is not a valid char.

also, your patch bombing seems screwed up as linux-mtd was bcc-ed.
-mike

Re: [PATCH 2/3] flash_eraseall: move constants out of for loop

[Mike Frysinger]

On Thu, Jun 17, 2010 at 04:42, Shevchenko Andriy wrote:
> --- a/flash_eraseall.c
> +++ b/flash_eraseall.c
> @@ -194,21 +194,21 @@ void process_options (int argc, char *argv[])
>  {
>        int error = 0;
>
> +       static const char *short_options = "jq";
> +       static const struct option long_options[] = {
> +               {"help", no_argument, 0, 0},
> +               {"version", no_argument, 0, 0},
> +               {"jffs2", no_argument, 0, 'j'},
> +               {"quiet", no_argument, 0, 'q'},
> +               {"silent", no_argument, 0, 'q'},
> +
> +               {0, 0, 0, 0},
> +       };
> +
>        exe_name = argv[0];
>
>        for (;;) {
>                int option_index = 0;
> -               static const char *short_options = "jq";
> -               static const struct option long_options[] = {
> -                       {"help", no_argument, 0, 0},
> -                       {"version", no_argument, 0, 0},
> -                       {"jffs2", no_argument, 0, 'j'},
> -                       {"quiet", no_argument, 0, 'q'},
> -                       {"silent", no_argument, 0, 'q'},
> -
> -                       {0, 0, 0, 0},
> -               };
> -
>                int c = getopt_long(argc, argv, short_options,
>                                long_options, &option_index);
>                if (c == EOF) {

NAK: no explanation why you're doing this, and current code has the
variables scoped to where they actually get used.

i wonder though why this code even bothers with "static".
-mike

Re: [PATCH 2/3] flash_eraseall: move constants out of for loop

[Jamie Lokier]

Mike Frysinger wrote:
> NAK: no explanation why you're doing this, and current code has the
> variables scoped to where they actually get used.
> 
> i wonder though why this code even bothers with "static".

The array is static to avoid compiling to code which fills in the
array at runtime.  I.e. it makes the code smaller, to the same size as
if they were globals.  And then, only because its static, the const
can put them in the .rodata section, reducing unshared data size.

Because they're static there's no benefit to moving them to another
scope.

-- Jamie

Re: [PATCH 2/3] flash_eraseall: move constants out of for loop

[Mike Frysinger]

On Tue, Jun 22, 2010 at 20:23, Jamie Lokier wrote:
> Mike Frysinger wrote:
>> i wonder though why this code even bothers with "static".
>
> The array is static to avoid compiling to code which fills in the
> array at runtime.  I.e. it makes the code smaller, to the same size as
> if they were globals.  And then, only because its static, the const
> can put them in the .rodata section, reducing unshared data size.

if it were generated on the stack at runtime, the .text is shared too

> Because they're static there's no benefit to moving them to another
> scope.

that's sort of what i expected, but i found it odd that it isnt:
    static const char * const short_options = "jq";
-mike

Re: [PATCH 2/3] flash_eraseall: move constants out of for loop

[Jamie Lokier]

Mike Frysinger wrote:
> On Tue, Jun 22, 2010 at 20:23, Jamie Lokier wrote:
> > Mike Frysinger wrote:
> >> i wonder though why this code even bothers with "static".
> >
> > The array is static to avoid compiling to code which fills in the
> > array at runtime.  I.e. it makes the code smaller, to the same size as
> > if they were globals.  And then, only because its static, the const
> > can put them in the .rodata section, reducing unshared data size.
> 
> if it were generated on the stack at runtime, the .text is shared too

Shared, but larger than the static-const data it replaces.

> > Because they're static there's no benefit to moving them to another
> > scope.
> 
> that's sort of what i expected, but i found it odd that it isnt:
>     static const char * const short_options = "jq";

You're right, I missed that.
I'd have used this myself:

static const char short_options[] = "jq";

-- Jamie