[flashrom] [PATCH] Reorganize shutting down.
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Fri Aug 30 02:00:34 CEST 2013
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 at 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;
}
--
Kind regards, Stefan Tauner
More information about the flashrom
mailing list