[PATCH v2 0/5] bugfix, cleanup for mtdinfo

[PATCH v2 0/5] bugfix, cleanup for mtdinfo

[Brian Norris]

This series includes v2 of a bugfix patch, as discussed in this thread:
  http://lists.infradead.org/pipermail/linux-mtd/2011-August/037386.html
and previously. Now, `mtdinfo --all' will not try to print out
region_info, since we need to supply a file path for use with ioctl. In
the future, if sysfs support is supplied for region_info, then we can
renew support in `mtdinfo --all'.

In addition to the bugfix, there are several other cleanups for mtdinfo.

Brian

Brian Norris (5):
  mtdinfo: don't open NULL pointer when getting region_info with `-a'
  mtdinfo: refactor code to remove "args.all" dependency
  mtdinfo: restructure help message
  mtdinfo: fixup "example usage" help section
  mtdinfo: consolidate help as display_help()

 ubi-utils/mtdinfo.c |   61 ++++++++++++++++++++++++++------------------------
 1 files changed, 32 insertions(+), 29 deletions(-)

[PATCH v2 1/5] mtdinfo: don't open NULL pointer when getting region_info with `-a'

[Brian Norris]

This "fixes" a regression found in:
  commit 266061ebd5d72391f0a0e831b018e8fc7fea68a1
  mtdinfo: add regioninfo/eraseblock map display

On certain flash (NOR flash that have eraseblock region info),
`mtdinfo -a' tries to open the MTD node file, for use with the ioctl
MEMGETREGIONINFO; however, we didn't supply a device node path to
`mtdinfo -a', so it's using NULL, resulting in errors like:

  mtdinfo: error!: couldn't open MTD dev: (null)
           error 14 (Bad address)

For now, we can just skip dumping region_info with the `-a' flag. If we
find a better way to do this (e.g., export via sysfs, find device nodes
via automatic routines, etc.), then we can kill the workaround and this
FIXME should be removed.

The regression was first reported at:

  http://lists.infradead.org/pipermail/linux-mtd/2011-July/037232.html

The result of recent changes is that we cannot get region_info for devices
via the `--all' option. We add a note in the help message warning that
mtdinfo may find more info when given a device patch, e.g., /dev/mtdX.

Reported-by: Brian Foster <brian.foster@maxim-ic.com>
CC: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 ubi-utils/mtdinfo.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index e72d69e..b4a2f4c 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -58,6 +58,8 @@ static const char optionsstr[] =
 "                                on this MTD device\n"
 "-M, --map                       print eraseblock map\n"
 "-a, --all                       print information about all MTD devices\n"
+"                                Note: `--all' may give less info per device\n"
+"                                than, e.g., `mtdinfo /dev/mtdX'\n"
 "-h, --help                      print help message\n"
 "-V, --version                   print program version";
 
@@ -239,8 +241,14 @@ static void print_region_info(const struct mtd_dev_info *mtd)
 	region_info_t reginfo;
 	int r, fd;
 
-	/* If we don't have any region info, just return */
-	if (!args.map && mtd->region_cnt == 0)
+	/*
+	 * If we don't have any region info, just return
+	 *
+	 * FIXME: We can't get region_info (via ioctl) without having the MTD
+	 *        node path. This is a problem for `mtdinfo -a', for example,
+	 *        since it doesn't provide any filepath information.
+	 */
+	if (!args.node || (!args.map && mtd->region_cnt == 0))
 		return;
 
 	/* First open the device so we can query it */
-- 
1.7.0.4

Re: [PATCH v2 1/5] mtdinfo: don't open NULL pointer when getting region_info with `-a'

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

[PATCH v2 2/5] mtdinfo: refactor code to remove "args.all" dependency

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 ubi-utils/mtdinfo.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index b4a2f4c..0a20c76 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -338,8 +338,7 @@ static int print_dev_info(libmtd_t libmtd, const struct mtd_info *mtd_info, int
 	return 0;
 }
 
-static int print_general_info(libmtd_t libmtd, const struct mtd_info *mtd_info,
-			      int all)
+static int print_general_info(libmtd_t libmtd, const struct mtd_info *mtd_info)
 {
 	int i, err, first = 1;
 	struct mtd_dev_info mtd;
@@ -366,15 +365,9 @@ static int print_general_info(libmtd_t libmtd, const struct mtd_info *mtd_info,
 		}
 	}
 	printf("\n");
