Condense 'read_it', 'write_it', 'erase_it', 'verify_it', and 'force' into a single 'action_opts' struct that's available via flashctx. This makes the function signatures much more sane, while also making it much easier to add new features/options in the future.
Signed-off-by: Nate Case ncase@xes-inc.com --- cli_classic.c | 39 ++++++++++++++++++++++----------------- flash.h | 11 ++++++++++- flashrom.c | 47 ++++++++++++++++++++++------------------------- 3 files changed, 54 insertions(+), 43 deletions(-)
v2 changes: Use bitfield struct in flashctx instead of integer flag
Index: flash.h =================================================================== --- flash.h (revision 1832) +++ flash.h (working copy) @@ -207,12 +207,21 @@ enum write_granularity gran; };
+struct action_opts { + unsigned do_read:1; + unsigned do_write:1; + unsigned do_erase:1; + unsigned do_verify:1; + unsigned force:1; +}; + struct flashctx { struct flashchip *chip; chipaddr virtual_memory; /* Some flash devices have an additional register space. */ chipaddr virtual_registers; struct registered_master *mst; + struct action_opts *action; };
/* Timing used in probe routines. ZERO is -2 to differentiate between an unset @@ -268,7 +277,7 @@ void print_banner(void); void list_programmers_linebreak(int startcol, int cols, int paren); int selfcheck(void); -int doit(struct flashctx *flash, int force, const char *filename, int read_it, int write_it, int erase_it, int verify_it); +int doit(struct flashctx *flash, const char *filename); int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename); int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename);
Index: cli_classic.c =================================================================== --- cli_classic.c (revision 1832) +++ cli_classic.c (working copy) @@ -98,11 +98,11 @@ struct flashctx *fill_flash; const char *name; int namelen, opt, i, j; - int startchip = -1, chipcount = 0, option_index = 0, force = 0; + int startchip = -1, chipcount = 0, option_index = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; #endif - int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; + struct action_opts action = {0}; int dont_verify_it = 0, list_supported = 0, operation_specified = 0; enum programmer prog = PROGRAMMER_INVALID; int ret = 0; @@ -142,6 +142,9 @@ if (selfcheck()) exit(1);
+ for (i = 0; i < ARRAY_SIZE(flashes); i++) + flashes[i].action = &action; + setbuf(stdout, NULL); /* FIXME: Delay all operation_specified checks until after command * line parsing to allow --help overriding everything else. @@ -156,7 +159,7 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - read_it = 1; + action.do_read = 1; break; case 'w': if (++operation_specified > 1) { @@ -165,7 +168,7 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - write_it = 1; + action.do_write = 1; break; case 'v': //FIXME: gracefully handle superfluous -v @@ -179,10 +182,10 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - verify_it = 1; + action.do_verify = 1; break; case 'n': - if (verify_it) { + if (action.do_verify) { fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); cli_classic_abort_usage(); } @@ -202,10 +205,10 @@ "specified. Aborting.\n"); cli_classic_abort_usage(); } - erase_it = 1; + action.do_erase = 1; break; case 'f': - force = 1; + action.force = 1; break; case 'l': if (layoutfile) { @@ -327,7 +330,8 @@ cli_classic_abort_usage(); }
- if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { + if ((action.do_read || action.do_write || action.do_verify) && + check_filename(filename, "image")) { cli_classic_abort_usage(); } if (layoutfile && check_filename(layoutfile, "layout")) { @@ -369,7 +373,7 @@ ret = 1; goto out; } - if (layoutfile != NULL && !write_it) { + if (layoutfile != NULL && !action.do_write) { msg_gerr("Layout files are currently supported for write operations only.\n"); ret = 1; goto out; @@ -447,11 +451,11 @@ goto out_shutdown; } else if (!chipcount) { msg_cinfo("No EEPROM/flash device found.\n"); - if (!force || !chip_to_probe) { + if (!action.force || !chip_to_probe) { msg_cinfo("Note: flashrom can never write if the flash chip isn't found " "automatically.\n"); } - if (force && read_it && chip_to_probe) { + if (action.force && action.do_read && chip_to_probe) { struct registered_master *mst; int compatible_masters = 0; msg_cinfo("Force read (-f -r -c) requested, pretending the chip is there:\n"); @@ -502,27 +506,28 @@ check_chip_supported(fill_flash->chip);
size = fill_flash->chip->total_size * 1024; - if (check_max_decode(fill_flash->mst->buses_supported & fill_flash->chip->bustype, size) && (!force)) { + if (check_max_decode(fill_flash->mst->buses_supported & fill_flash->chip->bustype, size) && (!action.force)) { msg_cerr("Chip is too big for this programmer (-V gives details). Use --force to override.\n"); ret = 1; goto out_shutdown; }
- if (!(read_it | write_it | verify_it | erase_it)) { + if (!(action.do_read || action.do_write || action.do_verify || + action.do_erase)) { msg_ginfo("No operations were specified.\n"); goto out_shutdown; }
/* Always verify write operations unless -n is used. */ - if (write_it && !dont_verify_it) - verify_it = 1; + if (action.do_write && !dont_verify_it) + action.do_verify = 1;
/* FIXME: We should issue an unconditional chip reset here. This can be * done once we have a .reset function in struct flashchip. * Give the chip time to settle. */ programmer_delay(100000); - ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + ret |= doit(fill_flash, filename);
out_shutdown: programmer_shutdown(); Index: flashrom.c =================================================================== --- flashrom.c (revision 1832) +++ flashrom.c (working copy) @@ -1850,30 +1850,28 @@ } }
-/* FIXME: This function signature needs to be improved once doit() has a better - * function signature. - */ -int chip_safety_check(const struct flashctx *flash, int force, int read_it, int write_it, int erase_it, - int verify_it) +int chip_safety_check(const struct flashctx *flash) { const struct flashchip *chip = flash->chip; + struct action_opts *opts = flash->action;
- if (!programmer_may_write && (write_it || erase_it)) { + if (!programmer_may_write && (opts->do_write || opts->do_erase)) { 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) + if (!opts->force) return 1; msg_cerr("Continuing anyway.\n"); }
- if (read_it || erase_it || write_it || verify_it) { + if (opts->do_read || opts->do_erase || opts->do_write || + opts->do_verify) { /* Everything needs read. */ if (chip->tested.read == BAD) { msg_cerr("Read is not working on this chip. "); - if (!force) + if (!opts->force) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1883,7 +1881,7 @@ return 1; } } - if (erase_it || write_it) { + if (opts->do_erase || opts->do_write) { /* Write needs erase. */ if (chip->tested.erase == NA) { msg_cerr("Erase is not possible on this chip.\n"); @@ -1891,7 +1889,7 @@ } if (chip->tested.erase == BAD) { msg_cerr("Erase is not working on this chip. "); - if (!force) + if (!opts->force) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1901,14 +1899,14 @@ return 1; } } - if (write_it) { + if (opts->do_write) { if (chip->tested.write == NA) { msg_cerr("Write is not possible on this chip.\n"); return 1; } if (chip->tested.write == BAD) { msg_cerr("Write is not working on this chip. "); - if (!force) + if (!opts->force) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1921,19 +1919,18 @@ 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. +/* + * FIXME: Abysmal code flow */ -int doit(struct flashctx *flash, int force, const char *filename, int read_it, - int write_it, int erase_it, int verify_it) +int doit(struct flashctx *flash, const char *filename) { uint8_t *oldcontents; uint8_t *newcontents; int ret = 0; unsigned long size = flash->chip->total_size * 1024; + struct action_opts *opts = flash->action;
- if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { + if (chip_safety_check(flash)) { msg_cerr("Aborting.\n"); return 1; } @@ -1949,7 +1946,7 @@ if (flash->chip->unlock) flash->chip->unlock(flash);
- if (read_it) { + if (opts->do_read) { return read_flash_to_file(flash, filename); }
@@ -1973,7 +1970,7 @@ * before we can write. */
- if (erase_it) { + if (opts->do_erase) { /* FIXME: Do we really want the scary warning if erase failed? * After all, after erase the chip is either blank or partially * blank or it has the old contents. A blank chip won't boot, @@ -1987,7 +1984,7 @@ goto out; }
- if (write_it || verify_it) { + if (opts->do_write || opts->do_verify) { if (read_buf_from_file(newcontents, size, filename)) { ret = 1; goto out; @@ -2026,7 +2023,7 @@
// ////////////////////////////////////////////////////////////
- if (write_it) { + if (opts->do_write) { if (erase_and_write_flash(flash, oldcontents, newcontents)) { msg_cerr("Uh oh. Erase/write failed. Checking if anything has changed.\n"); msg_cinfo("Reading current flash chip contents... "); @@ -2047,10 +2044,10 @@ }
/* Verify only if we either did not try to write (verify operation) or actually changed something. */ - if (verify_it && (!write_it || !all_skipped)) { + if (opts->do_verify && (!opts->do_write || !all_skipped)) { msg_cinfo("Verifying flash... ");
- if (write_it) { + if (opts->do_write) { /* Work around chips which need some time to calm down. */ programmer_delay(1000*1000); ret = verify_range(flash, newcontents, 0, size);