Condense 'read_it', 'write_it', 'erase_it', 'verify_it', and 'force' into a single 'flags' parameter. This makes the function signatures much more sane, while also making it much easier to add new features/flags in the future.
Signed-off-by: Nate Case ncase@xes-inc.com --- cli_classic.c | 34 +++++++++++++++++----------------- flash.h | 13 ++++++++++++- flashrom.c | 44 +++++++++++++++++++------------------------- 3 files changed, 48 insertions(+), 43 deletions(-)
Index: flashrom.c =================================================================== --- flashrom.c (revision 1832) +++ flashrom.c (working copy) @@ -1850,30 +1850,26 @@ } }
-/* 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, int flags) { const struct flashchip *chip = flash->chip;
- if (!programmer_may_write && (write_it || erase_it)) { + if (!programmer_may_write && (flags & (ACTION_FLAG_WRITE | ACTION_FLAG_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 (!(flags & ACTION_FLAG_FORCE)) return 1; msg_cerr("Continuing anyway.\n"); }
- if (read_it || erase_it || write_it || verify_it) { + if (flags & ACTION_FLAGS_RWEV) { /* Everything needs read. */ if (chip->tested.read == BAD) { msg_cerr("Read is not working on this chip. "); - if (!force) + if (!(flags & ACTION_FLAG_FORCE)) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1883,7 +1879,7 @@ return 1; } } - if (erase_it || write_it) { + if (flags & (ACTION_FLAG_ERASE | ACTION_FLAG_WRITE)) { /* Write needs erase. */ if (chip->tested.erase == NA) { msg_cerr("Erase is not possible on this chip.\n"); @@ -1891,7 +1887,7 @@ } if (chip->tested.erase == BAD) { msg_cerr("Erase is not working on this chip. "); - if (!force) + if (!(flags & ACTION_FLAG_FORCE)) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1901,14 +1897,14 @@ return 1; } } - if (write_it) { + if (flags & ACTION_FLAG_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 (!(flags & ACTION_FLAG_FORCE)) return 1; msg_cerr("Continuing anyway.\n"); } @@ -1921,19 +1917,17 @@ 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, int flags) { uint8_t *oldcontents; uint8_t *newcontents; int ret = 0; unsigned long size = flash->chip->total_size * 1024;
- if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { + if (chip_safety_check(flash, flags)) { msg_cerr("Aborting.\n"); return 1; } @@ -1949,7 +1943,7 @@ if (flash->chip->unlock) flash->chip->unlock(flash);
- if (read_it) { + if (flags & ACTION_FLAG_READ) { return read_flash_to_file(flash, filename); }
@@ -1973,7 +1967,7 @@ * before we can write. */
- if (erase_it) { + if (flags & ACTION_FLAG_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 +1981,7 @@ goto out; }
- if (write_it || verify_it) { + if (flags & (ACTION_FLAG_WRITE | ACTION_FLAG_VERIFY)) { if (read_buf_from_file(newcontents, size, filename)) { ret = 1; goto out; @@ -2026,7 +2020,7 @@
// ////////////////////////////////////////////////////////////
- if (write_it) { + if (flags & ACTION_FLAG_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 +2041,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 ((flags & ACTION_FLAG_VERIFY) && (!(flags & ACTION_FLAG_WRITE) || !all_skipped)) { msg_cinfo("Verifying flash... ");
- if (write_it) { + if (flags & ACTION_FLAG_WRITE) { /* Work around chips which need some time to calm down. */ programmer_delay(1000*1000); ret = verify_range(flash, newcontents, 0, size); 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; + int flags = 0; int dont_verify_it = 0, list_supported = 0, operation_specified = 0; enum programmer prog = PROGRAMMER_INVALID; int ret = 0; @@ -156,7 +156,7 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - read_it = 1; + flags |= ACTION_FLAG_READ; break; case 'w': if (++operation_specified > 1) { @@ -165,7 +165,7 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - write_it = 1; + flags |= ACTION_FLAG_WRITE; break; case 'v': //FIXME: gracefully handle superfluous -v @@ -179,10 +179,10 @@ cli_classic_abort_usage(); } filename = strdup(optarg); - verify_it = 1; + flags |= ACTION_FLAG_VERIFY; break; case 'n': - if (verify_it) { + if (flags & ACTION_FLAG_VERIFY) { fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); cli_classic_abort_usage(); } @@ -202,10 +202,10 @@ "specified. Aborting.\n"); cli_classic_abort_usage(); } - erase_it = 1; + flags |= ACTION_FLAG_ERASE; break; case 'f': - force = 1; + flags |= ACTION_FLAG_FORCE; break; case 'l': if (layoutfile) { @@ -327,7 +327,7 @@ cli_classic_abort_usage(); }
- if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { + if ((flags & ACTION_FLAGS_RWV) && check_filename(filename, "image")) { cli_classic_abort_usage(); } if (layoutfile && check_filename(layoutfile, "layout")) { @@ -369,7 +369,7 @@ ret = 1; goto out; } - if (layoutfile != NULL && !write_it) { + if (layoutfile != NULL && !(flags & ACTION_FLAG_WRITE)) { msg_gerr("Layout files are currently supported for write operations only.\n"); ret = 1; goto out; @@ -447,11 +447,11 @@ goto out_shutdown; } else if (!chipcount) { msg_cinfo("No EEPROM/flash device found.\n"); - if (!force || !chip_to_probe) { + if (!(flags & ACTION_FLAG_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 ((flags & ACTION_FLAG_FORCE) && (flags & ACTION_FLAG_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 +502,27 @@ 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) && !(flags & ACTION_FLAG_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 (!(flags & ACTION_FLAGS_RWEV)) { 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 ((flags & ACTION_FLAG_WRITE) && !dont_verify_it) + flags |= ACTION_FLAG_VERIFY;
/* 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, flags);
out_shutdown: programmer_shutdown(); Index: flash.h =================================================================== --- flash.h (revision 1832) +++ flash.h (working copy) @@ -121,6 +121,17 @@ #define FEATURE_OTP (1 << 8) #define FEATURE_QPI (1 << 9)
+#define ACTION_FLAG_READ (1 << 1) +#define ACTION_FLAG_WRITE (1 << 2) +#define ACTION_FLAG_ERASE (1 << 3) +#define ACTION_FLAG_VERIFY (1 << 4) +#define ACTION_FLAG_FORCE (1 << 5) + +#define ACTION_FLAGS_RWEV (ACTION_FLAG_READ | ACTION_FLAG_WRITE | \ + ACTION_FLAG_ERASE | ACTION_FLAG_VERIFY) +#define ACTION_FLAGS_RWV (ACTION_FLAG_READ | ACTION_FLAG_WRITE | \ + ACTION_FLAG_VERIFY) + enum test_state { OK = 0, NT = 1, /* Not tested */ @@ -268,7 +279,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 flags); 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);