-	printf("Sysfs interface supported:      %s\n",
+	printf("Sysfs interface supported:      %s\n\n",
 	       mtd_info->sysfs_supported ? "yes" : "no");
 
-	if (!all)
-		return 0;
-
-	first = 1;
-	printf("\n");
-
 	for (i = mtd_info->lowest_mtd_num;
 	     i <= mtd_info->highest_mtd_num; i++) {
 		err = print_dev_info(libmtd, mtd_info, i);
@@ -421,7 +414,7 @@ int main(int argc, char * const argv[])
 			goto out_libmtd;
 		err = print_dev_info(libmtd, &mtd_info, mtdn);
 	} else
-		err = print_general_info(libmtd, &mtd_info, args.all);
+		err = print_general_info(libmtd, &mtd_info);
 	if (err)
 		goto out_libmtd;
 
-- 
1.7.0.4

Re: [PATCH v2 2/5] mtdinfo: refactor code to remove "args.all" dependency

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

[PATCH v2 3/5] mtdinfo: restructure help message

[Brian Norris]

We weren't very consistent in how we listed our options in the mtdinfo
help string (listing short options, long options, or both). Plus, not all
options are inter-operable, so we should distinguish this somewhat.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 ubi-utils/mtdinfo.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index 0a20c76..cd55dce 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -64,8 +64,10 @@ static const char optionsstr[] =
 "-V, --version                   print program version";
 
 static const char usage[] =
-"Usage: " PROGRAM_NAME " <MTD device node file name> [-u] [-M] [-h] [-V] [--ubi-info] [--help]\n"
-"\t\t[--version]\n"
+"Usage: " PROGRAM_NAME " <MTD node file path> [--map | -M] [--ubi-info | -u]\n"
+"       " PROGRAM_NAME " --all [--ubi-info | -u]\n"
+"       " PROGRAM_NAME " [--help | --version]\n"
+"\n"
 "Example 1: " PROGRAM_NAME " - (no arguments) print general MTD information\n"
 "Example 2: " PROGRAM_NAME " /dev/mtd0 - print information MTD device /dev/mtd0\n"
 "Example 3: " PROGRAM_NAME " /dev/mtd0 -u - print information MTD device /dev/mtd0\n"
-- 
1.7.0.4

Re: [PATCH v2 3/5] mtdinfo: restructure help message

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

[PATCH v2 4/5] mtdinfo: fixup "example usage" help section

[Brian Norris]

Line up columns better so that everything is more readable.

Remove "Example 1" since `mtdinfo' does not print information when not
given any arguments.

Remove "...UBI layout information" from description of Example 4, since
Example 4 (now 3) doesn't include the `-u' flag.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 ubi-utils/mtdinfo.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index cd55dce..1c3a46f 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -68,12 +68,10 @@ static const char usage[] =
 "       " PROGRAM_NAME " --all [--ubi-info | -u]\n"
 "       " PROGRAM_NAME " [--help | --version]\n"
 "\n"
-"Example 1: " PROGRAM_NAME " - (no arguments) print general MTD information\n"
-"Example 2: " PROGRAM_NAME " /dev/mtd0 - print information MTD device /dev/mtd0\n"
-"Example 3: " PROGRAM_NAME " /dev/mtd0 -u - print information MTD device /dev/mtd0\n"
-"\t\t\t\tand include UBI layout information\n"
-"Example 4: " PROGRAM_NAME " -a - print information about all MTD devices\n"
-"\t\t\tand include UBI layout information\n";
+"Example 1: " PROGRAM_NAME " /dev/mtd0       print information MTD device /dev/mtd0\n"
+"Example 2: " PROGRAM_NAME " /dev/mtd0 -u    print information MTD device /dev/mtd0\n"
+"\t\t\t\t   and include UBI layout information\n"
+"Example 3: " PROGRAM_NAME " -a              print information about all MTD devices\n";
 
 static const struct option long_options[] = {
 	{ .name = "ubi-info",  .has_arg = 0, .flag = NULL, .val = 'u' },
-- 
1.7.0.4

Re: [PATCH v2 4/5] mtdinfo: fixup "example usage" help section

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

[PATCH v2 5/5] mtdinfo: consolidate help as display_help()

[Brian Norris]

The help message for mtdinfo is unnecessarily disjointed. It is split
into three strings which reuse the PROGRAM_NAME string inefficiently and
don't have a consistent style.

This fixup should provide a cleaner look with aligned columns and
easier-to-read source code.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 ubi-utils/mtdinfo.c |   36 +++++++++++++++++++-----------------
 1 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index 1c3a46f..9f25fd8 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -50,10 +50,16 @@ static struct args args = {
 	.node = NULL,
 };
 
-static const char doc[] = PROGRAM_NAME " version " VERSION
-			 " - a tool to print MTD information.";
-
-static const char optionsstr[] =
+static void display_help(void)
+{
+	printf(
+"%1$s version %2$s - a tool to print MTD information.\n"
+"\n"
+"Usage: %1$s <MTD node file path> [--map | -M] [--ubi-info | -u]\n"
+"       %1$s --all [--ubi-info | -u]\n"
+"       %1$s [--help | --version]\n"
+"\n"
+"Options:\n"
 "-u, --ubi-info                  print what would UBI layout be if it was put\n"
 "                                on this MTD device\n"
 "-M, --map                       print eraseblock map\n"
@@ -61,17 +67,15 @@ static const char optionsstr[] =
 "                                Note: `--all' may give less info per device\n"
 "                                than, e.g., `mtdinfo /dev/mtdX'\n"
 "-h, --help                      print help message\n"
-"-V, --version                   print program version";
-
-static const char usage[] =
-"Usage: " PROGRAM_NAME " <MTD node file path> [--map | -M] [--ubi-info | -u]\n"
-"       " PROGRAM_NAME " --all [--ubi-info | -u]\n"
-"       " PROGRAM_NAME " [--help | --version]\n"
+"-V, --version                   print program version\n"
 "\n"
-"Example 1: " PROGRAM_NAME " /dev/mtd0       print information MTD device /dev/mtd0\n"
-"Example 2: " PROGRAM_NAME " /dev/mtd0 -u    print information MTD device /dev/mtd0\n"
-"\t\t\t\t   and include UBI layout information\n"
-"Example 3: " PROGRAM_NAME " -a              print information about all MTD devices\n";
+"Examples:\n"
+"  %1$s /dev/mtd0             print information MTD device /dev/mtd0\n"
+"  %1$s /dev/mtd0 -u          print information MTD device /dev/mtd0\n"
+"\t\t\t\tand include UBI layout information\n"
+"  %1$s -a                    print information about all MTD devices\n",
+	PROGRAM_NAME, VERSION);
+}
 
 static const struct option long_options[] = {
 	{ .name = "ubi-info",  .has_arg = 0, .flag = NULL, .val = 'u' },
@@ -105,9 +109,7 @@ static int parse_opt(int argc, char * const argv[])
 			break;
 
 		case 'h':
-			printf("%s\n\n", doc);
-			printf("%s\n\n", usage);
-			printf("%s\n", optionsstr);
+			display_help();
 			exit(EXIT_SUCCESS);
 
 		case 'V':
-- 
1.7.0.4

Re: [PATCH v2 5/5] mtdinfo: consolidate help as display_help()

[Mike Frysinger]

On Tue, Aug 9, 2011 at 17:36, Brian Norris wrote:
> +static void display_help(void)
> +{
> +       printf(
> +"%1$s version %2$s - a tool to print MTD information.\n"

please properly indent
<tab>printf(
<tab><tab>"..........."
-mike

[PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Brian Norris]

The help message for mtdinfo is unnecessarily disjointed. It is split
into three strings which reuse the PROGRAM_NAME string inefficiently and
don't have a consistent style.

This fixup should provide a cleaner look with aligned columns and
easier-to-read source code.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v3: indented help message properly

 ubi-utils/mtdinfo.c |   52 ++++++++++++++++++++++++++------------------------
 1 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index 1c3a46f..2d47bd7 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -50,28 +50,32 @@ static struct args args = {
 	.node = NULL,
 };
 
-static const char doc[] = PROGRAM_NAME " version " VERSION
-			 " - a tool to print MTD information.";
-
-static const char optionsstr[] =
-"-u, --ubi-info                  print what would UBI layout be if it was put\n"
-"                                on this MTD device\n"
-"-M, --map                       print eraseblock map\n"
-"-a, --all                       print information about all MTD devices\n"
-"                                Note: `--all' may give less info per device\n"
-"                                than, e.g., `mtdinfo /dev/mtdX'\n"
-"-h, --help                      print help message\n"
-"-V, --version                   print program version";
-
-static const char usage[] =
-"Usage: " PROGRAM_NAME " <MTD node file path> [--map | -M] [--ubi-info | -u]\n"
-"       " PROGRAM_NAME " --all [--ubi-info | -u]\n"
-"       " PROGRAM_NAME " [--help | --version]\n"
-"\n"
-"Example 1: " PROGRAM_NAME " /dev/mtd0       print information MTD device /dev/mtd0\n"
-"Example 2: " PROGRAM_NAME " /dev/mtd0 -u    print information MTD device /dev/mtd0\n"
-"\t\t\t\t   and include UBI layout information\n"
-"Example 3: " PROGRAM_NAME " -a              print information about all MTD devices\n";
+static void display_help(void)
+{
+	printf(
+		"%1$s version %2$s - a tool to print MTD information.\n"
+		"\n"
+		"Usage: %1$s <MTD node file path> [--map | -M] [--ubi-info | -u]\n"
+		"       %1$s --all [--ubi-info | -u]\n"
+		"       %1$s [--help | --version]\n"
+		"\n"
+		"Options:\n"
+		"-u, --ubi-info                  print what would UBI layout be if it was put\n"
+		"                                on this MTD device\n"
+		"-M, --map                       print eraseblock map\n"
+		"-a, --all                       print information about all MTD devices\n"
+		"                                Note: `--all' may give less info per device\n"
+		"                                than, e.g., `mtdinfo /dev/mtdX'\n"
+		"-h, --help                      print help message\n"
+		"-V, --version                   print program version\n"
+		"\n"
+		"Examples:\n"
+		"  %1$s /dev/mtd0             print information MTD device /dev/mtd0\n"
+		"  %1$s /dev/mtd0 -u          print information MTD device /dev/mtd0\n"
+		"\t\t\t\tand include UBI layout information\n"
+		"  %1$s -a                    print information about all MTD devices\n",
+		PROGRAM_NAME, VERSION);
+}
 
 static const struct option long_options[] = {
 	{ .name = "ubi-info",  .has_arg = 0, .flag = NULL, .val = 'u' },
@@ -105,9 +109,7 @@ static int parse_opt(int argc, char * const argv[])
 			break;
 
 		case 'h':
-			printf("%s\n\n", doc);
-			printf("%s\n\n", usage);
-			printf("%s\n", optionsstr);
+			display_help();
 			exit(EXIT_SUCCESS);
 
 		case 'V':
-- 
1.7.0.4

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Brian Foster]

On Thursday 11 August 2011 18:19:40 Brian Norris wrote:
> The help message for mtdinfo is unnecessarily disjointed. It is split
> into three strings which reuse the PROGRAM_NAME string inefficiently and
> don't have a consistent style.
> 
> This fixup should provide a cleaner look with aligned columns and
> easier-to-read source code.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> v3: indented help message properly
> 
>  ubi-utils/mtdinfo.c |   52 ++++++++++++++++++++++++++------------------------
>  1 files changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
>[ ... ]
> +static void display_help(void)
> +{
> +	printf(
> +		"%1$s version %2$s - a tool to print MTD information.\n"
> +		"\n"
> +		"Usage: %1$s <MTD node file path> [--map | -M] [--ubi-info | -u]\n"
> +		"       %1$s --all [--ubi-info | -u]\n"
> +		"       %1$s [--help | --version]\n"
> +		"\n"
> +		"Options:\n"
> +		"-u, --ubi-info                  print what would UBI layout be if it was put\n"
> +		"                                on this MTD device\n"
> +		"-M, --map                       print eraseblock map\n"
> +		"-a, --all                       print information about all MTD devices\n"
> +		"                                Note: `--all' may give less info per device\n"
> +		"                                than, e.g., `mtdinfo /dev/mtdX'\n"
> +		"-h, --help                      print help message\n"
> +		"-V, --version                   print program version\n"
> +		"\n"
> +		"Examples:\n"
> +		"  %1$s /dev/mtd0             print information MTD device /dev/mtd0\n"
> +		"  %1$s /dev/mtd0 -u          print information MTD device /dev/mtd0\n"
> +		"\t\t\t\tand include UBI layout information\n"

 Any reason this line, and only this line,
 uses tabs (\t) for formatting whilst every
 other line uses spaces?  (FWIW, I prefer
 spaces, but more importantly think there
 should be some consistency.)

 Yes, this is pedantic trivia!  ;-)
cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Brian Norris]

On Fri, Aug 12, 2011 at 12:06 AM, Brian Foster
<brian.foster@maxim-ic.com> wrote:
>> +             "Examples:\n"
>> +             "  %1$s /dev/mtd0             print information MTD device /dev/mtd0\n"
>> +             "  %1$s /dev/mtd0 -u          print information MTD device /dev/mtd0\n"
>> +             "\t\t\t\tand include UBI layout information\n"
>
>  Any reason this line, and only this line,
>  uses tabs (\t) for formatting whilst every
>  other line uses spaces?  (FWIW, I prefer
>  spaces, but more importantly think there
>  should be some consistency.)

Yes, sort of. It's used to be because we "don't know" the length of
the "%1$s" strings (i.e., the PROGRAM_NAME macro, which here is just
"mtdinfo"). I suppose we could either hardcode the spaces in or try to
do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel)
to find the length of PROGRAM_NAME. I'll see how the second option
works (for a potential v4 patch?) unless someone objects.

>  Yes, this is pedantic trivia!  ;-)

Still a valid question...in fact, this was something I had meant to
reconsider before sending this patch, but I forgot :)

Brian

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Mike Frysinger]

On Fri, Aug 12, 2011 at 12:37, Brian Norris wrote:
> On Fri, Aug 12, 2011 at 12:06 AM, Brian Foster wrote:
>>> +             "Examples:\n"
>>> +             "  %1$s /dev/mtd0             print information MTD device /dev/mtd0\n"
>>> +             "  %1$s /dev/mtd0 -u          print information MTD device /dev/mtd0\n"
>>> +             "\t\t\t\tand include UBI layout information\n"
>>
>>  Any reason this line, and only this line,
>>  uses tabs (\t) for formatting whilst every
>>  other line uses spaces?  (FWIW, I prefer
>>  spaces, but more importantly think there
>>  should be some consistency.)
>
> Yes, sort of. It's used to be because we "don't know" the length of
> the "%1$s" strings (i.e., the PROGRAM_NAME macro, which here is just
> "mtdinfo"). I suppose we could either hardcode the spaces in or try to
> do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel)
> to find the length of PROGRAM_NAME. I'll see how the second option
> works (for a potential v4 patch?) unless someone objects.

you could probably use %*s and then pass in "" as well as <some max
spacing number - strlen(PROGRAM_NAME)>.

printf("%*s\n", 80 - strlen(PROGRAM_NAME), "");
-mike

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Brian Norris]

