[PATCH] Fix and clean up cli_classic.c

[Carl-Daniel Hailfinger]

Don't ignore -i if it is specified before -l
Check if image mentioned by -i is present in layout file
Clean up cli_classic.c:
- Consolidate duplicated programmer_shutdown calls
- Kill outdated comments
- Finish parameter checking before -L/-z is executed

This patch was inspired by a cleanup patch by Stefan Tauner.
I did not fix up the -i/-l ordering dependency because the new layout
code may have different requirements anyway, and introducing gratuitous
user interface changes would be a bad idea.
Side note: This also reduces the amount of code changes needed for the
logfile patch.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-cli_classic_cleanup/cli_classic.c
===================================================================
--- flashrom-cli_classic_cleanup/cli_classic.c	(Revision 1372)
+++ flashrom-cli_classic_cleanup/cli_classic.c	(Arbeitskopie)
@@ -117,6 +117,7 @@
 #endif
    int operation_specified = 0;
    int i;
+	int ret = 0;

static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
    static const struct option long_options[] = {
@@ -232,8 +233,14 @@
    			cli_classic_abort_usage();
    		break;
    	case 'i':
+			/* FIXME: -l has to be specified before -i. */
    		tempstr = strdup(optarg);
-			find_romentry(tempstr);
+			if (find_romentry(tempstr)) {
+				fprintf(stderr, "Error: image %s not found in "
+					"layout file or -i specified before "
+					"-l\n", tempstr);
+				cli_classic_abort_usage();
+			}
    		break;
    	case 'L':
    		if (++operation_specified > 1) {
@@ -313,6 +320,11 @@
    	}
    }

+	if (optind < argc) {
+		fprintf(stderr, "Error: Extra parameter found.\n");
+		cli_classic_abort_usage();
+	}
+
    /* FIXME: Print the actions flashrom will take. */

if (list_supported) {
@@ -327,11 +339,6 @@
    }
 #endif

-	if (optind < argc) {
-		fprintf(stderr, "Error: Extra parameter found.\n");
-		cli_classic_abort_usage();
-	}
-
 #if CONFIG_INTERNAL == 1
    if ((programmer != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
    	fprintf(stderr, "Error: --mainboard requires the internal "
@@ -360,8 +367,8 @@

if (programmer_init(pparam)) {
    	fprintf(stderr, "Error: Programmer initialization failed.\n");
-		programmer_shutdown();
-		exit(1);
+		ret = 1;
+		goto out_shutdown;
    }

for (i = 0; i < ARRAY_SIZE(flashes); i++) {
@@ -377,8 +384,8 @@
    	for (i = 0; i < chipcount; i++)
    		printf(" %s", flashes[i].name);
    	printf("\nPlease specify which chip to use with the -c <chipname> option.\n");
-		programmer_shutdown();
-		exit(1);
+		ret = 1;
+		goto out_shutdown;
    } else if (!chipcount) {
    	printf("No EEPROM/flash device found.\n");
    	if (!force || !chip_to_probe) {
@@ -389,15 +396,14 @@
    		startchip = probe_flash(0, &flashes[0], 1);
    		if (startchip == -1) {
    			printf("Probing for flash chip '%s' failed.\n", chip_to_probe);
-				programmer_shutdown();
-				exit(1);
+				ret = 1;
+				goto out_shutdown;
    		}
    		printf("Please note that forced reads most likely contain garbage.\n");
    		return read_flash_to_file(&flashes[0], filename);
    	}
-		// FIXME: flash writes stay enabled!
-		programmer_shutdown();
-		exit(1);
+		ret = 1;
+		goto out_shutdown;
    }

fill_flash = &flashes[0];
@@ -409,22 +415,19 @@
        (!force)) {
    	fprintf(stderr, "Chip is too big for this programmer "
    		"(-V gives details). Use --force to override.\n");
-		programmer_shutdown();
-		return 1;
+		ret = 1;
+		goto out_shutdown;
    }

if (!(read_it | write_it | verify_it | erase_it)) {
    	printf("No operations were specified.\n");
-		// FIXME: flash writes stay enabled!
-		programmer_shutdown();
-		exit(0);
+		goto out_shutdown;
    }

if (!filename && !erase_it) {
    	printf("Error: No filename specified.\n");
-		// FIXME: flash writes stay enabled!
-		programmer_shutdown();
-		exit(1);
+		ret = 1;
+		goto out_shutdown;
    }

/* Always verify write operations unless -n is used. */
@@ -437,4 +440,8 @@
     */
    programmer_delay(100000);
    return doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it);
+
+out_shutdown:
+	programmer_shutdown();
+	return ret;
 }

[David Hendricks]

