[flashrom] [PATCH] Clean up action parameter handling
Nate Case
ncase at xes-inc.com
Wed Jul 30 16:01:43 CEST 2014
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 at 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);
More information about the flashrom
mailing list