On Sat, Aug 13, 2011 at 11:10 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Fri, Aug 12, 2011 at 12:37, Brian Norris wrote:
...
>> "mtdinfo"). I suppose we could either hardcode the spaces in or try to
>> do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel)
>
> you could probably use %*s and then pass in "" as well as <some max
> spacing number - strlen(PROGRAM_NAME)>.
>
> printf("%*s\n", 80 - strlen(PROGRAM_NAME), "");

Yes, that's what I meant...of course "strlen" is better than writing
your own ARRAY_SIZE() when you already have the full C library :)

But I'm not sure about the subtraction part. Perhaps just use the
length as extra indentation...I'll send a patch to make it clear!

Brian

"Someday, you might find yourself accidentally using printk() instead
of printf() in your user-space code. When that day comes, you can say
you are a true kernel hacker."
        --Robert Love, Linux Kernel Development

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Mike Frysinger]

On Mon, Aug 15, 2011 at 13:27, Brian Norris wrote:
> On Sat, Aug 13, 2011 at 11:10 AM, Mike Frysinger wrote:
>> On Fri, Aug 12, 2011 at 12:37, Brian Norris wrote:
> ...
>>> "mtdinfo"). I suppose we could either hardcode the spaces in or try to
>>> do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel)
>>
>> you could probably use %*s and then pass in "" as well as <some max
>> spacing number - strlen(PROGRAM_NAME)>.
>>
>> printf("%*s\n", 80 - strlen(PROGRAM_NAME), "");
>
> Yes, that's what I meant...of course "strlen" is better than writing
> your own ARRAY_SIZE() when you already have the full C library :)

