>From 3997dffde2789b7c904f8a9f93a74fd5513220c4 Mon Sep 17 00:00:00 2001
From: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>
Date: Wed, 7 May 2014 18:26:13 +0200
Subject: [PATCH] Reorganize shutting down.

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.

Signed-off-by: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>
---
 cli_classic.c | 139 ++++++++++++++++++++++++----------------------------------
 1 file changed, 57 insertions(+), 82 deletions(-)

diff --git a/cli_classic.c b/cli_classic.c
index a32d55b..2d309c0 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,29 +207,26 @@ int main(int argc, char *argv[])
 			tempstr = strdup(optarg);
 			if (register_include_arg(tempstr)) {
 				free(tempstr);
-				cli_classic_abort_usage();
+				goto out_abort;
 			}
 			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':
@@ -251,7 +236,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;
@@ -283,62 +268,57 @@ 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();
+		goto out_abort;
 	if (logfile && open_logfile(logfile))
-		cli_classic_abort_usage();
+		goto out_abort;
 #endif /* !STANDALONE */
 
 #if CONFIG_PRINT_WIKI == 1
@@ -350,7 +330,7 @@ int main(int argc, char *argv[])
 
 	if (list_supported) {
 		if (print_supported())
-			ret = 1;
+			goto out_abort;
 		goto out;
 	}
 
@@ -366,8 +346,7 @@ int main(int argc, char *argv[])
 	msg_gdbg("\n");
 
 	if (layoutfile && read_romlayout(layoutfile)) {
-		ret = 1;
-		goto out;
+		goto out_abort;
 	}
 	if (layoutfile != NULL && !write_it) {
 		msg_gerr("Layout files are currently supported for write operations only.\n");
@@ -376,8 +355,7 @@ int main(int argc, char *argv[])
 	}
 
 	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) {
@@ -387,8 +365,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. */
 	}
@@ -407,8 +384,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;
 		}
 	}
 
@@ -418,7 +394,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);
@@ -441,8 +417,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) {
@@ -462,8 +437,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 "
@@ -477,16 +451,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);
@@ -502,13 +474,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. */
@@ -521,10 +492,14 @@ int main(int argc, char *argv[])
 	 */
 	programmer_delay(100000);
 	ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it);
+	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);
 
-- 
Kind regards, Stefan Tauner

