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

[David Hendricks]

On Fri, Jul 15, 2011 at 4:06 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> 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 <c-d.hailfinger.devel.2006 at gmx.net>
>

Looks good to me.

Acked-by: David Hendricks <dhendrix at google.com>


>
> 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;
>  }
>
>
> --
> http://www.hailfinger.org/
>
>
> _______________________________________________
> flashrom mailing list
> flashrom at flashrom.org
> http://www.flashrom.org/mailman/listinfo/flashrom
>



-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110715/9b23105e/attachment.html>