gcc optimizes strlen("constant") into a number
-mike

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Brian Foster]

On Tuesday 16 August 2011 06:00:57 Mike Frysinger wrote:
> On Mon, Aug 15, 2011 at 13:27, Brian Norris wrote:
> > On Sat, Aug 13, 2011 at 11:10 AM, Mike Frysinger wrote:
> >> On Fri, Aug 12, 2011 at 12:37, Brian Norris wrote:
> > ...
> >>> "mtdinfo"). I suppose we could either hardcode the spaces in or try to
> >>> do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel)
> >>
> >> you could probably use %*s and then pass in "" as well as <some max
> >> spacing number - strlen(PROGRAM_NAME)>.
> >>
> >> printf("%*s\n", 80 - strlen(PROGRAM_NAME), "");
> >
> > Yes, that's what I meant...of course "strlen" is better than writing
> > your own ARRAY_SIZE() when you already have the full C library :)
> 
> gcc optimizes strlen("constant") into a number

 And ‘ARRAY_SIZE(...)’, in this case, is simply ‘sizeof(...)’
 (or, actually, ‘sizeof(...)-1’ to be equivalent to ‘strlen’).

 Albeit I won't worry about the case of  strlen > 80  here,
 I am mildly curious what the output would be if that were
 indeed true...?

 The other thing I won't worry about is the assumption every
 output byte in PROGRAM_NAME is a printing glyph occupying
 exactly one cell.  That is true (in this case) ....

