Change in flashrom[master]: cli_classic.c: Tidy up some repeated handling patterns into funcs
Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
cli_classic.c: Tidy up some repeated handling patterns into funcs
Introduce cli_classic_single_operation() to consolidate the repeating pattern of multiple CLI operations at once. Also modify cli_classic_abort_usage() to take an optional error abort string and print it to stderr, this allows for trimming a few more lines off the cli implementation.
Change-Id: I54598efdaee2b95cb278b0f2aac05f48bbd95bef Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M cli_classic.c 1 file changed, 47 insertions(+), 98 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/35611/1
diff --git a/cli_classic.c b/cli_classic.c index 6f362f1..7fd0495 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -71,12 +71,21 @@ "If no operation is specified, flashrom will only probe for flash chips.\n"); }
-static void cli_classic_abort_usage(void) +static void cli_classic_abort_usage(const char *msg) { printf("Please run "flashrom --help" for usage info.\n"); + if (msg) + fprintf(stderr, "%s", msg); exit(1); }
+static void cli_classic_single_operation(int operation_specified) +{ + if (++operation_specified > 1) { + cli_classic_abort_usage("More than one operation specified. Aborting.\n"); + } +} + static int check_filename(char *filename, char *type) { if (!filename || (filename[0] == '\0')) { @@ -171,41 +180,27 @@ long_options, &option_index)) != EOF) { switch (opt) { case 'r': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); filename = strdup(optarg); read_it = 1; break; case 'w': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); filename = strdup(optarg); write_it = 1; break; case 'v': //FIXME: gracefully handle superfluous -v - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); if (dont_verify_it) { - fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); } filename = strdup(optarg); verify_it = 1; break; case 'n': if (verify_it) { - fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); } dont_verify_it = 1; break; @@ -221,11 +216,7 @@ verbose_logfile = verbose_screen; break; case 'E': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); erase_it = 1; break; case 'f': @@ -233,61 +224,49 @@ break; case 'l': if (layoutfile) { - fprintf(stderr, "Error: --layout specified " - "more than once. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --layout specified more than once. Aborting.\n"); } if (ifd) { - fprintf(stderr, "Error: --layout and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); } if (fmap) { - fprintf(stderr, "Error: --layout and --fmap-file both specified. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --layout and --fmap-file both specified. Aborting.\n"); } layoutfile = strdup(optarg); break; case OPTION_IFD: if (layoutfile) { - fprintf(stderr, "Error: --layout and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); } if (fmap) { - fprintf(stderr, "Error: --fmap-file and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --fmap-file and --ifd both specified. Aborting.\n"); } ifd = 1; break; case OPTION_FMAP_FILE: if (fmap) { - fprintf(stderr, "Error: --fmap or --fmap-file specified " + cli_classic_abort_usage("Error: --fmap or --fmap-file specified " "more than once. Aborting.\n"); - cli_classic_abort_usage(); } if (ifd) { - fprintf(stderr, "Error: --fmap-file and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --fmap-file and --ifd both specified. Aborting.\n"); } if (layoutfile) { - fprintf(stderr, "Error: --fmap-file and --layout both specified. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --fmap-file and --layout both specified. Aborting.\n"); } fmapfile = strdup(optarg); fmap = 1; break; case OPTION_FMAP: if (fmap) { - fprintf(stderr, "Error: --fmap or --fmap-file specified " + cli_classic_abort_usage("Error: --fmap or --fmap-file specified " "more than once. Aborting.\n"); - cli_classic_abort_usage(); } if (ifd) { - fprintf(stderr, "Error: --fmap and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --fmap and --ifd both specified. Aborting.\n"); } if (layoutfile) { - fprintf(stderr, "Error: --layout and --fmap both specified. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: --layout and --fmap both specified. Aborting.\n"); } fmap = 1; break; @@ -295,58 +274,40 @@ tempstr = strdup(optarg); if (register_include_arg(&include_args, tempstr)) { free(tempstr); - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); } break; case OPTION_FLASH_CONTENTS: referencefile = strdup(optarg); break; case OPTION_FLASH_NAME: - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); flash_name = 1; break; case OPTION_GET_SIZE: - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); get_size = 1; break; case 'L': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); list_supported = 1; break; case 'z': #if CONFIG_PRINT_WIKI == 1 - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); list_supported_wiki = 1; #else - fprintf(stderr, "Error: Wiki output was not compiled " + cli_classic_abort_usage("Error: Wiki output was not compiled " "in. Aborting.\n"); - cli_classic_abort_usage(); #endif break; case 'p': if (prog != PROGRAMMER_INVALID) { - fprintf(stderr, "Error: --programmer specified " + cli_classic_abort_usage("Error: --programmer specified " "more than once. You can separate " "multiple\nparameters for a programmer " "with ",". Please see the man page " "for details.\n"); - cli_classic_abort_usage(); } for (prog = 0; prog < PROGRAMMER_INVALID; prog++) { name = programmer_table[prog].name; @@ -378,31 +339,22 @@ optarg); list_programmers_linebreak(0, 80, 0); msg_ginfo(".\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); } break; case 'R': /* print_version() is always called during startup. */ - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); exit(0); break; case 'h': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_single_operation(operation_specified); cli_classic_usage(argv[0]); exit(0); break; case 'o': #ifdef STANDALONE - fprintf(stderr, "Log file not supported in standalone mode. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Log file not supported in standalone mode. Aborting.\n"); #else /* STANDALONE */ if (logfile) { fprintf(stderr, "Warning: -o/--output specified multiple times.\n"); @@ -411,40 +363,37 @@
logfile = strdup(optarg); if (logfile[0] == '\0') { - fprintf(stderr, "No log filename specified.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("No log filename specified.\n"); } #endif /* STANDALONE */ break; default: - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); break; } }
if (optind < argc) { - fprintf(stderr, "Error: Extra parameter found.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: Extra parameter found.\n"); } - if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); } if (layoutfile && check_filename(layoutfile, "layout")) { - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); } if (fmapfile && check_filename(fmapfile, "fmap")) { - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); } if (referencefile && check_filename(referencefile, "reference")) { - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); }
#ifndef STANDALONE if (logfile && check_filename(logfile, "log")) - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); if (logfile && open_logfile(logfile)) - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); #endif /* !STANDALONE */
#if CONFIG_PRINT_WIKI == 1
Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 1: Code-Review+1
Nice!
Hello Alan Green, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/35611
to look at the new patch set (#2).
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
cli_classic.c: Tidy up some repeated handling patterns into funcs
Introduce cli_classic_single_operation() to consolidate the repeating pattern of multiple CLI operations at once. Also modify cli_classic_abort_usage() to take an optional error abort string and print it to stderr, this allows for trimming a few more lines off the cli implementation.
Change-Id: I54598efdaee2b95cb278b0f2aac05f48bbd95bef Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M cli_classic.c 1 file changed, 47 insertions(+), 98 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/35611/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@74 PS2, Line 74: static void cli_classic_abort_usage(const char *msg) Minor: Is there some sort of "noreturn" attribute for functions like this one? I guess it could be used here.
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@82 PS2, Line 82: int operation_specified Does this work? AFAIK, this parameter gets passed by value, so incrementing it inside the function will not result in a change outside of it.
I guess you would want to pass a reference (pointer) instead.
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@82 PS2, Line 82: cli_classic_single_operation A function name without a verb? Since functions do things, I would appreciate if the name had a verb somewhere, e.g.: check, test.
Maybe something like "test single operation or abort" would reflect what this function does better.
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@226 PS2, Line 226: if (layoutfile) { : cli_classic_abort_usage("Error: --layout specified more than once. Aborting.\n"); : } : if (ifd) { : cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); : } : if (fmap) { : cli_classic_abort_usage("Error: --layout and --fmap-file both specified. Aborting.\n"); : } I shall point out that this is quite redundant as well, so that you can't unsee it :)
Hello build bot (Jenkins), Alan Green, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/35611
to look at the new patch set (#3).
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
cli_classic.c: Tidy up some repeated handling patterns into funcs
Introduce cli_classic_single_operation() to consolidate the repeating pattern of multiple CLI operations at once. Also modify cli_classic_abort_usage() to take an optional error abort string and print it to stderr, this allows for trimming a few more lines off the cli implementation.
V.2: A few fixes upon review: - Trim off some unnecessary braces for single line branches. - Pass 'operation_specified' by reference. - Rename a function.
Change-Id: I54598efdaee2b95cb278b0f2aac05f48bbd95bef Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M cli_classic.c 1 file changed, 63 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/35611/3
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@74 PS2, Line 74: static void cli_classic_abort_usage(const char *msg)
Minor: Is there some sort of "noreturn" attribute for functions like this one? I guess it could be u […]
Sounds a bit unrelated to this change, maybe it should go into the compiler warn cleanups I saw since I am not introducing this function here.
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@82 PS2, Line 82: int operation_specified
Does this work? AFAIK, this parameter gets passed by value, so incrementing it inside the function w […]
woops, done thanks! O_o
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@82 PS2, Line 82: cli_classic_single_operation
A function name without a verb? Since functions do things, I would appreciate if the name had a verb […]
Done
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@226 PS2, Line 226: if (layoutfile) { : cli_classic_abort_usage("Error: --layout specified more than once. Aborting.\n"); : } : if (ifd) { : cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); : } : if (fmap) { : cli_classic_abort_usage("Error: --layout and --fmap-file both specified. Aborting.\n"); : }
I shall point out that this is quite redundant as well, so that you can't unsee it :)
I assume you mean the extra '{ .. }' here, done!
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 3: Code-Review+1
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/flashrom/+/35611/3/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/35611/3/cli_classic.c@a430 PS3, Line 430: if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { : cli_classic_abort_usage(); : } : if (layoutfile && check_filename(layoutfile, "layout")) { : cli_classic_abort_usage(); : } : if (fmapfile && check_filename(fmapfile, "fmap")) { : cli_classic_abort_usage(); : } : if (referencefile && check_filename(referencefile, "reference")) { : cli_classic_abort_usage(); : } are we removing this more "defensive" coding style on purpose?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 3:
(1 comment)
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/flashrom/+/35611/3/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/35611/3/cli_classic.c@a430 PS3, Line 430: if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { : cli_classic_abort_usage(); : } : if (layoutfile && check_filename(layoutfile, "layout")) { : cli_classic_abort_usage(); : } : if (fmapfile && check_filename(fmapfile, "fmap")) { : cli_classic_abort_usage(); : } : if (referencefile && check_filename(referencefile, "reference")) { : cli_classic_abort_usage(); : }
are we removing this more "defensive" coding style on purpose?
It was requested in the previous review? I don't think this is defensive much here as there isn't any resource management to be had on these branches that validate arg combinatorics.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 3: Code-Review+2
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 3: Code-Review+1
Hello David Hendricks, Stefan Reinauer, build bot (Jenkins), Alan Green, Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/35611
to look at the new patch set (#4).
Change subject: cli_classic.c: Tidy up some repeated handling patterns into funcs ......................................................................
cli_classic.c: Tidy up some repeated handling patterns into funcs
Introduce cli_classic_single_operation() to consolidate the repeating pattern of multiple CLI operations at once. Also modify cli_classic_abort_usage() to take an optional error abort string and print it to stderr, this allows for trimming a few more lines off the cli implementation.
V.2: A few fixes upon review: - Trim off some unnecessary braces for single line branches. - Pass 'operation_specified' by reference. - Rename a function.
Change-Id: I54598efdaee2b95cb278b0f2aac05f48bbd95bef Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M cli_classic.c 1 file changed, 63 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/35611/4
Hello David Hendricks, Stefan Reinauer, build bot (Jenkins), Alan Green, Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/35611
to look at the new patch set (#5).
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
cli_classic: Tidy up some repeated handling patterns into funcs
Introduce cli_classic_single_operation() to consolidate the repeating pattern of multiple CLI operations at once. Also modify cli_classic_abort_usage() to take an optional error abort string and print it to stderr, this allows for trimming a few more lines off the cli implementation.
V.2: A few fixes upon review: - Trim off some unnecessary braces for single line branches. - Pass 'operation_specified' by reference. - Rename a function.
Change-Id: I54598efdaee2b95cb278b0f2aac05f48bbd95bef Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M cli_classic.c 1 file changed, 63 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/35611/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
LGTM, but waiting for Stefan's ack on the open comment.
https://review.coreboot.org/c/flashrom/+/35611/3/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/35611/3/cli_classic.c@a430 PS3, Line 430: if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { : cli_classic_abort_usage(); : } : if (layoutfile && check_filename(layoutfile, "layout")) { : cli_classic_abort_usage(); : } : if (fmapfile && check_filename(fmapfile, "fmap")) { : cli_classic_abort_usage(); : } : if (referencefile && check_filename(referencefile, "reference")) { : cli_classic_abort_usage(); : }
It was requested in the previous review? I don't think this is defensive much here as there isn't an […]
I didn't express my concerns properly on my comment on patchset 2. What I was thinking of was not about the braces, but rather reducing the number of printed strings.
However, now that I see it, I agree it would result in a less "defensive" coding style, so I wouldn't change it now. Apologies for the noise.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 5: Code-Review+1
(3 comments)
Looks good, I just wonder why the printing order is changed?
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/35611/2/cli_classic.c@74 PS2, Line 74: static void cli_classic_abort_usage(const char *msg)
Sounds a bit unrelated to this change, maybe it should go into the compiler warn cleanups I saw sinc […]
Ack
https://review.coreboot.org/c/flashrom/+/35611/5/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/35611/5/cli_classic.c@79 PS5, Line 79: if (msg) : fprintf(stderr, "%s", msg); Shouldn't this be the first message?
https://review.coreboot.org/c/flashrom/+/35611/5/cli_classic.c@293 PS5, Line 293: "in. Aborting.\n"); nit, does it fit in a single line?
Hello Angel Pons, David Hendricks, Stefan Reinauer, build bot (Jenkins), Alan Green, Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/35611
to look at the new patch set (#7).
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
cli_classic: Tidy up some repeated handling patterns into funcs
Introduce cli_classic_single_operation() to consolidate the repeating pattern of multiple CLI operations at once. Also modify cli_classic_abort_usage() to take an optional error abort string and print it to stderr, this allows for trimming a few more lines off the cli implementation.
V.2: A few fixes upon review: - Trim off some unnecessary braces for single line branches. - Pass 'operation_specified' by reference. - Rename a function. V.3: Fix print order of cli_classic_abort_usage().
Change-Id: I54598efdaee2b95cb278b0f2aac05f48bbd95bef Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M cli_classic.c 1 file changed, 64 insertions(+), 131 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/35611/7
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/flashrom/+/35611/5/cli_classic.c File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/35611/5/cli_classic.c@79 PS5, Line 79: if (msg) : fprintf(stderr, "%s", msg);
Shouldn't this be the first message?
Done
https://review.coreboot.org/c/flashrom/+/35611/5/cli_classic.c@293 PS5, Line 293: "in. Aborting.\n");
nit, does it fit in a single line?
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 7: Code-Review+2
Hello Angel Pons, David Hendricks, Stefan Reinauer, build bot (Jenkins), Alan Green, Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/35611
to look at the new patch set (#8).
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
cli_classic: Tidy up some repeated handling patterns into funcs
Introduce cli_classic_single_operation() to consolidate the repeating pattern of multiple CLI operations at once. Also modify cli_classic_abort_usage() to take an optional error abort string and print it to stderr, this allows for trimming a few more lines off the cli implementation.
V.2: A few fixes upon review: - Trim off some unnecessary braces for single line branches. - Pass 'operation_specified' by reference. - Rename a function. V.3: Fix print order of cli_classic_abort_usage().
Change-Id: I54598efdaee2b95cb278b0f2aac05f48bbd95bef Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M cli_classic.c 1 file changed, 64 insertions(+), 131 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/35611/8
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
Patch Set 8: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/35611 )
Change subject: cli_classic: Tidy up some repeated handling patterns into funcs ......................................................................
cli_classic: Tidy up some repeated handling patterns into funcs
Introduce cli_classic_single_operation() to consolidate the repeating pattern of multiple CLI operations at once. Also modify cli_classic_abort_usage() to take an optional error abort string and print it to stderr, this allows for trimming a few more lines off the cli implementation.
V.2: A few fixes upon review: - Trim off some unnecessary braces for single line branches. - Pass 'operation_specified' by reference. - Rename a function. V.3: Fix print order of cli_classic_abort_usage().
Change-Id: I54598efdaee2b95cb278b0f2aac05f48bbd95bef Signed-off-by: Edward O'Callaghan quasisec@chromium.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/35611 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M cli_classic.c 1 file changed, 64 insertions(+), 131 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/cli_classic.c b/cli_classic.c index de6ce04..73cc417 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -76,12 +76,21 @@ "If no operation is specified, flashrom will only probe for flash chips.\n"); }
-static void cli_classic_abort_usage(void) +static void cli_classic_abort_usage(const char *msg) { + if (msg) + fprintf(stderr, "%s", msg); printf("Please run "flashrom --help" for usage info.\n"); exit(1); }
+static void cli_classic_validate_singleop(int *operation_specified) +{ + if (++(*operation_specified) > 1) { + cli_classic_abort_usage("More than one operation specified. Aborting.\n"); + } +} + static int check_filename(char *filename, const char *type) { if (!filename || (filename[0] == '\0')) { @@ -177,41 +186,27 @@ long_options, &option_index)) != EOF) { switch (opt) { case 'r': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); filename = strdup(optarg); read_it = 1; break; case 'w': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); filename = strdup(optarg); write_it = 1; break; case 'v': //FIXME: gracefully handle superfluous -v - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); if (dont_verify_it) { - fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); } filename = strdup(optarg); verify_it = 1; break; case 'n': if (verify_it) { - fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); } dont_verify_it = 1; break; @@ -227,132 +222,87 @@ verbose_logfile = verbose_screen; break; case 'E': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); erase_it = 1; break; case 'f': force = 1; break; case 'l': - if (layoutfile) { - fprintf(stderr, "Error: --layout specified " - "more than once. Aborting.\n"); - cli_classic_abort_usage(); - } - if (ifd) { - fprintf(stderr, "Error: --layout and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); - } - if (fmap) { - fprintf(stderr, "Error: --layout and --fmap-file both specified. Aborting.\n"); - cli_classic_abort_usage(); - } + if (layoutfile) + cli_classic_abort_usage("Error: --layout specified more than once. Aborting.\n"); + if (ifd) + cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); + if (fmap) + cli_classic_abort_usage("Error: --layout and --fmap-file both specified. Aborting.\n"); layoutfile = strdup(optarg); break; case OPTION_IFD: - if (layoutfile) { - fprintf(stderr, "Error: --layout and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); - } - if (fmap) { - fprintf(stderr, "Error: --fmap-file and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); - } + if (layoutfile) + cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); + if (fmap) + cli_classic_abort_usage("Error: --fmap-file and --ifd both specified. Aborting.\n"); ifd = 1; break; case OPTION_FMAP_FILE: - if (fmap) { - fprintf(stderr, "Error: --fmap or --fmap-file specified " + if (fmap) + cli_classic_abort_usage("Error: --fmap or --fmap-file specified " "more than once. Aborting.\n"); - cli_classic_abort_usage(); - } - if (ifd) { - fprintf(stderr, "Error: --fmap-file and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); - } - if (layoutfile) { - fprintf(stderr, "Error: --fmap-file and --layout both specified. Aborting.\n"); - cli_classic_abort_usage(); - } + if (ifd) + cli_classic_abort_usage("Error: --fmap-file and --ifd both specified. Aborting.\n"); + if (layoutfile) + cli_classic_abort_usage("Error: --fmap-file and --layout both specified. Aborting.\n"); fmapfile = strdup(optarg); fmap = 1; break; case OPTION_FMAP: - if (fmap) { - fprintf(stderr, "Error: --fmap or --fmap-file specified " + if (fmap) + cli_classic_abort_usage("Error: --fmap or --fmap-file specified " "more than once. Aborting.\n"); - cli_classic_abort_usage(); - } - if (ifd) { - fprintf(stderr, "Error: --fmap and --ifd both specified. Aborting.\n"); - cli_classic_abort_usage(); - } - if (layoutfile) { - fprintf(stderr, "Error: --layout and --fmap both specified. Aborting.\n"); - cli_classic_abort_usage(); - } + if (ifd) + cli_classic_abort_usage("Error: --fmap and --ifd both specified. Aborting.\n"); + if (layoutfile) + cli_classic_abort_usage("Error: --layout and --fmap both specified. Aborting.\n"); fmap = 1; break; case 'i': tempstr = strdup(optarg); if (register_include_arg(&include_args, tempstr)) { free(tempstr); - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); } break; case OPTION_FLASH_CONTENTS: referencefile = strdup(optarg); break; case OPTION_FLASH_NAME: - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); flash_name = 1; break; case OPTION_FLASH_SIZE: - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); flash_size = 1; break; case 'L': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); list_supported = 1; break; case 'z': #if CONFIG_PRINT_WIKI == 1 - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); list_supported_wiki = 1; #else - fprintf(stderr, "Error: Wiki output was not compiled " - "in. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Error: Wiki output was not" + "compiled in. Aborting.\n"); #endif break; case 'p': if (prog != PROGRAMMER_INVALID) { - fprintf(stderr, "Error: --programmer specified " + cli_classic_abort_usage("Error: --programmer specified " "more than once. You can separate " "multiple\nparameters for a programmer " "with ",". Please see the man page " "for details.\n"); - cli_classic_abort_usage(); } for (prog = 0; prog < PROGRAMMER_INVALID; prog++) { name = programmer_table[prog].name; @@ -384,31 +334,22 @@ optarg); list_programmers_linebreak(0, 80, 0); msg_ginfo(".\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); } break; case 'R': /* print_version() is always called during startup. */ - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); exit(0); break; case 'h': - if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); - } + cli_classic_validate_singleop(&operation_specified); cli_classic_usage(argv[0]); exit(0); break; case 'o': #ifdef STANDALONE - fprintf(stderr, "Log file not supported in standalone mode. Aborting.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("Log file not supported in standalone mode. Aborting.\n"); #else /* STANDALONE */ if (logfile) { fprintf(stderr, "Warning: -o/--output specified multiple times.\n"); @@ -417,40 +358,32 @@
logfile = strdup(optarg); if (logfile[0] == '\0') { - fprintf(stderr, "No log filename specified.\n"); - cli_classic_abort_usage(); + cli_classic_abort_usage("No log filename specified.\n"); } #endif /* STANDALONE */ break; default: - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); break; } }
- if (optind < argc) { - fprintf(stderr, "Error: Extra parameter found.\n"); - cli_classic_abort_usage(); - } - - if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { - cli_classic_abort_usage(); - } - if (layoutfile && check_filename(layoutfile, "layout")) { - cli_classic_abort_usage(); - } - if (fmapfile && check_filename(fmapfile, "fmap")) { - cli_classic_abort_usage(); - } - if (referencefile && check_filename(referencefile, "reference")) { - cli_classic_abort_usage(); - } + if (optind < argc) + cli_classic_abort_usage("Error: Extra parameter found.\n"); + if ((read_it | write_it | verify_it) && check_filename(filename, "image")) + cli_classic_abort_usage(NULL); + if (layoutfile && check_filename(layoutfile, "layout")) + cli_classic_abort_usage(NULL); + if (fmapfile && check_filename(fmapfile, "fmap")) + cli_classic_abort_usage(NULL); + if (referencefile && check_filename(referencefile, "reference")) + cli_classic_abort_usage(NULL);
#ifndef STANDALONE if (logfile && check_filename(logfile, "log")) - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); if (logfile && open_logfile(logfile)) - cli_classic_abort_usage(); + cli_classic_abort_usage(NULL); #endif /* !STANDALONE */
#if CONFIG_PRINT_WIKI == 1
-
Alan Green (Code Review) -
Angel Pons (Code Review)
-
David Hendricks (Code Review)
-
Edward O'Callaghan (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)