This was mainly driven by getting rid of cli_classic_abort_usage() in cli_classic.c.
This makes the whole main function more consistent and fixes a myriad of (theoretical) memory leaks. This slightly changes output in most error cases. doit() stops calling programmer_shutdown() with this patch, beware.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at --- cli_classic.c | 147 ++++++++++++++++++++++++---------------------------------- flashrom.c | 13 +++--- 2 files changed, 68 insertions(+), 92 deletions(-)
diff --git a/cli_classic.c b/cli_classic.c index 4c71d07..5ad554c 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -71,12 +71,6 @@ static void cli_classic_usage(const char *name) "If no operation is specified, flashrom will only probe for flash chips.\n"); }
-static void cli_classic_abort_usage(void) -{ - printf("Please run "flashrom --help" for usage info.\n"); - exit(1); -} - static int check_filename(char *filename, char *type) { if (!filename || (filename[0] == '\0')) { @@ -151,40 +145,36 @@ int main(int argc, char *argv[]) switch (opt) { case 'r': if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); + fprintf(stderr, "More than one operation specified.\n"); + goto out_abort; } 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(); + fprintf(stderr, "More than one operation specified.\n"); + goto out_abort; } 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(); + fprintf(stderr, "More than one operation specified.\n"); + goto out_abort; } if (dont_verify_it) { - fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); - cli_classic_abort_usage(); + fprintf(stderr, "--verify and --noverify are mutually exclusive.\n"); + goto out_abort; } 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(); + fprintf(stderr, "--verify and --noverify are mutually exclusive.\n"); + goto out_abort; } dont_verify_it = 1; break; @@ -198,9 +188,8 @@ int main(int argc, char *argv[]) break; case 'E': if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); + fprintf(stderr, "More than one operation specified.\n"); + goto out_abort; } erase_it = 1; break; @@ -209,9 +198,8 @@ int main(int argc, char *argv[]) break; case 'l': if (layoutfile) { - fprintf(stderr, "Error: --layout specified " - "more than once. Aborting.\n"); - cli_classic_abort_usage(); + fprintf(stderr, "Error: --layout specified more than once.\n"); + goto out_abort; } layoutfile = strdup(optarg); break; @@ -219,31 +207,28 @@ int main(int argc, char *argv[]) tempstr = strdup(optarg); if (register_include_arg(tempstr)) { free(tempstr); - cli_classic_abort_usage(); + goto out_abort; } /* FIXME: A pointer to the image name is saved in a static array (of size MAX_ROMLAYOUT) * by register_include_arg() and needs to be freed after processing them. */ break; case 'L': if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); + fprintf(stderr, "More than one operation specified.\n"); + goto out_abort; } 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(); + fprintf(stderr, "More than one operation specified.\n"); + goto out_abort; } list_supported_wiki = 1; #else - fprintf(stderr, "Error: Wiki output was not compiled " - "in. Aborting.\n"); - cli_classic_abort_usage(); + fprintf(stderr, "Error: Wiki output was not compiled in.\n"); + goto out_abort; #endif break; case 'p': @@ -253,7 +238,7 @@ int main(int argc, char *argv[]) "multiple\nparameters for a programmer " "with ",". Please see the man page " "for details.\n"); - cli_classic_abort_usage(); + goto out_abort; } for (prog = 0; prog < PROGRAMMER_INVALID; prog++) { name = programmer_table[prog].name; @@ -285,63 +270,59 @@ int main(int argc, char *argv[]) optarg); list_programmers_linebreak(0, 80, 0); msg_ginfo(".\n"); - cli_classic_abort_usage(); + goto out_abort; } 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(); + fprintf(stderr, "More than one operation specified.\n"); + goto out_abort; } - exit(0); - break; + goto out; case 'h': if (++operation_specified > 1) { - fprintf(stderr, "More than one operation " - "specified. Aborting.\n"); - cli_classic_abort_usage(); + fprintf(stderr, "More than one operation specified.\n"); + goto out_abort; } cli_classic_usage(argv[0]); - exit(0); - break; + goto out; case 'o': #ifdef STANDALONE - fprintf(stderr, "Log file not supported in standalone mode. Aborting.\n"); - cli_classic_abort_usage(); + fprintf(stderr, "Log file not supported in standalone mode.\n"); + goto out_abort; #else /* STANDALONE */ logfile = strdup(optarg); if (logfile[0] == '\0') { fprintf(stderr, "No log filename specified.\n"); - cli_classic_abort_usage(); + goto out_abort; } #endif /* STANDALONE */ break; default: - cli_classic_abort_usage(); - break; + goto out_abort; } }
if (optind < argc) { fprintf(stderr, "Error: Extra parameter found.\n"); - cli_classic_abort_usage(); + goto out_abort; }
if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { - cli_classic_abort_usage(); + goto out_abort; } if (layoutfile && check_filename(layoutfile, "layout")) { - cli_classic_abort_usage(); + goto out_abort; }
#ifndef STANDALONE - if (logfile && check_filename(logfile, "log")) - cli_classic_abort_usage(); - if (logfile && open_logfile(logfile)) - return 1; - free(logfile); + if (logfile && check_filename(logfile, "log")) { + goto out_abort; + } + if (logfile && open_logfile(logfile)) { + goto out_abort; + } #endif /* !STANDALONE */
#if CONFIG_PRINT_WIKI == 1 @@ -353,7 +334,7 @@ int main(int argc, char *argv[])
if (list_supported) { if (print_supported()) - ret = 1; + goto out_abort; goto out; }
@@ -369,12 +350,10 @@ int main(int argc, char *argv[]) msg_gdbg("\n");
if (layoutfile && read_romlayout(layoutfile)) { - ret = 1; - goto out; + goto out_abort; } if (process_include_args()) { - ret = 1; - goto out; + goto out_abort; } /* Does a chip with the requested name exist in the flashchips array? */ if (chip_to_probe) { @@ -384,8 +363,7 @@ int main(int argc, char *argv[]) if (!chip || !chip->name) { msg_cerr("Error: Unknown chip '%s' specified.\n", chip_to_probe); msg_gerr("Run flashrom -L to view the hardware supported in this flashrom version.\n"); - ret = 1; - goto out; + goto out_abort; } /* Keep chip around for later usage in case a forced read is requested. */ } @@ -404,8 +382,7 @@ int main(int argc, char *argv[]) "Valid choices are:\n"); list_programmers_linebreak(0, 80, 0); msg_ginfo(".\n"); - ret = 1; - goto out; + goto out_abort; } }
@@ -415,7 +392,7 @@ int main(int argc, char *argv[]) if (programmer_init(prog, pparam)) { msg_perr("Error: Programmer initialization failed.\n"); ret = 1; - goto out_shutdown; + goto out; /* Lots of init code prints "Aborting." at the end. Avoid printing it again here. */ } tempstr = flashbuses_to_text(get_buses_supported()); msg_pdbg("The following protocols are supported: %s.\n", tempstr); @@ -438,8 +415,7 @@ int main(int argc, char *argv[]) for (i = 1; i < chipcount; i++) msg_cinfo(", "%s"", flashes[i].chip->name); msg_cinfo("\nPlease specify which chip definition to use with the -c <chipname> option.\n"); - ret = 1; - goto out_shutdown; + goto out_abort; } else if (!chipcount) { msg_cinfo("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { @@ -459,8 +435,7 @@ int main(int argc, char *argv[]) } if (!compatible_programmers) { msg_cinfo("No compatible controller found for the requested flash chip.\n"); - ret = 1; - goto out_shutdown; + goto out_abort; } if (compatible_programmers > 1) msg_cinfo("More than one compatible controller found for the requested flash " @@ -474,16 +449,14 @@ int main(int argc, char *argv[]) if (startchip == -1) { // FIXME: This should never happen! Ask for a bug report? msg_cinfo("Probing for flash chip '%s' failed.\n", chip_to_probe); - ret = 1; - goto out_shutdown; + goto out_abort; } msg_cinfo("Please note that forced reads most likely contain garbage.\n"); ret = read_flash_to_file(&flashes[0], filename); free(flashes[0].chip); - goto out_shutdown; + goto out; } - ret = 1; - goto out_shutdown; + goto out_abort; } else if (!chip_to_probe) { /* repeat for convenience when looking at foreign logs */ tempstr = flashbuses_to_text(flashes[0].chip->bustype); @@ -499,13 +472,12 @@ int main(int argc, char *argv[]) size = fill_flash->chip->total_size * 1024; if (check_max_decode(fill_flash->pgm->buses_supported & fill_flash->chip->bustype, size) && (!force)) { msg_cerr("Chip is too big for this programmer (-V gives details). Use --force to override.\n"); - ret = 1; - goto out_shutdown; + goto out_abort; }
if (!(read_it | write_it | verify_it | erase_it)) { msg_ginfo("No operations were specified.\n"); - goto out_shutdown; + goto out; }
/* Always verify write operations unless -n is used. */ @@ -518,12 +490,14 @@ int main(int argc, char *argv[]) */ programmer_delay(100000); ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); - /* Note: doit() already calls programmer_shutdown(). */ goto out;
-out_shutdown: - programmer_shutdown(); +out_abort: + msg_gerr("Aborting.\n"); + ret = 1; out: + programmer_shutdown(); + for (i = 0; i < chipcount; i++) free(flashes[i].chip);
@@ -534,6 +508,7 @@ out: free((char *)chip_to_probe); /* Silence! Freeing is not modifying contents. */ chip_to_probe = NULL; #ifndef STANDALONE + free(logfile); ret |= close_logfile(); #endif /* !STANDALONE */ return ret; diff --git a/flashrom.c b/flashrom.c index 86e64a2..0f4b367 100644 --- a/flashrom.c +++ b/flashrom.c @@ -413,6 +413,11 @@ int programmer_init(enum programmer prog, const char *param) return ret; }
+/** Calls registered shutdown functions and resets internal programmer-related variables. + * Calling it is safe even without previous initialization, but further interactions with programmer support + * require a call to programmer_init() (afterwards). + * + * @return The OR-ed result values of all shutdown functions (i.e. 0 on success). */ int programmer_shutdown(void) { int ret = 0; @@ -1900,8 +1905,7 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { msg_cerr("Aborting.\n"); - ret = 1; - goto out_nofree; + return 1; }
/* Given the existence of read locks, we want to unlock for read, @@ -1911,8 +1915,7 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it, flash->chip->unlock(flash);
if (read_it) { - ret = read_flash_to_file(flash, filename); - goto out_nofree; + return read_flash_to_file(flash, filename); }
oldcontents = malloc(size); @@ -2030,7 +2033,5 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it, out: free(oldcontents); free(newcontents); -out_nofree: - programmer_shutdown(); return ret; }