cheers!
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Brian Norris]

On Tue, Aug 16, 2011 at 12:18 AM, Brian Foster
<brian.foster@maxim-ic.com> wrote:
> On Tuesday 16 August 2011 06:00:57 Mike Frysinger wrote:
>> On Mon, Aug 15, 2011 at 13:27, Brian Norris wrote:
>> > Yes, that's what I meant...of course "strlen" is better than writing
>> > your own ARRAY_SIZE() when you already have the full C library :)
>>
>> gcc optimizes strlen("constant") into a number

I was referring to the fact that the kernel does not provide all
standard C library functions, whereas user-space does. Even so, I
overlooked the fact that the kernel has a "strlen" function
(linux/kernel.h)

>  And ‘ARRAY_SIZE(...)’, in this case, is simply ‘sizeof(...)’
>  (or, actually, ‘sizeof(...)-1’ to be equivalent to ‘strlen’).
>
>  Albeit I won't worry about the case of  strlen > 80  here,
>  I am mildly curious what the output would be if that were
>  indeed true...?

Mike's example is a little different than my patch that was just
included in mtd-utils. I don't think my version would have a problem
with strlen > 80.

>  The other thing I won't worry about is the assumption every
>  output byte in PROGRAM_NAME is a printing glyph occupying
>  exactly one cell.  That is true (in this case) ....