On Fri, Jul 15, 2011 at 4:06 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Don't ignore -i if it is specified before -l
> Check if image mentioned by -i is present in layout file
> Clean up cli_classic.c:
> Consolidate duplicated programmer_shutdown calls
> Kill outdated comments
> Finish parameter checking before -L/-z is executed
> This patch was inspired by a cleanup patch by Stefan Tauner.
> I did not fix up the -i/-l ordering dependency because the new layout
> code may have different requirements anyway, and introducing gratuitous
> user interface changes would be a bad idea.
> Side note: This also reduces the amount of code changes needed for the
> logfile patch.
>
> Signed-off-by: Carl-Daniel Hailfinger

Looks good to me.

Acked-by: David Hendricks
> Index: flashrom-cli_classic_cleanup/cli_classic.c
>
> --- flashrom-cli_classic_cleanup/cli_classic.c  (Revision 1372)
> +++ flashrom-cli_classic_cleanup/cli_classic.c  (Arbeitskopie)
> @@ -117,6 +117,7 @@
>  #endif
>        int operation_specified = 0;
>        int i;
>   int ret = 0;
>
>  static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
>  static const struct option long_options[] = {
>
> @@ -232,8 +233,14 @@
>                                cli_classic_abort_usage();
>                        break;
>                case 'i':
>                   /* FIXME: -l has to be specified before -i. */
>                  tempstr = strdup(optarg);
>                   find_romentry(tempstr);
>                   if (find_romentry(tempstr)) {
>                           fprintf(stderr, "Error: image %s not found
>
> in "
>                                   "layout file or -i specified before
>
> "
>                                   "-l\n", tempstr);
>                           cli_classic_abort_usage();
>                   }
>                  break;
>          case 'L':
>                  if (++operation_specified > 1) {
>
> @@ -313,6 +320,11 @@
>                }
>        }
>   if (optind < argc) {
>           fprintf(stderr, "Error: Extra parameter found.\n");
>           cli_classic_abort_usage();
>   }
>  /* FIXME: Print the actions flashrom will take. */
>
>  if (list_supported) {
>
> @@ -327,11 +339,6 @@
>        }
>  #endif
>   if (optind < argc) {
>           fprintf(stderr, "Error: Extra parameter found.\n");
>           cli_classic_abort_usage();
>   }
>
> #if CONFIG_INTERNAL == 1
>        if ((programmer != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
>                fprintf(stderr, "Error: --mainboard requires the internal "
> @@ -360,8 +367,8 @@
>    if (programmer_init(pparam)) {
>            fprintf(stderr, "Error: Programmer initialization
>
> failed.\n");
>           programmer_shutdown();
>           exit(1);
>           ret = 1;
>           goto out_shutdown;
>  }
>
>  for (i = 0; i < ARRAY_SIZE(flashes); i++) {
>
> @@ -377,8 +384,8 @@
>                for (i = 0; i < chipcount; i++)
>                        printf(" %s", flashes[i].name);
>                printf("\nPlease specify which chip to use with the -c
> <chipname> option.\n");
>           programmer_shutdown();
>           exit(1);
>           ret = 1;
>           goto out_shutdown;
>  } else if (!chipcount) {
>          printf("No EEPROM/flash device found.\n");
>          if (!force || !chip_to_probe) {
>
> @@ -389,15 +396,14 @@
>                        startchip = probe_flash(0, &flashes[0], 1);
>                        if (startchip == -1) {
>                                printf("Probing for flash chip '%s'
> failed.\n", chip_to_probe);
>                           programmer_shutdown();
>                           exit(1);
>                           ret = 1;
>                           goto out_shutdown;
>                  }
>                  printf("Please note that forced reads most likely
>
> contain garbage.\n");
>                        return read_flash_to_file(&flashes[0], filename);
>                }
>           // FIXME: flash writes stay enabled!
>           programmer_shutdown();
>           exit(1);
>           ret = 1;
>           goto out_shutdown;
>  }
>
>  fill_flash = &flashes[0];
>
> @@ -409,22 +415,19 @@
>            (!force)) {
>                fprintf(stderr, "Chip is too big for this programmer "
>                        "(-V gives details). Use --force to override.\n");
>           programmer_shutdown();
>           return 1;
>           ret = 1;
>           goto out_shutdown;
>  }
>
>  if (!(read_it | write_it | verify_it | erase_it)) {
>          printf("No operations were specified.\n");
>           // FIXME: flash writes stay enabled!
>           programmer_shutdown();
>           exit(0);
>           goto out_shutdown;
>  }
>
>  if (!filename && !erase_it) {
>          printf("Error: No filename specified.\n");
>           // FIXME: flash writes stay enabled!
>           programmer_shutdown();
>           exit(1);
>           ret = 1;
>           goto out_shutdown;
>  }
>
>  /* Always verify write operations unless -n is used. */
>
> @@ -437,4 +440,8 @@
>         */
>        programmer_delay(100000);
>        return doit(fill_flash, force, filename, read_it, write_it,
> erase_it, verify_it);
>
> +out_shutdown:
>   programmer_shutdown();
>   return ret;
>
> }
>
> flashrom mailing list
> [e-mail]
> http://www.flashrom.org/mailman/listinfo/flashrom

[Stefan Tauner]

On Sat, 16 Jul 2011 01:06:40 +0200
Carl-Daniel Hailfinger  wrote:
> 	if (find_romentry(tempstr)) {
> 		fprintf(stderr, "Error: image %s not found in "
> 			"layout file or -i specified before "
> 			"-l\n", tempstr);
> 		cli_classic_abort_usage();
> 	}

find_romentry does return the index of the (first matching) rom entry
(>=0) on success, or -1 on failure:

int find_romentry(char *name)
{
    int i;

if (!romimages)
    	return -1;

msg_ginfo("Looking for "%s"... ", name);

for (i = 0; i < romimages; i++) {
    	if (!strcmp(rom_entries[i].name, name)) {
    		rom_entries[i].included = 1;
    		msg_ginfo("found.\n");
    		return i;
    	}
    }
    msg_ginfo("not found.\n");	// Not found. Error.

return -1;
}

the attached patch fixes this. thanks to Florian Zumbiehl for reporting
this (http://paste.flashrom.org/view.php?id=707).

carldani: you wrote you did not fix the -l/-i ordering problem "because
the new layout code may have different requirements anyway".
the first step to fix this is to extract the needed file and region
names from the command line (to use them after the parsing loop has
finished). this is somewhat independent from whatever we do with the
arguments then. if you agree i would like to fix this now (i did not
look for what is needed exactly yet), if not you can just ack the patch.

[Carl-Daniel Hailfinger]

Am 18.07.2011 14:13 schrieb Stefan Tauner:
> thanks to Florian Zumbiehl for reporting
> this (http://paste.flashrom.org/view.php?id=707).

Can you credit him in the changelog? Thanks!
> carldani: you wrote you did not fix the -l/-i ordering problem "because
> the new layout code may have different requirements anyway".
> the first step to fix this is to extract the needed file and region
> names from the command line (to use them after the parsing loop has
> finished). this is somewhat independent from whatever we do with the
> arguments then. if you agree i would like to fix this now (i did not
> look for what is needed exactly yet), if not you can just ack the patch.

The flashrom-chromium tree has a workaround for that. The issue here is
that there can be an unlimited number of -i parameters, so you have to
either create an array layout_images[] or build a combined string which
needs to be split up afterwards. I think one variant is done by the
chromium tree. That said, my long-term hope is to allow only one -i
parameter which is split like the programmer parameters with ",".

It all boils down to whether we want to provide the best possible user
experience at the cost of an ugly hack or if we want to avoid such a
hack and move to the new layout commandline interface after 0.9.4. With
r1373 and your bugfix on top (which should be committed now regardless
of whether we hack around the -i limitation or not) the silent
corruption is fixed.

Side note: Should we check for each flashrom parameter (except -V) if it
was specified more than once?
> From: Stefan Tauner 
> Date: Mon, 18 Jul 2011 13:55:01 +0200
> Subject: [PATCH] fix a bug breaking layout file handling in r1373
>
> Signed-off-by: Stefan Tauner

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[Stefan Tauner]

On Mon, 18 Jul 2011 23:31:43 +0200
Carl-Daniel Hailfinger  wrote:
> Am 18.07.2011 14:13 schrieb Stefan Tauner:
> > thanks to Florian Zumbiehl for reporting
> > this (http://paste.flashrom.org/view.php?id=707).
>
> Can you credit him in the changelog? Thanks!

sure
> > carldani: you wrote you did not fix the -l/-i ordering problem "because
> > the new layout code may have different requirements anyway".
> > the first step to fix this is to extract the needed file and region
> > names from the command line (to use them after the parsing loop has
> > finished). this is somewhat independent from whatever we do with the
> > arguments then. if you agree i would like to fix this now (i did not
> > look for what is needed exactly yet), if not you can just ack the patch.
>
> The flashrom-chromium tree has a workaround for that. The issue here is
> that there can be an unlimited number of -i parameters, so you have to
> either create an array layout_images[] or build a combined string which
> needs to be split up afterwards. I think one variant is done by the
> chromium tree. That said, my long-term hope is to allow only one -i
> parameter which is split like the programmer parameters with ",".
>
> It all boils down to whether we want to provide the best possible user
> experience at the cost of an ugly hack or if we want to avoid such a
> hack and move to the new layout commandline interface after 0.9.4. With
> r1373 and your bugfix on top (which should be committed now regardless
> of whether we hack around the -i limitation or not) the silent
> corruption is fixed.

i think multiple -i switches are very nice for the users and us.
creating a growing array by reallocing a new slot at each -i occurrence
does not sound so bad too me. parsing a long -i string OTOH isn't so
nice, although it is matching current -p behavior better.
> Side note: Should we check for each flashrom parameter (except -V) if it
> was specified more than once?

probably. although it is an unnecessary limitation in the case it is
clear to do anyway (e.g. -ww is not really a problem). OTOH it
indicates the use either does not know what he is doing or he does but
does not care much. :)
> > Signed-off-by: Stefan Tauner
>
> Acked-by: Carl-Daniel Hailfinger

thanks, committed in r1374