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

login
register
mail settings
Submitter Brian Norris
Date Aug. 11, 2011, 4:19 p.m.
Message ID <1313079580-22144-1-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/109649/
State New
Headers show

Comments

Brian Norris - Aug. 11, 2011, 4:19 p.m.
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(-)
Mike Frysinger - Aug. 11, 2011, 5:15 p.m.
Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
Brian Foster - Aug. 12, 2011, 7:06 a.m.
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 Norris - Aug. 12, 2011, 4:37 p.m.
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
Mike Frysinger - Aug. 13, 2011, 6:10 p.m.
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
Brian Norris - Aug. 15, 2011, 5:27 p.m.
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
Mike Frysinger - Aug. 16, 2011, 4 a.m.
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
Brian Foster - Aug. 16, 2011, 7:18 a.m.
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 Norris - Aug. 16, 2011, 5:12 p.m.
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
Brian Foster - Aug. 17, 2011, 8:02 a.m.
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-

Patch

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':