Not sure how often anyone needs to worry about this one...really, this
particular discussion is mostly an exercise in frivolity IMO, as I
don't see us changing the PROGRAM_NAME for mtdinfo any time soon, and
even if we did, it's not that hard to edit some whitespace.

Brian

Re: [PATCH v3 5/5] mtdinfo: consolidate help as display_help()

[Brian Foster]

On Tuesday 16 August 2011 19:12:38 Brian Norris wrote:
> On Tue, Aug 16, 2011 at 12:18 AM, Brian Foster <brian.foster@maxim-ic.com> wrote:
>[ ... ]
> >  The other thing I won't worry about is the assumption every
> >  output byte in PROGRAM_NAME is a printing glyph occupying
> >  exactly one cell.  That is true (in this case) ....
> 
> Not sure how often anyone needs to worry about this one...

 In a previous job, this issue(in several variations) was real and live.
 So it's always in the back of my mind....   ;-)

> this particular discussion is mostly an exercise in frivolity

 Agreed!

 Many thanks for your kind and patient help.
cheers,
	-blf-
-- 
Brian FOSTER
Principal MTS, Software
Maxim Integrated Products (Microcontroller BU), formerly Innova Card
Web    : http://www.maxim-ic.com/

[PATCH v4 5/5] mtdinfo: consolidate help as display_help()

[Brian Norris]

The help message for mtdinfo is unnecessarily disjointed. It is split
into three strings which reuse the PROGRAM_NAME string inefficiently and
don't have a consistent style.

This fixup should provide a cleaner look with aligned columns and
easier-to-read source code.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v4: use spaces instead of tabs for indentation, using "strlen" plus printf
    positional parameters

v3: indented help message 'properly'

v2: split off from previous patch

 ubi-utils/mtdinfo.c |   52 ++++++++++++++++++++++++++------------------------
 1 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index 1c3a46f..d919673 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -50,28 +50,32 @@ static struct args args = {
 	.node = NULL,
 };
 
