doit() is the monster function we split off from main() when we created cli_classic() and tried to introduce some abstraction. doit() is a poster child of WTFs on an astronomic scale.
Make doit() marginally less bad by factoring out self-contained code. Create read_bug_from_file() and chip_safety_check() functions The new code flow is designed to make partial writes easier to implement and to reduce eye bleeding if someone is looking at the code. Tests appreciated. Signed-off-by: Carl-Daniel Hailfinger <[email protected]> Index: flashrom-doit_refactor/flashrom.c =================================================================== --- flashrom-doit_refactor/flashrom.c (Revision 1211) +++ flashrom-doit_refactor/flashrom.c (Arbeitskopie) @@ -1116,6 +1116,39 @@ return ret; } +int read_buf_from_file(unsigned char *buf, unsigned long size, char *filename) +{ + unsigned long numbytes; + FILE *image; + struct stat image_stat; + + if ((image = fopen(filename, "rb")) == NULL) { + perror(filename); + return 1; + } + if (fstat(fileno(image), &image_stat) != 0) { + perror(filename); + fclose(image); + return 1; + } + if (image_stat.st_size != size) { + msg_gerr("Error: Image size doesn't match\n"); + fclose(image); + return 1; + } + numbytes = fread(buf, 1, size, image); + if (fclose(image)) { + perror(filename); + return 1; + } + if (numbytes != size) { + msg_gerr("Error: Failed to read complete file. Got %ld bytes, " + "wanted %ld!\n", numbytes, size); + return 1; + } + return 0; +} + int write_buf_to_file(unsigned char *buf, unsigned long size, char *filename) { unsigned long numbytes; @@ -1507,123 +1540,109 @@ return cli_classic(argc, argv); } -/* This function signature is horrible. We need to design a better interface, - * but right now it allows us to split off the CLI code. - * Besides that, the function itself is a textbook example of abysmal code flow. +/* FIXME: This function signature needs to be improved once doit() has a better + * function signature. */ -int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it) +int chip_safety_check(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it) { - uint8_t *buf; - unsigned long numbytes; - FILE *image; - int ret = 0; - unsigned long size; - - size = flash->total_size * 1024; - buf = (uint8_t *) calloc(size, sizeof(char)); - if (!programmer_may_write && (write_it || erase_it)) { msg_perr("Write/erase is not working yet on your programmer in " "its current configuration.\n"); /* --force is the wrong approach, but it's the best we can do * until the generic programmer parameter parser is merged. */ - if (!force) { - msg_perr("Aborting.\n"); - programmer_shutdown(); + if (!force) return 1; - } else { - msg_cerr("Continuing anyway.\n"); - } + msg_cerr("Continuing anyway.\n"); } - if (erase_it) { - if (flash->tested & TEST_BAD_ERASE) { - msg_cerr("Erase is not working on this chip. "); - if (!force) { - msg_cerr("Aborting.\n"); - programmer_shutdown(); + if (read_it || erase_it || write_it || verify_it) { + /* Everything needs read. */ + if (flash->tested & TEST_BAD_READ) { + msg_cerr("Read is not working on this chip. "); + if (!force) return 1; - } else { - msg_cerr("Continuing anyway.\n"); - } + msg_cerr("Continuing anyway.\n"); } - if (flash->unlock) - flash->unlock(flash); - - if (erase_flash(flash)) { - emergency_help_message(); - programmer_shutdown(); + if (!flash->read) { + msg_cerr("flashrom has no read function for this " + "flash chip.\n"); return 1; } - } else if (read_it) { - if (flash->unlock) - flash->unlock(flash); - - if (read_flash_to_file(flash, filename)) { - programmer_shutdown(); - return 1; - } - } else if (write_it) { + } + if (erase_it || write_it) { + /* Write needs erase. */ if (flash->tested & TEST_BAD_ERASE) { - msg_cerr("Erase is not working on this chip " - "and erase is needed for write. "); - if (!force) { - msg_cerr("Aborting.\n"); - programmer_shutdown(); + msg_cerr("Erase is not working on this chip. "); + if (!force) return 1; - } else { - msg_cerr("Continuing anyway.\n"); - } + msg_cerr("Continuing anyway.\n"); } + /* FIXME: Check if at least one erase function exists. */ + } + if (write_it) { if (flash->tested & TEST_BAD_WRITE) { msg_cerr("Write is not working on this chip. "); - if (!force) { - msg_cerr("Aborting.\n"); - programmer_shutdown(); + if (!force) return 1; - } else { - msg_cerr("Continuing anyway.\n"); - } + msg_cerr("Continuing anyway.\n"); } if (!flash->write) { - msg_cerr("Error: flashrom has no write function for this flash chip.\n"); - programmer_shutdown(); + msg_cerr("flashrom has no write function for this " + "flash chip.\n"); return 1; } - if (flash->unlock) - flash->unlock(flash); + } + return 0; +} +/* This function signature is horrible. We need to design a better interface, + * but right now it allows us to split off the CLI code. + * Besides that, the function itself is a textbook example of abysmal code flow. + */ +int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it) +{ + uint8_t *buf; + int ret = 0; + unsigned long size; + + if (chip_safety_check(flash, force, filename, read_it, write_it, erase_it, verify_it)) { + msg_cerr("Aborting.\n"); + programmer_shutdown(); + return 1; } - if (write_it || verify_it) { - struct stat image_stat; - if ((image = fopen(filename, "rb")) == NULL) { - perror(filename); + size = flash->total_size * 1024; + buf = (uint8_t *) calloc(size, sizeof(char)); + + /* Given the existence of read locks, we want to unlock for read, + * erase and write. + */ + if (flash->unlock) + flash->unlock(flash); + + if (read_it) { + ret = read_flash_to_file(flash, filename); + programmer_shutdown(); + return ret; + } + if (erase_it) { + if (erase_flash(flash)) { + emergency_help_message(); programmer_shutdown(); - exit(1); + return 1; } - if (fstat(fileno(image), &image_stat) != 0) { - perror(filename); + } + + if (write_it || verify_it) { + if (read_buf_from_file(buf, flash->total_size * 1024, filename)) { programmer_shutdown(); - exit(1); + return 1; } - if (image_stat.st_size != flash->total_size * 1024) { - msg_gerr("Error: Image size doesn't match\n"); - programmer_shutdown(); - exit(1); - } - numbytes = fread(buf, 1, size, image); #if CONFIG_INTERNAL == 1 show_id(buf, size, force); #endif - fclose(image); - if (numbytes != size) { - msg_gerr("Error: Failed to read file. Got %ld bytes, wanted %ld!\n", numbytes, size); - programmer_shutdown(); - return 1; - } } // This should be moved into each flash part's code to do it -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
