[flashrom] [PATCH v2] Clean up action parameter handling
Nate Case
ncase at xes-inc.com
Thu Jul 31 17:54:04 CEST 2014
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 at 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);
More information about the flashrom
mailing list