-static const char doc[] = PROGRAM_NAME " version " VERSION
-			 " - a tool to print MTD information.";
-
-static const char optionsstr[] =
-"-u, --ubi-info                  print what would UBI layout be if it was put\n"
-"                                on this MTD device\n"
-"-M, --map                       print eraseblock map\n"
-"-a, --all                       print information about all MTD devices\n"
-"                                Note: `--all' may give less info per device\n"
-"                                than, e.g., `mtdinfo /dev/mtdX'\n"
-"-h, --help                      print help message\n"
-"-V, --version                   print program version";
-
-static const char usage[] =
-"Usage: " PROGRAM_NAME " <MTD node file path> [--map | -M] [--ubi-info | -u]\n"
-"       " PROGRAM_NAME " --all [--ubi-info | -u]\n"
-"       " PROGRAM_NAME " [--help | --version]\n"
-"\n"
-"Example 1: " PROGRAM_NAME " /dev/mtd0       print information MTD device /dev/mtd0\n"
-"Example 2: " PROGRAM_NAME " /dev/mtd0 -u    print information MTD device /dev/mtd0\n"
-"\t\t\t\t   and include UBI layout information\n"
-"Example 3: " PROGRAM_NAME " -a              print information about all MTD devices\n";
+static void display_help(void)
+{
+	printf(
+		"%1$s version %2$s - a tool to print MTD information.\n"
+		"\n"
+		"Usage: %1$s <MTD node file path> [--map | -M] [--ubi-info | -u]\n"
+		"       %1$s --all [--ubi-info | -u]\n"
+		"       %1$s [--help | --version]\n"
+		"\n"
+		"Options:\n"
+		"-u, --ubi-info                  print what would UBI layout be if it was put\n"
+		"                                on this MTD device\n"
+		"-M, --map                       print eraseblock map\n"
+		"-a, --all                       print information about all MTD devices\n"
+		"                                Note: `--all' may give less info per device\n"
+		"                                than, e.g., `mtdinfo /dev/mtdX'\n"
+		"-h, --help                      print help message\n"
+		"-V, --version                   print program version\n"
+		"\n"
+		"Examples:\n"
+		"  %1$s /dev/mtd0             print information MTD device /dev/mtd0\n"
+		"  %1$s /dev/mtd0 -u          print information MTD device /dev/mtd0\n"
+		"  %4$*3$s                    and include UBI layout information\n"
+		"  %1$s -a                    print information about all MTD devices\n",
+		PROGRAM_NAME, VERSION, (int)strlen(PROGRAM_NAME) + 3, "");
+}
 
 static const struct option long_options[] = {
 	{ .name = "ubi-info",  .has_arg = 0, .flag = NULL, .val = 'u' },
@@ -105,9 +109,7 @@ static int parse_opt(int argc, char * const argv[])
 			break;
 
 		case 'h':
-			printf("%s\n\n", doc);
-			printf("%s\n\n", usage);
-			printf("%s\n", optionsstr);
+			display_help();
 			exit(EXIT_SUCCESS);
 
 		case 'V':
-- 
1.7.0.4

Re: [PATCH v2 0/5] bugfix, cleanup for mtdinfo

[Artem Bityutskiy]

On Tue, 2011-08-09 at 14:36 -0700, Brian Norris wrote:
> This series includes v2 of a bugfix patch, as discussed in this thread:
>   http://lists.infradead.org/pipermail/linux-mtd/2011-August/037386.html
> and previously. Now, `mtdinfo --all' will not try to print out
> region_info, since we need to supply a file path for use with ioctl. In
> the future, if sysfs support is supplied for region_info, then we can
> renew support in `mtdinfo --all'.
> 
> In addition to the bugfix, there are several other cleanups for mtdinfo.
> 
> Brian
> 
> Brian Norris (5):
>   mtdinfo: don't open NULL pointer when getting region_info with `-a'
>   mtdinfo: refactor code to remove "args.all" dependency
>   mtdinfo: restructure help message
>   mtdinfo: fixup "example usage" help section
>   mtdinfo: consolidate help as display_help()
> 
>  ubi-utils/mtdinfo.c |   61 ++++++++++++++++++++++++++------------------------
>  1 files changed, 32 insertions(+), 29 deletions(-)

Pushed the series to mtd-utils, thanks!

-- 
Best Regards,
Artem Bityutskiy