Re: [flashrom] [PATCH] Fix and clean up cli_classic.c

[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