[flashrom] [PATCH 2/2] convert general print messages to msg_g*
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Tue Mar 30 08:52:36 CEST 2010
On 27.03.2010 21:12, Sean Nelson wrote:
> cli_classic.c | 76 +++++++++++++++---------------
> flash.h | 1 -
> flashrom.c | 144 ++++++++++++++++++++++++++++----------------------------
> layout.c | 30 ++++++------
> print.c | 74 +++++++++++++++---------------
> print_wiki.c | 52 ++++++++++----------
> 6 files changed, 188 insertions(+), 189 deletions(-)
>
First things first.
I believe that this patch is really hard to review (not your fault)
because print_wiki and frontend stuff are special from a design perspective.
Given that the foremost goal of msg_* is to allow for libflashrom, a
conversion of cli_classic.c doesn't seem necessary. cli_classic will
always be a commandline frontend (as the name indicates) and thus we
don't need to use any msg_* indirection. OTOH, using msg_* everywhere
would make the codebase more uniform.
Some parts of layout.c are just wrong (the original code, not your
patch). I mean, this code would probably be rejected outright in a
review today. Besides that, modern coreboot images might not even
conform to these old imagetype checks anyway.
print.c doesn't make that much sense for libflashrom (or does it?)
print_wiki.c... I'd argue that this should never end up in libflashrom
due to size issues (and it's not an end user command anyway). That also
means a conversion to msg_* is a bit questionable.
> diff --git a/cli_classic.c b/cli_classic.c
> index 6cb0578..e99fa7c 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -27,79 +27,79 @@
> #include <string.h>
> #include <stdlib.h>
> #include <getopt.h>
> #include "flash.h"
> #include "flashchips.h"
>
> void cli_classic_usage(const char *name)
> {
> const char *pname;
> int pnamelen;
> int remaining = 0;
> enum programmer p;
>
> - printf("Usage: %s [-VfLzhR] [-E|-r file|-w file|-v file] [-c chipname]\n"
> + msg_ginfo("Usage: %s [-VfLzhR] [-E|-r file|-w file|-v file] [-c chipname]\n"
> " [-m [vendor:]part] [-l file] [-i image] [-p programmer]\n\n", name);
>
> - printf("Please note that the command line interface for flashrom will "
> + msg_ginfo("Please note that the command line interface for flashrom will "
> "change before\nflashrom 1.0. Do not use flashrom in scripts "
> "or other automated tools without\nchecking that your flashrom"
> " version won't interpret options in a different way.\n\n");
>
> - printf
> + msg_ginfo
> (" -r | --read: read flash and save into file\n"
> " -w | --write: write file into flash\n"
> " -v | --verify: verify flash against file\n"
> " -n | --noverify: don't verify flash against file\n"
> " -E | --erase: erase flash device\n"
> " -V | --verbose: more verbose output\n"
> " -c | --chip <chipname>: probe only for specified flash chip\n"
> #if INTERNAL_SUPPORT == 1
> " -m | --mainboard <[vendor:]part>: override mainboard settings\n"
> #endif
> " -f | --force: force write without checking image\n"
> " -l | --layout <file.layout>: read ROM layout from file\n"
> " -i | --image <name>: only flash image name from flash layout\n"
> " -L | --list-supported: print supported devices\n"
> #if PRINT_WIKI_SUPPORT == 1
> " -z | --list-supported-wiki: print supported devices in wiki syntax\n"
> #endif
> " -p | --programmer <name>: specify the programmer device");
>
> for (p = 0; p < PROGRAMMER_INVALID; p++) {
> pname = programmer_table[p].name;
> pnamelen = strlen(pname);
> if (remaining - pnamelen - 2 < 0) {
> - printf("\n ");
> + msg_ginfo("\n ");
> remaining = 43;
> } else {
> - printf(" ");
> + msg_ginfo(" ");
> remaining--;
> }
> if (p == 0) {
> - printf("(");
> + msg_ginfo("(");
> remaining--;
> }
> - printf("%s", pname);
> + msg_ginfo("%s", pname);
> remaining -= pnamelen;
> if (p < PROGRAMMER_INVALID - 1) {
> - printf(",");
> + msg_ginfo(",");
> remaining--;
> } else {
> - printf(")\n");
> + msg_ginfo(")\n");
> }
> }
>
> - printf(
> + msg_ginfo(
> " -h | --help: print this help text\n"
> " -R | --version: print the version (release)\n"
> "\nYou can specify one of -E, -r, -w, -v or no operation. If no operation is\n"
> "specified, then all that happens is that flash info is dumped.\n\n");
> exit(1);
> }
>
> int cli_classic(int argc, char *argv[])
> {
> unsigned long size;
> /* Probe for up to three flash chips. */
> struct flashchip *flash, *flashes[3];
> const char *name;
> @@ -140,85 +140,85 @@ int cli_classic(int argc, char *argv[])
> {"help", 0, 0, 'h'},
> {"version", 0, 0, 'R'},
> {0, 0, 0, 0}
> };
>
> char *filename = NULL;
>
> char *tempstr = NULL;
>
> print_version();
>
> if (argc > 1) {
> /* Yes, print them. */
> - printf_debug("The arguments are:\n");
> + msg_gdbg("The arguments are:\n");
> for (i = 1; i < argc; ++i)
> - printf_debug("%s\n", argv[i]);
> + msg_gdbg("%s\n", argv[i]);
> }
>
> if (selfcheck())
> exit(1);
>
> setbuf(stdout, NULL);
> while ((opt = getopt_long(argc, argv, optstring,
> long_options, &option_index)) != EOF) {
> switch (opt) {
> case 'r':
> if (++operation_specified > 1) {
> - fprintf(stderr, "More than one operation "
> + msg_ginfo("More than one operation "
>
msg_gerr because it is an error and we even abort.
> "specified. Aborting.\n");
> exit(1);
> }
> read_it = 1;
> break;
> case 'w':
> if (++operation_specified > 1) {
> - fprintf(stderr, "More than one operation "
> + msg_ginfo("More than one operation "
>
dito
> "specified. Aborting.\n");
> exit(1);
> }
> write_it = 1;
> break;
> case 'v':
> //FIXME: gracefully handle superfluous -v
> if (++operation_specified > 1) {
> - fprintf(stderr, "More than one operation "
> + msg_ginfo("More than one operation "
>
dito
> "specified. Aborting.\n");
> exit(1);
> }
> if (dont_verify_it) {
> - fprintf(stderr, "--verify and --noverify are"
> + msg_ginfo("--verify and --noverify are"
>
dito
> "mutually exclusive. Aborting.\n");
> exit(1);
> }
> verify_it = 1;
> break;
> case 'n':
> if (verify_it) {
> - fprintf(stderr, "--verify and --noverify are"
> + msg_ginfo("--verify and --noverify are"
>
dito
> "mutually exclusive. Aborting.\n");
> exit(1);
> }
> dont_verify_it = 1;
> break;
> case 'c':
> chip_to_probe = strdup(optarg);
> break;
> case 'V':
> verbose++;
> break;
> case 'E':
> if (++operation_specified > 1) {
> - fprintf(stderr, "More than one operation "
> + msg_ginfo("More than one operation "
>
dito
> "specified. Aborting.\n");
> exit(1);
> }
> erase_it = 1;
> break;
> #if INTERNAL_SUPPORT == 1
> case 'm':
> tempstr = strdup(optarg);
> lb_vendor_dev_from_string(tempstr);
> break;
> #endif
> case 'f':
> force = 1;
> @@ -253,27 +253,27 @@ int cli_classic(int argc, char *argv[])
> break;
> default:
> /* The continue refers to the
> * for loop. It is here to be
> * able to differentiate between
> * foo and foobar.
> */
> continue;
> }
> break;
> }
> }
> if (programmer == PROGRAMMER_INVALID) {
> - printf("Error: Unknown programmer %s.\n", optarg);
> + msg_ginfo("Error: Unknown programmer %s.\n", optarg);
>
dito
> exit(1);
> }
> break;
> case 'R':
> /* print_version() is always called during startup. */
> exit(0);
> break;
> case 'h':
> default:
> cli_classic_usage(argv[0]);
> break;
> }
> }
> @@ -281,106 +281,106 @@ int cli_classic(int argc, char *argv[])
> if (list_supported) {
> print_supported();
> exit(0);
> }
>
> #if PRINT_WIKI_SUPPORT == 1
> if (list_supported_wiki) {
> print_supported_wiki();
> exit(0);
> }
> #endif
>
> if (read_it && write_it) {
> - printf("Error: -r and -w are mutually exclusive.\n");
> + msg_ginfo("Error: -r and -w are mutually exclusive.\n");
>
_gerr
> cli_classic_usage(argv[0]);
> }
>
> if (optind < argc)
> filename = argv[optind++];
>
> if (optind < argc) {
> - printf("Error: Extra parameter found.\n");
> + msg_ginfo("Error: Extra parameter found.\n");
>
_gerr
> cli_classic_usage(argv[0]);
> }
>
> if (programmer_init()) {
> - fprintf(stderr, "Error: Programmer initialization failed.\n");
> + msg_ginfo("Error: Programmer initialization failed.\n");
>
_gerr
> exit(1);
> }
>
> // FIXME: Delay calibration should happen in programmer code.
> myusec_calibrate_delay();
>
> for (i = 0; i < ARRAY_SIZE(flashes); i++) {
> flashes[i] =
> probe_flash(i ? flashes[i - 1] + 1 : flashchips, 0);
> if (!flashes[i])
> for (i++; i < ARRAY_SIZE(flashes); i++)
> flashes[i] = NULL;
> }
>
> if (flashes[1]) {
> - printf("Multiple flash chips were detected:");
> + msg_ginfo("Multiple flash chips were detected:");
> for (i = 0; i < ARRAY_SIZE(flashes) && flashes[i]; i++)
> - printf(" %s", flashes[i]->name);
> - printf("\nPlease specify which chip to use with the -c <chipname> option.\n");
> + msg_ginfo(" %s", flashes[i]->name);
> + msg_ginfo("\nPlease specify which chip to use with the -c <chipname> option.\n");
> programmer_shutdown();
> exit(1);
> } else if (!flashes[0]) {
> - printf("No EEPROM/flash device found.\n");
> + msg_ginfo("No EEPROM/flash device found.\n");
> if (!force || !chip_to_probe) {
> - printf("If you know which flash chip you have, and if this version of flashrom\n");
> - printf("supports a similar flash chip, you can try to force read your chip. Run:\n");
> - printf("flashrom -f -r -c similar_supported_flash_chip filename\n");
> - printf("\n");
> - printf("Note: flashrom can never write when the flash chip isn't found automatically.\n");
> + msg_ginfo("If you know which flash chip you have, and if this version of flashrom\n");
> + msg_ginfo("supports a similar flash chip, you can try to force read your chip. Run:\n");
> + msg_ginfo("flashrom -f -r -c similar_supported_flash_chip filename\n");
>
I think we should kill this message completely because it creates
expectations that write should work. Yes, we tell people that write
can't work in that case, but we had too many users who assumed that a
non-working write for non-detected chips was due to a flashrom bug.
Besides that, this advice is misleading and dangerous for SPI chips.
Overall, it causes more hassle than it helps.
> + msg_ginfo("\n");
> + msg_ginfo("Note: flashrom can never write when the flash chip isn't found automatically.\n");
>
s/when/if/ unless I'm mistaken.
> }
> if (force && read_it && chip_to_probe) {
> - printf("Force read (-f -r -c) requested, forcing chip probe success:\n");
> + msg_ginfo("Force read (-f -r -c) requested, forcing chip probe success:\n");
> flashes[0] = probe_flash(flashchips, 1);
> if (!flashes[0]) {
> - printf("flashrom does not support a flash chip named '%s'.\n", chip_to_probe);
> - printf("Run flashrom -L to view the hardware supported in this flashrom version.\n");
> + msg_ginfo("flashrom does not support a flash chip named '%s'.\n", chip_to_probe);
> + msg_ginfo("Run flashrom -L to view the hardware supported in this flashrom version.\n");
> exit(1);
> }
> - printf("Please note that forced reads most likely contain garbage.\n");
> + msg_ginfo("Please note that forced reads most likely contain garbage.\n");
> return read_flash(flashes[0], filename);
> }
>
Should we add something like this:
if (force && chip_to_probe && !read_it) {
/* Circumventing this protection would be stupid, and every single user
* who tried it got corrupted flash. We should be handing out Darwin Awards
* to such users instead of helping them.
*/
msg_gerr("flashrom can never write if the flash chip isn't found automatically.\n");
msg_gerr("Trying to cirumvent this protection will cause flash corruption.\n");
msg_gdbg("You can fix this by adding support for your flash chip, chipset, mainboard or programmer. Or ask for help at flashrom at flashrom.org\n");
}
> // FIXME: flash writes stay enabled!
> programmer_shutdown();
> exit(1);
> }
>
> flash = flashes[0];
>
> check_chip_supported(flash);
>
> size = flash->total_size * 1024;
> if (check_max_decode((buses_supported & flash->bustype), size) &&
> (!force)) {
> - fprintf(stderr, "Chip is too big for this programmer "
> + msg_ginfo("Chip is too big for this programmer "
>
_gerr
> "(-V gives details). Use --force to override.\n");
> programmer_shutdown();
> return 1;
> }
>
> if (!(read_it | write_it | verify_it | erase_it)) {
> - printf("No operations were specified.\n");
> + msg_ginfo("No operations were specified.\n");
> // FIXME: flash writes stay enabled!
> programmer_shutdown();
> exit(1);
>
Ouch. This should be
exit(0);
because everything is OK (the user only wanted probe).
> }
>
> if (!filename && !erase_it) {
> - printf("Error: No filename specified.\n");
> + msg_ginfo("Error: No filename specified.\n");
>
_gerr
> // FIXME: flash writes stay enabled!
> programmer_shutdown();
> exit(1);
> }
>
> /* Always verify write operations unless -n is used. */
> if (write_it && !dont_verify_it)
> verify_it = 1;
>
> return doit(flash, force, filename, read_it, write_it, erase_it, verify_it);
> }
> diff --git a/flash.h b/flash.h
> index aee95a3..3d0d114 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -521,27 +521,26 @@ enum write_granularity {
> extern enum chipbustype buses_supported;
> struct decode_sizes {
> uint32_t parallel;
> uint32_t lpc;
> uint32_t fwh;
> uint32_t spi;
> };
> extern struct decode_sizes max_rom_decode;
> extern char *programmer_param;
> extern unsigned long flashbase;
> extern int verbose;
> extern const char *flashrom_version;
> extern char *chip_to_probe;
> -#define printf_debug(x...) { if (verbose) printf(x); }
>
Heh, right. If we decide not to convert all printf_debug statements,
this may have to stay.
> void map_flash_registers(struct flashchip *flash);
> int read_memmapped(struct flashchip *flash, uint8_t *buf, int start, int len);
> int erase_flash(struct flashchip *flash);
> struct flashchip *probe_flash(struct flashchip *first_flash, int force);
> int read_flash(struct flashchip *flash, char *filename);
> void check_chip_supported(struct flashchip *flash);
> int check_max_decode(enum chipbustype buses, uint32_t size);
> int min(int a, int b);
> int max(int a, int b);
> char *extract_param(char **haystack, char *needle, char *delim);
> int check_erased_range(struct flashchip *flash, int start, int len);
> int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message);
> int need_erase(uint8_t *have, uint8_t *want, int len, enum write_granularity gran);
> diff --git a/flashrom.c b/flashrom.c
> index 2450dee..eb52661 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -341,27 +341,27 @@ struct shutdown_func_data {
> } shutdown_fn[SHUTDOWN_MAXFN];
>
> /* Register a function to be executed on programmer shutdown.
> * The advantage over atexit() is that you can supply a void pointer which will
> * be used as parameter to the registered function upon programmer shutdown.
> * This pointer can point to arbitrary data used by said function, e.g. undo
> * information for GPIO settings etc. If unneeded, set data=NULL.
> * Please note that the first (void *data) belongs to the function signature of
> * the function passed as first parameter.
> */
> int register_shutdown(void (*function) (void *data), void *data)
> {
> if (shutdown_fn_count >= SHUTDOWN_MAXFN) {
> - msg_perr("Tried to register more than %n shutdown functions.\n",
> + msg_gerr("Tried to register more than %n shutdown functions.\n",
>
Not sure. Usually only programmers register shutdown functions, so this
would be more of a programmer-related message.
> SHUTDOWN_MAXFN);
> return 1;
> }
> shutdown_fn[shutdown_fn_count].func = function;
> shutdown_fn[shutdown_fn_count].data = data;
> shutdown_fn_count++;
>
> return 0;
> }
>
> int programmer_init(void)
> {
> return programmer_table[programmer].init();
> @@ -500,56 +500,56 @@ char *extract_param(char **haystack, char *needle, char *delim)
> if (strchr(delim, *(param_pos - 1)))
> break;
> /* Continue searching. */
> param_pos++;
> param_pos = strstr(param_pos, needle);
> } while (1);
>
> if (param_pos) {
> param_pos += strlen(needle);
> devlen = strcspn(param_pos, delim);
> if (devlen) {
> dev = malloc(devlen + 1);
> if (!dev) {
> - fprintf(stderr, "Out of memory!\n");
> + msg_gerr("Out of memory!\n");
> exit(1);
> }
> strncpy(dev, param_pos, devlen);
> dev[devlen] = '\0';
> }
> rest = param_pos + devlen;
> rest += strspn(rest, delim);
> param_pos -= strlen(needle);
> memmove(param_pos, rest, strlen(rest) + 1);
> tmp = realloc(*haystack, strlen(*haystack) + 1);
> if (!tmp) {
> - fprintf(stderr, "Out of memory!\n");
> + msg_gerr("Out of memory!\n");
> exit(1);
> }
> *haystack = tmp;
> }
>
>
> return dev;
> }
>
> /* start is an offset to the base address of the flash chip */
> int check_erased_range(struct flashchip *flash, int start, int len)
> {
> int ret;
> uint8_t *cmpbuf = malloc(len);
>
> if (!cmpbuf) {
> - fprintf(stderr, "Could not allocate memory!\n");
> + msg_gerr("Could not allocate memory!\n");
> exit(1);
> }
> memset(cmpbuf, 0xff, len);
> ret = verify_range(flash, cmpbuf, start, len, "ERASE");
> free(cmpbuf);
> return ret;
> }
>
> /**
> * @cmpbuf buffer to compare against, cmpbuf[0] is expected to match the
> flash content at location start
> * @start offset to the base address of the flash chip
> * @len length of the verified area
> @@ -557,36 +557,36 @@ int check_erased_range(struct flashchip *flash, int start, int len)
> * @return 0 for success, -1 for failure
> */
> int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message)
> {
> int i, j, starthere, lenhere, ret = 0;
> int page_size = flash->page_size;
> uint8_t *readbuf = malloc(page_size);
> int failcount = 0;
>
> if (!len)
> goto out_free;
>
> if (!flash->read) {
> - fprintf(stderr, "ERROR: flashrom has no read function for this flash chip.\n");
> + msg_gerr("ERROR: flashrom has no read function for this flash chip.\n");
> return 1;
> }
> if (!readbuf) {
> - fprintf(stderr, "Could not allocate memory!\n");
> + msg_gerr("Could not allocate memory!\n");
> exit(1);
> }
>
> if (start + len > flash->total_size * 1024) {
> - fprintf(stderr, "Error: %s called with start 0x%x + len 0x%x >"
> + msg_gerr("Error: %s called with start 0x%x + len 0x%x >"
> " total_size 0x%x\n", __func__, start, len,
> flash->total_size * 1024);
> ret = -1;
> goto out_free;
> }
> if (!message)
> message = "VERIFY";
>
> /* Warning: This loop has a very unusual condition and body.
> * The loop needs to go through each page with at least one affected
> * byte. The lowest page number is (start / page_size) since that
> * division rounds down. The highest page number we want is the page
> * where the last byte of the range lives. That last byte has the
> @@ -594,36 +594,36 @@ int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, c
> * (start + len - 1) / page_size. Since we want to include that last
> * page as well, the loop condition uses <=.
> */
> for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
> /* Byte position of the first byte in the range in this page. */
> starthere = max(start, i * page_size);
> /* Length of bytes in the range in this page. */
> lenhere = min(start + len, (i + 1) * page_size) - starthere;
> flash->read(flash, readbuf, starthere, lenhere);
> for (j = 0; j < lenhere; j++) {
> if (cmpbuf[starthere - start + j] != readbuf[j]) {
> /* Only print the first failure. */
> if (!failcount++)
> - fprintf(stderr, "%s FAILED at 0x%08x! "
> + msg_gerr("%s FAILED at 0x%08x! "
>
This one is difficult. It tells us that chip writing went wrong, but the
reason may be the programmer or the chip.
IMHO _cerr, but I can see the point of _gerr.
> "Expected=0x%02x, Read=0x%02x,",
> message, starthere + j,
> cmpbuf[starthere - start + j],
> readbuf[j]);
> }
> }
> }
> if (failcount) {
> - fprintf(stderr, " failed byte count from 0x%08x-0x%08x: 0x%x\n",
> + msg_gerr(" failed byte count from 0x%08x-0x%08x: 0x%x\n",
>
Difficult as well.
> start, start + len - 1, failcount);
> ret = -1;
> }
>
> out_free:
> free(readbuf);
> return ret;
> }
>
> /**
> * Check if the buffer @have can be programmed to the content of @want without
> * erasing. This is only possible if all chunks of size @gran are either kept
> * as-is or changed from an all-ones state to any other state.
> @@ -728,27 +728,27 @@ int need_erase(uint8_t *have, uint8_t *want, int len, enum write_granularity gra
> * Patterns 10-11 are special purpose for detecting subblock aliasing with
> * block sizes >256 bytes (some Dataflash chips etc.)
> * AND Pattern 8/9 == Pattern 12
> * AND Pattern 10/11 == Pattern 12
> * Pattern 13 is the completely erased state.
> * None of the patterns can detect aliasing at boundaries which are a multiple
> * of 16 MBytes (but such chips do not exist anyway for Parallel/LPC/FWH/SPI).
> */
> int generate_testpattern(uint8_t *buf, uint32_t size, int variant)
> {
> int i;
>
> if (!buf) {
> - fprintf(stderr, "Invalid buffer!\n");
> + msg_gerr("Invalid buffer!\n");
> return 1;
> }
>
> switch (variant) {
> case 0:
> for (i = 0; i < size; i++)
> buf[i] = (i & 0xf) << 4 | 0x5;
> break;
> case 1:
> for (i = 0; i < size; i++)
> buf[i] = (i & 0xf) << 4 | 0xa;
> break;
> case 2:
> @@ -816,64 +816,64 @@ int generate_testpattern(uint8_t *buf, uint32_t size, int variant)
> buf[i * 256 + 255] = i & 0xff;
> }
> }
>
> return 0;
> }
>
> int check_max_decode(enum chipbustype buses, uint32_t size)
> {
> int limitexceeded = 0;
> if ((buses & CHIP_BUSTYPE_PARALLEL) &&
> (max_rom_decode.parallel < size)) {
> limitexceeded++;
> - printf_debug("Chip size %u kB is bigger than supported "
> + msg_gdbg("Chip size %u kB is bigger than supported "
> "size %u kB of chipset/board/programmer "
> "for %s interface, "
> "probe/read/erase/write may fail. ", size / 1024,
> max_rom_decode.parallel / 1024, "Parallel");
> }
> if ((buses & CHIP_BUSTYPE_LPC) && (max_rom_decode.lpc < size)) {
> limitexceeded++;
> - printf_debug("Chip size %u kB is bigger than supported "
> + msg_gdbg("Chip size %u kB is bigger than supported "
> "size %u kB of chipset/board/programmer "
> "for %s interface, "
> "probe/read/erase/write may fail. ", size / 1024,
> max_rom_decode.lpc / 1024, "LPC");
> }
> if ((buses & CHIP_BUSTYPE_FWH) && (max_rom_decode.fwh < size)) {
> limitexceeded++;
> - printf_debug("Chip size %u kB is bigger than supported "
> + msg_gdbg("Chip size %u kB is bigger than supported "
> "size %u kB of chipset/board/programmer "
> "for %s interface, "
> "probe/read/erase/write may fail. ", size / 1024,
> max_rom_decode.fwh / 1024, "FWH");
> }
> if ((buses & CHIP_BUSTYPE_SPI) && (max_rom_decode.spi < size)) {
> limitexceeded++;
> - printf_debug("Chip size %u kB is bigger than supported "
> + msg_gdbg("Chip size %u kB is bigger than supported "
> "size %u kB of chipset/board/programmer "
> "for %s interface, "
> "probe/read/erase/write may fail. ", size / 1024,
> max_rom_decode.spi / 1024, "SPI");
> }
> if (!limitexceeded)
> return 0;
> /* Sometimes chip and programmer have more than one bus in common,
> * and the limit is not exceeded on all buses. Tell the user.
> */
> if (bitcount(buses) > limitexceeded)
> /* FIXME: This message is designed towards CLI users. */
> - printf_debug("There is at least one common chip/programmer "
> + msg_gdbg("There is at least one common chip/programmer "
> "interface which can support a chip of this size. "
> "You can try --force at your own risk.\n");
> return 1;
> }
>
> struct flashchip *probe_flash(struct flashchip *first_flash, int force)
> {
> struct flashchip *flash;
> unsigned long base = 0;
> uint32_t size;
> enum chipbustype buses_common;
> char *tmp;
>
> @@ -914,78 +914,78 @@ struct flashchip *probe_flash(struct flashchip *first_flash, int force)
> goto notfound;
>
> if (first_flash == flashchips
> || flash->model_id != GENERIC_DEVICE_ID)
> break;
>
> notfound:
> programmer_unmap_flash_region((void *)flash->virtual_memory, size);
> }
>
> if (!flash || !flash->name)
> return NULL;
>
> - printf("Found chip \"%s %s\" (%d KB, %s) at physical address 0x%lx.\n",
> + msg_ginfo("Found chip \"%s %s\" (%d KB, %s) at physical address 0x%lx.\n",
>
_cinfo?
> flash->vendor, flash->name, flash->total_size,
> flashbuses_to_text(flash->bustype), base);
>
> if (flash->printlock)
> flash->printlock(flash);
>
> return flash;
> }
>
> int verify_flash(struct flashchip *flash, uint8_t *buf)
> {
> int ret;
> int total_size = flash->total_size * 1024;
>
> - printf("Verifying flash... ");
> + msg_ginfo("Verifying flash... ");
>
_cinfo
>
> ret = verify_range(flash, buf, 0, total_size, NULL);
>
> if (!ret)
> - printf("VERIFIED. \n");
> + msg_ginfo("VERIFIED. \n");
>
_cinfo
>
> return ret;
> }
>
> int read_flash(struct flashchip *flash, char *filename)
> {
> unsigned long numbytes;
> FILE *image;
> unsigned long size = flash->total_size * 1024;
> unsigned char *buf = calloc(size, sizeof(char));
>
> if (!filename) {
> - printf("Error: No filename specified.\n");
> + msg_ginfo("Error: No filename specified.\n");
>
_gerr
> return 1;
> }
> if ((image = fopen(filename, "wb")) == NULL) {
> perror(filename);
> exit(1);
> }
> - printf("Reading flash... ");
> + msg_ginfo("Reading flash... ");
>
_cinfo?
> if (!flash->read) {
> - printf("FAILED!\n");
> - fprintf(stderr, "ERROR: flashrom has no read function for this flash chip.\n");
> + msg_ginfo("FAILED!\n");
>
_cinfo?
> + msg_gerr("ERROR: flashrom has no read function for this flash chip.\n");
>
_cerr?
> return 1;
> } else
> flash->read(flash, buf, 0, size);
>
> numbytes = fwrite(buf, 1, size, image);
> fclose(image);
> free(buf);
> - printf("%s.\n", numbytes == size ? "done" : "FAILED");
> + msg_ginfo("%s.\n", numbytes == size ? "done" : "FAILED");
>
_cinfo?
> if (numbytes != size)
> return 1;
> return 0;
> }
>
> /* This function shares a lot of its structure with erase_flash().
> * Even if an error is found, the function will keep going and check the rest.
> */
> int selfcheck_eraseblocks(struct flashchip *flash)
> {
> int i, j, k;
> int ret = 0;
>
> @@ -1007,27 +1007,27 @@ int selfcheck_eraseblocks(struct flashchip *flash)
> if (!eraser.eraseblocks[i].count &&
> eraser.eraseblocks[i].size) {
> msg_gerr("ERROR: Flash chip %s erase function "
> "%i region %i has count 0. Please report"
> " a bug at flashrom at flashrom.org\n",
> flash->name, k, i);
> ret = 1;
> }
> done += eraser.eraseblocks[i].count *
> eraser.eraseblocks[i].size;
> }
> /* Empty eraseblock definition with erase function. */
> if (!done && eraser.block_erase)
> - msg_pspew("Strange: Empty eraseblock definition with "
> + msg_gspew("Strange: Empty eraseblock definition with "
> "non-empty erase function. Not an error.\n");
> if (!done)
> continue;
> if (done != flash->total_size * 1024) {
> msg_gerr("ERROR: Flash chip %s erase function %i "
> "region walking resulted in 0x%06x bytes total,"
> " expected 0x%06x bytes. Please report a bug at"
> " flashrom at flashrom.org\n", flash->name, k,
> done, flash->total_size * 1024);
> ret = 1;
> }
> if (!eraser.block_erase)
> continue;
> @@ -1044,106 +1044,106 @@ int selfcheck_eraseblocks(struct flashchip *flash)
> flash->name, k, j);
> ret = 1;
> }
> }
> }
> return ret;
> }
>
> int erase_flash(struct flashchip *flash)
> {
> int i, j, k, ret = 0, found = 0;
> unsigned int start, len;
>
> - printf("Erasing flash chip... ");
> + msg_ginfo("Erasing flash chip... ");
>
_cinfo?
> for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {
> unsigned int done = 0;
> struct block_eraser eraser = flash->block_erasers[k];
>
> - printf_debug("Looking at blockwise erase function %i... ", k);
> + msg_gdbg("Looking at blockwise erase function %i... ", k);
>
_cdbg?
> if (!eraser.block_erase && !eraser.eraseblocks[0].count) {
> - printf_debug("not defined. "
> + msg_gdbg("not defined. "
>
_cdbg?
> "Looking for another erase function.\n");
> continue;
> }
> if (!eraser.block_erase && eraser.eraseblocks[0].count) {
> - printf_debug("eraseblock layout is known, but no "
> + msg_gdbg("eraseblock layout is known, but no "
>
_cdbg?
> "matching block erase function found. "
> "Looking for another erase function.\n");
> continue;
> }
> if (eraser.block_erase && !eraser.eraseblocks[0].count) {
> - printf_debug("block erase function found, but "
> + msg_gdbg("block erase function found, but "
>
_cdbg?
> "eraseblock layout is unknown. "
> "Looking for another erase function.\n");
> continue;
> }
> found = 1;
> - printf_debug("trying... ");
> + msg_gdbg("trying... ");
>
_cdbg?
> for (i = 0; i < NUM_ERASEREGIONS; i++) {
> /* count==0 for all automatically initialized array
> * members so the loop below won't be executed for them.
> */
> for (j = 0; j < eraser.eraseblocks[i].count; j++) {
> start = done + eraser.eraseblocks[i].size * j;
> len = eraser.eraseblocks[i].size;
> - printf_debug("0x%06x-0x%06x, ", start,
> + msg_gdbg("0x%06x-0x%06x, ", start,
>
_cdbg?
> start + len - 1);
> ret = eraser.block_erase(flash, start, len);
> if (ret)
> break;
> }
> if (ret)
> break;
> done += eraser.eraseblocks[i].count *
> eraser.eraseblocks[i].size;
> }
> - printf_debug("\n");
> + msg_gdbg("\n");
>
_cdbg?
> /* If everything is OK, don't try another erase function. */
> if (!ret)
> break;
> }
> if (!found) {
> - fprintf(stderr, "ERROR: flashrom has no erase function for this flash chip.\n");
> + msg_gerr("ERROR: flashrom has no erase function for this flash chip.\n");
>
_cerr?
> return 1;
> }
>
> if (ret) {
> - fprintf(stderr, "FAILED!\n");
> + msg_gerr("FAILED!\n");
>
_cerr?
> } else {
> - printf("SUCCESS.\n");
> + msg_ginfo("SUCCESS.\n");
>
_cinfo?
> }
> return ret;
> }
>
> void emergency_help_message(void)
> {
> - fprintf(stderr, "Your flash chip is in an unknown state.\n"
> + msg_gerr("Your flash chip is in an unknown state.\n"
> "Get help on IRC at irc.freenode.net (channel #flashrom) or\n"
> "mail flashrom at flashrom.org!\n--------------------"
> "-----------------------------------------------------------\n"
> "DO NOT REBOOT OR POWEROFF!\n");
> }
>
> /* The way to go if you want a delimited list of programmers*/
> void list_programmers(char *delim)
> {
> enum programmer p;
> for (p = 0; p < PROGRAMMER_INVALID; p++) {
> - printf("%s", programmer_table[p].name);
> + msg_ginfo("%s", programmer_table[p].name);
> if (p < PROGRAMMER_INVALID - 1)
> - printf("%s", delim);
> + msg_ginfo("%s", delim);
> }
> - printf("\n");
> + msg_ginfo("\n");
> }
>
> void print_sysinfo(void)
> {
> #if HAVE_UTSNAME == 1
> struct utsname osinfo;
> uname(&osinfo);
>
> msg_ginfo(" on %s %s (%s)", osinfo.sysname, osinfo.release,
> osinfo.machine);
> #else
> msg_ginfo(" on unknown machine");
> #endif
> @@ -1162,91 +1162,91 @@ void print_sysinfo(void)
> #ifdef __VERSION__
> msg_ginfo(" %s", __VERSION__);
> #else
> msg_ginfo(" unknown version");
> #endif
> #else
> msg_ginfo(" unknown compiler");
> #endif
> msg_ginfo("\n");
> }
>
> void print_version(void)
> {
> - printf("flashrom v%s", flashrom_version);
> + msg_ginfo("flashrom v%s\n", flashrom_version);
> print_sysinfo();
> }
>
> int selfcheck(void)
> {
> int ret = 0;
> struct flashchip *flash;
>
> /* Safety check. Instead of aborting after the first error, check
> * if more errors exist.
> */
> if (ARRAY_SIZE(programmer_table) - 1 != PROGRAMMER_INVALID) {
> - fprintf(stderr, "Programmer table miscompilation!\n");
> + msg_gerr("Programmer table miscompilation!\n");
> ret = 1;
> }
> if (spi_programmer_count - 1 != SPI_CONTROLLER_INVALID) {
> - fprintf(stderr, "SPI programmer table miscompilation!\n");
> + msg_gerr("SPI programmer table miscompilation!\n");
> ret = 1;
> }
> #if BITBANG_SPI_SUPPORT == 1
> if (bitbang_spi_master_count - 1 != BITBANG_SPI_INVALID) {
> - fprintf(stderr, "Bitbanging SPI master table miscompilation!\n");
> + msg_gerr("Bitbanging SPI master table miscompilation!\n");
> ret = 1;
> }
> #endif
> for (flash = flashchips; flash && flash->name; flash++)
> if (selfcheck_eraseblocks(flash))
> ret = 1;
> return ret;
> }
>
> void check_chip_supported(struct flashchip *flash)
> {
> if (TEST_OK_MASK != (flash->tested & TEST_OK_MASK)) {
> - printf("===\n");
> + msg_ginfo("===\n");
> if (flash->tested & TEST_BAD_MASK) {
> - printf("This flash part has status NOT WORKING for operations:");
> + msg_ginfo("This flash part has status NOT WORKING for operations:");
> if (flash->tested & TEST_BAD_PROBE)
> - printf(" PROBE");
> + msg_ginfo(" PROBE");
> if (flash->tested & TEST_BAD_READ)
> - printf(" READ");
> + msg_ginfo(" READ");
> if (flash->tested & TEST_BAD_ERASE)
> - printf(" ERASE");
> + msg_ginfo(" ERASE");
> if (flash->tested & TEST_BAD_WRITE)
> - printf(" WRITE");
> - printf("\n");
> + msg_ginfo(" WRITE");
> + msg_ginfo("\n");
> }
> if ((!(flash->tested & TEST_BAD_PROBE) && !(flash->tested & TEST_OK_PROBE)) ||
> (!(flash->tested & TEST_BAD_READ) && !(flash->tested & TEST_OK_READ)) ||
> (!(flash->tested & TEST_BAD_ERASE) && !(flash->tested & TEST_OK_ERASE)) ||
> (!(flash->tested & TEST_BAD_WRITE) && !(flash->tested & TEST_OK_WRITE))) {
> - printf("This flash part has status UNTESTED for operations:");
> + msg_ginfo("This flash part has status UNTESTED for operations:");
> if (!(flash->tested & TEST_BAD_PROBE) && !(flash->tested & TEST_OK_PROBE))
> - printf(" PROBE");
> + msg_ginfo(" PROBE");
> if (!(flash->tested & TEST_BAD_READ) && !(flash->tested & TEST_OK_READ))
> - printf(" READ");
> + msg_ginfo(" READ");
> if (!(flash->tested & TEST_BAD_ERASE) && !(flash->tested & TEST_OK_ERASE))
> - printf(" ERASE");
> + msg_ginfo(" ERASE");
> if (!(flash->tested & TEST_BAD_WRITE) && !(flash->tested & TEST_OK_WRITE))
> - printf(" WRITE");
> - printf("\n");
> + msg_ginfo(" WRITE");
> + msg_ginfo("\n");
> }
> /* FIXME: This message is designed towards CLI users. */
> - printf("Please email a report to flashrom at flashrom.org if any "
> + msg_ginfo("Please email a report to flashrom at flashrom.org if any "
> "of the above operations\nwork correctly for you with "
> "this flash part. Please include the flashrom\noutput "
> "with the additional -V option for all operations you "
> "tested (-V, -rV,\n-wV, -EV), and mention which "
> "mainboard or programmer you tested.\nThanks for your "
> "help!\n===\n");
> }
> }
>
All of the above: msg_c* instead of msg_g*?
>
> int main(int argc, char *argv[])
> {
> return cli_classic(argc, argv);
> }
> @@ -1257,127 +1257,127 @@ int main(int argc, char *argv[])
> int doit(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 (erase_it) {
> if (flash->tested & TEST_BAD_ERASE) {
> - fprintf(stderr, "Erase is not working on this chip. ");
> + msg_gerr("Erase is not working on this chip. ");
> if (!force) {
> - fprintf(stderr, "Aborting.\n");
> + msg_gerr("Aborting.\n");
> programmer_shutdown();
> return 1;
> } else {
> - fprintf(stderr, "Continuing anyway.\n");
> + msg_gerr("Continuing anyway.\n");
> }
> }
> if (flash->unlock)
> flash->unlock(flash);
>
> if (erase_flash(flash)) {
> emergency_help_message();
> programmer_shutdown();
> return 1;
> }
> } else if (read_it) {
> if (flash->unlock)
> flash->unlock(flash);
>
> if (read_flash(flash, filename)) {
> programmer_shutdown();
> return 1;
> }
> } else {
> struct stat image_stat;
>
> if (flash->unlock)
> flash->unlock(flash);
>
> if (flash->tested & TEST_BAD_ERASE) {
> - fprintf(stderr, "Erase is not working on this chip "
> + msg_gerr("Erase is not working on this chip "
> "and erase is needed for write. ");
> if (!force) {
> - fprintf(stderr, "Aborting.\n");
> + msg_gerr("Aborting.\n");
> programmer_shutdown();
> return 1;
> } else {
> - fprintf(stderr, "Continuing anyway.\n");
> + msg_gerr("Continuing anyway.\n");
> }
> }
> if (flash->tested & TEST_BAD_WRITE) {
> - fprintf(stderr, "Write is not working on this chip. ");
> + msg_gerr("Write is not working on this chip. ");
> if (!force) {
> - fprintf(stderr, "Aborting.\n");
> + msg_gerr("Aborting.\n");
> programmer_shutdown();
> return 1;
> } else {
> - fprintf(stderr, "Continuing anyway.\n");
> + msg_gerr("Continuing anyway.\n");
>
Same msg_c* instead of msg_g* here.
> }
> }
> if ((image = fopen(filename, "rb")) == NULL) {
> perror(filename);
> programmer_shutdown();
> exit(1);
> }
> if (fstat(fileno(image), &image_stat) != 0) {
> perror(filename);
> programmer_shutdown();
> exit(1);
> }
> if (image_stat.st_size != flash->total_size * 1024) {
> - fprintf(stderr, "Error: Image size doesn't match\n");
> + msg_gerr("Error: Image size doesn't match\n");
>
OK.
> programmer_shutdown();
> exit(1);
> }
>
> numbytes = fread(buf, 1, size, image);
> #if INTERNAL_SUPPORT == 1
> show_id(buf, size, force);
> #endif
> fclose(image);
> if (numbytes != size) {
> - fprintf(stderr, "Error: Failed to read file. Got %ld bytes, wanted %ld!\n", 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
> // cleanly. This does the job.
> handle_romentries(buf, flash);
>
> // ////////////////////////////////////////////////////////////
>
> if (write_it) {
> - printf("Writing flash chip... ");
> + msg_ginfo("Writing flash chip... ");
> if (!flash->write) {
> - fprintf(stderr, "Error: flashrom has no write function for this flash chip.\n");
> + msg_gerr("Error: flashrom has no write function for this flash chip.\n");
>
_cerr?
> programmer_shutdown();
> return 1;
> }
> ret = flash->write(flash, buf);
> if (ret) {
> - fprintf(stderr, "FAILED!\n");
> + msg_gerr("FAILED!\n");
>
_cerr?
> emergency_help_message();
> programmer_shutdown();
> return 1;
> } else {
> - printf("COMPLETE.\n");
> + msg_ginfo("COMPLETE.\n");
>
_cinfo?
> }
> }
>
> if (verify_it) {
> /* Work around chips which need some time to calm down. */
> if (write_it)
> programmer_delay(1000*1000);
> ret = verify_flash(flash, buf);
> /* If we tried to write, and verification now fails, we
> * might have an emergency situation.
> */
> if (ret && write_it)
> emergency_help_message();
> diff --git a/layout.c b/layout.c
> index 26b7c6a..e9760d5 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -63,149 +63,149 @@ int show_id(uint8_t *bios, int size, int force)
>
The whole show_id() code should be examined and possibly killed. It was
cool to have in the ancient days when nobody was using flashrom for
anything except coreboot.
Besides that, invoking this code for anything but the internal
programmer is just too bogus for words.
> walk--;
> }
>
> /*
> * Check if coreboot last image size is 0 or not a multiple of 1k or
> * bigger than the chip or if the pointers to vendor ID or mainboard ID
> * are outside the image of if the start of ID strings are nonsensical
> * (nonprintable and not \0).
> */
> mb_part_offset = *(walk - 1);
> mb_vendor_offset = *(walk - 2);
> if ((*walk) == 0 || ((*walk) & 0x3ff) != 0 || (*walk) > size ||
> mb_part_offset > size || mb_vendor_offset > size) {
> - printf("Flash image seems to be a legacy BIOS. Disabling checks.\n");
> + msg_ginfo("Flash image seems to be a legacy BIOS. Disabling checks.\n");
> return 0;
> }
>
> mb_part = (char *)(bios + size - mb_part_offset);
> mb_vendor = (char *)(bios + size - mb_vendor_offset);
> if (!isprint((unsigned char)*mb_part) ||
> !isprint((unsigned char)*mb_vendor)) {
> - printf("Flash image seems to have garbage in the ID location."
> + msg_ginfo("Flash image seems to have garbage in the ID location."
> " Disabling checks.\n");
> return 0;
> }
>
> - printf_debug("coreboot last image size "
> + msg_gdbg("coreboot last image size "
> "(not ROM size) is %d bytes.\n", *walk);
>
> mainboard_part = strdup(mb_part);
> mainboard_vendor = strdup(mb_vendor);
> - printf_debug("Manufacturer: %s\n", mainboard_vendor);
> - printf_debug("Mainboard ID: %s\n", mainboard_part);
> + msg_gdbg("Manufacturer: %s\n", mainboard_vendor);
> + msg_gdbg("Mainboard ID: %s\n", mainboard_part);
>
> /*
> * If lb_vendor is not set, the coreboot table was
> * not found. Nor was -m VENDOR:PART specified.
> */
> if (!lb_vendor || !lb_part) {
> - printf("Note: If the following flash access fails, "
> + msg_ginfo("Note: If the following flash access fails, "
> "try -m <vendor>:<mainboard>.\n");
> return 0;
> }
>
> /* These comparisons are case insensitive to make things
> * a little less user^Werror prone.
> */
> if (!strcasecmp(mainboard_vendor, lb_vendor) &&
> !strcasecmp(mainboard_part, lb_part)) {
> - printf_debug("This firmware image matches this mainboard.\n");
> + msg_ginfo("This firmware image matches this mainboard.\n");
> } else {
> if (force) {
> - printf("WARNING: This firmware image does not "
> + msg_ginfo("WARNING: This firmware image does not "
> "seem to fit to this machine - forcing it.\n");
> } else {
> - printf("ERROR: Your firmware image (%s:%s) does not "
> + msg_ginfo("ERROR: Your firmware image (%s:%s) does not "
> "appear to\n be correct for the detected "
> "mainboard (%s:%s)\n\nOverride with --force if you "
> "are absolutely sure that you\nare using a correct "
> "image for this mainboard or override\nthe detected "
> "values with --mainboard <vendor>:<mainboard>.\n\n",
> mainboard_vendor, mainboard_part, lb_vendor,
> lb_part);
> exit(1);
> }
> }
>
> return 0;
> }
> #endif
>
> int read_romlayout(char *name)
> {
> FILE *romlayout;
> char tempstr[256];
> int i;
>
> romlayout = fopen(name, "r");
>
> if (!romlayout) {
> - fprintf(stderr, "ERROR: Could not open ROM layout (%s).\n",
> + msg_gerr("ERROR: Could not open ROM layout (%s).\n",
> name);
> return -1;
> }
>
> while (!feof(romlayout)) {
> char *tstr1, *tstr2;
> if (2 != fscanf(romlayout, "%s %s\n", tempstr, rom_entries[romimages].name))
> continue;
> #if 0
> // fscanf does not like arbitrary comments like that :( later
> if (tempstr[0] == '#') {
> continue;
> }
> #endif
> tstr1 = strtok(tempstr, ":");
> tstr2 = strtok(NULL, ":");
> if (!tstr1 || !tstr2) {
> - fprintf(stderr, "Error parsing layout file.\n");
> + msg_gerr("Error parsing layout file.\n");
> fclose(romlayout);
> return 1;
> }
> rom_entries[romimages].start = strtol(tstr1, (char **)NULL, 16);
> rom_entries[romimages].end = strtol(tstr2, (char **)NULL, 16);
> rom_entries[romimages].included = 0;
> romimages++;
> }
>
> for (i = 0; i < romimages; i++) {
> - printf_debug("romlayout %08x - %08x named %s\n",
> + msg_gdbg("romlayout %08x - %08x named %s\n",
> rom_entries[i].start,
> rom_entries[i].end, rom_entries[i].name);
> }
>
> fclose(romlayout);
>
> return 0;
> }
>
> int find_romentry(char *name)
> {
> int i;
>
> if (!romimages)
> return -1;
>
> - printf("Looking for \"%s\"... ", name);
> + msg_ginfo("Looking for \"%s\"... ", name);
>
> for (i = 0; i < romimages; i++) {
> if (!strcmp(rom_entries[i].name, name)) {
> rom_entries[i].included = 1;
> - printf("found.\n");
> + msg_ginfo("found.\n");
> return i;
> }
> }
> - printf("not found.\n"); // Not found. Error.
> + msg_ginfo("not found.\n"); // Not found. Error.
>
Shouldn't this be _gerr?
>
> return -1;
> }
>
> int handle_romentries(uint8_t *buffer, struct flashchip *flash)
> {
> int i;
>
> // This function does not save flash write cycles.
> //
> // Also it does not cope with overlapping rom layout
> // sections.
> // example:
> diff --git a/print.c b/print.c
> index 2a2c81b..e7ff27e 100644
> --- a/print.c
> +++ b/print.c
> @@ -47,27 +47,27 @@ char *flashbuses_to_text(enum chipbustype bustype)
> if (bustype & CHIP_BUSTYPE_FWH)
> ret = strcat_realloc(ret, "FWH,");
> if (bustype & CHIP_BUSTYPE_SPI)
> ret = strcat_realloc(ret, "SPI,");
> if (bustype == CHIP_BUSTYPE_NONE)
> ret = strcat_realloc(ret, "None,");
> }
> /* Kill last comma. */
> ret[strlen(ret) - 1] = '\0';
> ret = realloc(ret, strlen(ret) + 1);
> return ret;
> }
>
> -#define POS_PRINT(x) do { pos += strlen(x); printf(x); } while (0)
> +#define POS_PRINT(x) do { pos += strlen(x); msg_ginfo(x); } while (0)
>
> static int digits(int n)
>
Should be called count_digits() or maybe log10().
Oh well. Not in this patch.
> {
> int i;
>
> if (!n)
> return 1;
>
> for (i = 0; n; ++i)
> n /= 10;
>
> return i;
> }
> @@ -77,172 +77,172 @@ void print_supported_chips(void)
> int okcol = 0, pos = 0, i, chipcount = 0;
> struct flashchip *f;
>
> for (f = flashchips; f->name != NULL; f++) {
> if (GENERIC_DEVICE_ID == f->model_id)
> continue;
> okcol = max(okcol, strlen(f->vendor) + 1 + strlen(f->name));
> }
> okcol = (okcol + 7) & ~7;
>
> for (f = flashchips; f->name != NULL; f++)
> chipcount++;
>
> - printf("\nSupported flash chips (total: %d):\n\n", chipcount);
> + msg_ginfo("\nSupported flash chips (total: %d):\n\n", chipcount);
> POS_PRINT("Vendor: Device:");
> while (pos < okcol) {
> - printf("\t");
> + msg_ginfo("\t");
>
Does any of this printing even make sense for any graphical frontend
with variable-width characters? I mean, stuff will look totally messed
up because we use tabs and spaces for formatting.
> pos += 8 - (pos % 8);
> }
>
> - printf("Tested OK:\tKnown BAD: Size/KB: Type:\n\n");
> - printf("(P = PROBE, R = READ, E = ERASE, W = WRITE)\n\n");
> + msg_ginfo("Tested OK:\tKnown BAD: Size/KB: Type:\n\n");
> + msg_ginfo("(P = PROBE, R = READ, E = ERASE, W = WRITE)\n\n");
>
> for (f = flashchips; f->name != NULL; f++) {
> /* Don't print "unknown XXXX SPI chip" entries. */
> if (!strncmp(f->name, "unknown", 7))
> continue;
>
> - printf("%s", f->vendor);
> + msg_ginfo("%s", f->vendor);
> for (i = 0; i < 10 - strlen(f->vendor); i++)
> - printf(" ");
> - printf("%s", f->name);
> + msg_ginfo(" ");
> + msg_ginfo("%s", f->name);
>
> pos = 10 + strlen(f->name);
> while (pos < okcol) {
> - printf("\t");
> + msg_ginfo("\t");
> pos += 8 - (pos % 8);
> }
> if ((f->tested & TEST_OK_MASK)) {
> if ((f->tested & TEST_OK_PROBE))
> POS_PRINT("P ");
> if ((f->tested & TEST_OK_READ))
> POS_PRINT("R ");
> if ((f->tested & TEST_OK_ERASE))
> POS_PRINT("E ");
> if ((f->tested & TEST_OK_WRITE))
> POS_PRINT("W ");
> }
> while (pos < okcol + 9) {
> - printf("\t");
> + msg_ginfo("\t");
> pos += 8 - (pos % 8);
> }
> if ((f->tested & TEST_BAD_MASK)) {
> if ((f->tested & TEST_BAD_PROBE))
> - printf("P ");
> + msg_ginfo("P ");
> if ((f->tested & TEST_BAD_READ))
> - printf("R ");
> + msg_ginfo("R ");
> if ((f->tested & TEST_BAD_ERASE))
> - printf("E ");
> + msg_ginfo("E ");
> if ((f->tested & TEST_BAD_WRITE))
> - printf("W ");
> + msg_ginfo("W ");
> }
>
> - printf("\t %d", f->total_size);
> + msg_ginfo("\t %d", f->total_size);
> for (i = 0; i < 10 - digits(f->total_size); i++)
> - printf(" ");
> - printf("%s\n", flashbuses_to_text(f->bustype));
> + msg_ginfo(" ");
> + msg_ginfo("%s\n", flashbuses_to_text(f->bustype));
> }
> }
>
> #if INTERNAL_SUPPORT == 1
> void print_supported_chipsets(void)
> {
> int i, j, chipsetcount = 0;
> const struct penable *c = chipset_enables;
>
> for (i = 0; c[i].vendor_name != NULL; i++)
> chipsetcount++;
>
> - printf("\nSupported chipsets (total: %d):\n\nVendor: "
> + msg_ginfo("\nSupported chipsets (total: %d):\n\nVendor: "
> "Chipset: PCI IDs:\n\n", chipsetcount);
>
> for (i = 0; c[i].vendor_name != NULL; i++) {
> - printf("%s", c[i].vendor_name);
> + msg_ginfo("%s", c[i].vendor_name);
> for (j = 0; j < 25 - strlen(c[i].vendor_name); j++)
> - printf(" ");
> - printf("%s", c[i].device_name);
> + msg_ginfo(" ");
> + msg_ginfo("%s", c[i].device_name);
> for (j = 0; j < 25 - strlen(c[i].device_name); j++)
> - printf(" ");
> - printf("%04x:%04x%s\n", c[i].vendor_id, c[i].device_id,
> + msg_ginfo(" ");
> + msg_ginfo("%04x:%04x%s\n", c[i].vendor_id, c[i].device_id,
> (c[i].status == OK) ? "" : " (untested)");
> }
> }
>
> void print_supported_boards_helper(const struct board_info *b, const char *msg)
> {
> int i, j, boardcount = 0;
>
> for (i = 0; b[i].vendor != NULL; i++)
> boardcount++;
>
> - printf("\n%s (total: %d):\n\n", msg, boardcount);
> + msg_ginfo("\n%s (total: %d):\n\n", msg, boardcount);
>
> for (i = 0; b[i].vendor != NULL; i++) {
> - printf("%s", b[i].vendor);
> + msg_ginfo("%s", b[i].vendor);
> for (j = 0; j < 25 - strlen(b[i].vendor); j++)
> - printf(" ");
> - printf("%s", b[i].name);
> + msg_ginfo(" ");
> + msg_ginfo("%s", b[i].name);
> for (j = 0; j < 28 - strlen(b[i].name); j++)
> - printf(" ");
> - printf("\n");
> + msg_ginfo(" ");
> + msg_ginfo("\n");
> }
> }
>
> void print_supported_boards(void)
> {
> int i, j, boardcount = 0;
> struct board_pciid_enable *b = board_pciid_enables;
>
> for (i = 0; b[i].vendor_name != NULL; i++)
> boardcount++;
>
> - printf("\nSupported boards which need write-enable code (total: %d):"
> + msg_ginfo("\nSupported boards which need write-enable code (total: %d):"
> "\n\nVendor: Board: "
> "Required option:\n\n", boardcount);
>
> for (i = 0; b[i].vendor_name != NULL; i++) {
> - printf("%s", b[i].vendor_name);
> + msg_ginfo("%s", b[i].vendor_name);
> for (j = 0; j < 25 - strlen(b[i].vendor_name); j++)
> - printf(" ");
> - printf("%s", b[i].board_name);
> + msg_ginfo(" ");
> + msg_ginfo("%s", b[i].board_name);
> for (j = 0; j < 30 - strlen(b[i].board_name); j++)
> - printf(" ");
> + msg_ginfo(" ");
> if (b[i].lb_vendor != NULL)
> - printf("-m %s:%s\n", b[i].lb_vendor, b[i].lb_part);
> + msg_ginfo("-m %s:%s\n", b[i].lb_vendor, b[i].lb_part);
> else
> - printf("(none, board is autodetected)\n");
> + msg_ginfo("(none, board is autodetected)\n");
> }
>
> print_supported_boards_helper(boards_ok,
> "Supported boards which don't need write-enable code");
> print_supported_boards_helper(boards_bad,
> "Boards which have been verified to NOT work yet");
> print_supported_boards_helper(laptops_ok,
> "Laptops which have been verified to work");
> print_supported_boards_helper(laptops_bad,
> "Laptops which have been verified to NOT work yet");
> }
> #endif
>
> void print_supported(void)
> {
> print_supported_chips();
> #if INTERNAL_SUPPORT == 1
> print_supported_chipsets();
> print_supported_boards();
> #endif
> #if (NIC3COM_SUPPORT == 1) || (GFXNVIDIA_SUPPORT == 1) || (DRKAISER_SUPPORT == 1) || (SATASII_SUPPORT == 1)
>
Ooh, bug! Here and below we totally forget about ATAHPT_SUPPORT.
Needs to be fixed in a separate patch.
> - printf("\nSupported PCI devices flashrom can use "
> + msg_ginfo("\nSupported PCI devices flashrom can use "
> "as programmer:\n\n");
> #endif
> #if NIC3COM_SUPPORT == 1
> print_supported_pcidevs(nics_3com);
> #endif
> #if GFXNVIDIA_SUPPORT == 1
> print_supported_pcidevs(gfx_nvidia);
> #endif
> #if DRKAISER_SUPPORT == 1
> print_supported_pcidevs(drkaiser_pcidev);
> #endif
> #if SATASII_SUPPORT == 1
> print_supported_pcidevs(satas_sii);
> diff --git a/print_wiki.c b/print_wiki.c
> index 9c0143c..e5eb521 100644
> --- a/print_wiki.c
> +++ b/print_wiki.c
> @@ -363,237 +363,237 @@ static int note(const char *vendor, const char *board)
> }
>
> return -1;
> }
>
> void print_supported_chipsets_wiki(void)
> {
> int i, j, enablescount = 0, color = 1;
> const struct penable *e;
>
> for (e = chipset_enables; e->vendor_name != NULL; e++)
> enablescount++;
>
> - printf("\n== Supported chipsets ==\n\nTotal amount of supported "
> + msg_ginfo("\n== Supported chipsets ==\n\nTotal amount of supported "
> "chipsets: '''%d'''\n\n{| border=\"0\" valign=\"top\"\n| "
> "valign=\"top\"|\n\n%s", enablescount, chipset_th);
>
> e = chipset_enables;
> for (i = 0, j = 0; e[i].vendor_name != NULL; i++, j++) {
> /* Alternate colors if the vendor changes. */
> if (i > 0 && strcmp(e[i].vendor_name, e[i - 1].vendor_name))
> color = !color;
>
> - printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s "
> + msg_ginfo("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s "
> "|| %04x:%04x || %s\n", (color) ? "eeeeee" : "dddddd",
> e[i].vendor_name, e[i].device_name,
> e[i].vendor_id, e[i].device_id,
> (e[i].status == OK) ? "{{OK}}" : "?");
>
> /* Split table in three columns. */
> if (j >= (enablescount / 3 + 1)) {
> - printf("\n|}\n\n| valign=\"top\"|\n\n%s", chipset_th);
> + msg_ginfo("\n|}\n\n| valign=\"top\"|\n\n%s", chipset_th);
> j = 0;
> }
> }
>
> - printf("\n|}\n\n|}\n");
> + msg_ginfo("\n|}\n\n|}\n");
> }
>
> static void wiki_helper(const char *heading, const char *status,
> int cols, const struct board_info boards[])
> {
> int i, j, k, c, boardcount = 0, color = 1, num_notes = 0;
> const struct board_info *b;
> const struct board_info_url *u = boards_url;
> char *notes = calloc(1, 1);
> char tmp[900 + 1];
>
> for (b = boards; b->vendor != NULL; b++)
> boardcount++;
>
> - printf("\n'''%s'''\n\nTotal amount of boards: '''%d'''\n\n"
> + msg_ginfo("\n'''%s'''\n\nTotal amount of boards: '''%d'''\n\n"
> "{| border=\"0\" valign=\"top\"\n| valign=\"top\"|\n\n%s",
> heading, boardcount, board_th);
>
> for (i = 0, j = 0, b = boards; b[i].vendor != NULL; i++, j++) {
> /* Alternate colors if the vendor changes. */
> if (i > 0 && strcmp(b[i].vendor, b[i - 1].vendor))
> color = !color;
>
> k = url(b[i].vendor, b[i].name);
> c = note(b[i].vendor, b[i].name);
>
> - printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s%s %s%s ||"
> + msg_ginfo("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s%s %s%s ||"
> " {{%s}}", (color) ? "eeeeee" : "dddddd", b[i].vendor,
> (k != -1 && u[k].url) ? "[" : "",
> (k != -1 && u[k].url) ? u[k].url : "",
> b[i].name, (k != -1 && u[k].url) ? "]" : "", status);
>
> if (c != -1) {
> - printf("<sup>%d</sup>\n", num_notes + 1);
> - snprintf((char *)&tmp, 900, "<sup>%d</sup> %s<br />\n",
> + msg_ginfo("<sup>%d</sup>\n", num_notes + 1);
> + snmsg_ginfo((char *)&tmp, 900, "<sup>%d</sup> %s<br />\n",
> 1 + num_notes++, boards_notes[c].note);
> notes = strcat_realloc(notes, (char *)&tmp);
> } else {
> - printf("\n");
> + msg_ginfo("\n");
> }
>
> /* Split table in 'cols' columns. */
> if (j >= (boardcount / cols + 1)) {
> - printf("\n|}\n\n| valign=\"top\"|\n\n%s", board_th);
> + msg_ginfo("\n|}\n\n| valign=\"top\"|\n\n%s", board_th);
> j = 0;
> }
> }
>
> - printf("\n|}\n\n|}\n");
> + msg_ginfo("\n|}\n\n|}\n");
>
> if (num_notes > 0)
> - printf("\n<small>\n%s</small>\n", notes);
> + msg_ginfo("\n<small>\n%s</small>\n", notes);
> free(notes);
> }
>
> static void wiki_helper2(const char *heading, int cols)
> {
> int i, j, k, boardcount = 0, color = 1;
> struct board_pciid_enable *b;
> const struct board_info_url *u = boards_url;
>
> for (b = board_pciid_enables; b->vendor_name != NULL; b++)
> boardcount++;
>
> - printf("\n'''%s'''\n\nTotal amount of boards: '''%d'''\n\n"
> + msg_ginfo("\n'''%s'''\n\nTotal amount of boards: '''%d'''\n\n"
> "{| border=\"0\" valign=\"top\"\n| valign=\"top\"|\n\n%s",
> heading, boardcount, board_th2);
>
> b = board_pciid_enables;
> for (i = 0, j = 0; b[i].vendor_name != NULL; i++, j++) {
> /* Alternate colors if the vendor changes. */
> if (i > 0 && strcmp(b[i].vendor_name, b[i - 1].vendor_name))
> color = !color;
>
> k = url(b[i].vendor_name, b[i].board_name);
>
> - printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s%s %s%s "
> + msg_ginfo("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s%s %s%s "
> "|| %s%s%s%s || {{OK}}\n", (color) ? "eeeeee" : "dddddd",
> b[i].vendor_name, (k != -1 && u[k].url) ? "[" : "",
> (k != -1 && u[k].url) ? u[k].url : "", b[i].board_name,
> (k != -1 && u[k].url) ? "]" : "",
> (b[i].lb_vendor) ? "-m " : "—",
> (b[i].lb_vendor) ? b[i].lb_vendor : "",
> (b[i].lb_vendor) ? ":" : "",
> (b[i].lb_vendor) ? b[i].lb_part : "");
>
> /* Split table in three columns. */
> if (j >= (boardcount / cols + 1)) {
> - printf("\n|}\n\n| valign=\"top\"|\n\n%s", board_th2);
> + msg_ginfo("\n|}\n\n| valign=\"top\"|\n\n%s", board_th2);
> j = 0;
> }
> }
>
> - printf("\n|}\n\n|}\n");
> + msg_ginfo("\n|}\n\n|}\n");
> }
>
> void print_supported_boards_wiki(void)
> {
> - printf("%s", board_intro);
> + msg_ginfo("%s", board_intro);
> wiki_helper("Known good (worked out of the box)", "OK", 3, boards_ok);
> wiki_helper2("Known good (with write-enable code in flashrom)", 3);
> wiki_helper("Not supported (yet)", "No", 3, boards_bad);
>
> - printf("%s", laptop_intro);
> + msg_ginfo("%s", laptop_intro);
> wiki_helper("Known good (worked out of the box)", "OK", 1, laptops_ok);
> wiki_helper("Not supported (yet)", "No", 1, laptops_bad);
> }
>
> void print_supported_chips_wiki(void)
> {
> int i = 0, c = 1, chipcount = 0;
> struct flashchip *f, *old = NULL;
> uint32_t t;
>
> for (f = flashchips; f->name != NULL; f++)
> chipcount++;
>
> - printf("\n== Supported chips ==\n\nTotal amount of supported "
> + msg_ginfo("\n== Supported chips ==\n\nTotal amount of supported "
> "chips: '''%d'''\n\n{| border=\"0\" valign=\"top\"\n"
> "| valign=\"top\"|\n\n%s", chipcount, chip_th);
>
> for (f = flashchips; f->name != NULL; f++, i++) {
> /* Don't print "unknown XXXX SPI chip" entries. */
> if (!strncmp(f->name, "unknown", 7))
> continue;
>
> /* Alternate colors if the vendor changes. */
> if (old != NULL && strcmp(old->vendor, f->vendor))
> c = !c;
>
> t = f->tested;
> - printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s || %d "
> + msg_ginfo("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s || %d "
> "|| %s || {{%s}} || {{%s}} || {{%s}} || {{%s}}\n",
> (c == 1) ? "eeeeee" : "dddddd", f->vendor, f->name,
> f->total_size, flashbuses_to_text(f->bustype),
> (t & TEST_OK_PROBE) ? "OK" :
> (t & TEST_BAD_PROBE) ? "No" : ((c) ? "?2" : "?"),
> (t & TEST_OK_READ) ? "OK" :
> (t & TEST_BAD_READ) ? "No" : ((c) ? "?2" : "?"),
> (t & TEST_OK_ERASE) ? "OK" :
> (t & TEST_BAD_ERASE) ? "No" : ((c) ? "?2" : "?"),
> (t & TEST_OK_WRITE) ? "OK" :
> (t & TEST_BAD_WRITE) ? "No" : ((c) ? "?2" : "?"));
>
> /* Split table into three columns. */
> if (i >= (chipcount / 3 + 1)) {
> - printf("\n|}\n\n| valign=\"top\"|\n\n%s", chip_th);
> + msg_ginfo("\n|}\n\n| valign=\"top\"|\n\n%s", chip_th);
> i = 0;
> }
>
> old = f;
> }
>
> - printf("\n|}\n\n|}\n");
> + msg_ginfo("\n|}\n\n|}\n");
> }
>
> void print_supported_pcidevs_wiki(struct pcidev_status *devs)
> {
> int i = 0;
> static int c = 0;
>
> /* Alternate colors if the vendor changes. */
> c = !c;
>
> for (i = 0; devs[i].vendor_name != NULL; i++) {
> - printf("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s || "
> + msg_ginfo("|- bgcolor=\"#%s\" valign=\"top\"\n| %s || %s || "
> "%04x:%04x || {{%s}}\n", (c) ? "eeeeee" : "dddddd",
> devs[i].vendor_name, devs[i].device_name,
> devs[i].vendor_id, devs[i].device_id,
> (devs[i].status == NT) ? (c) ? "?2" : "?" : "OK");
> }
> }
>
> void print_supported_wiki(void)
> {
> time_t t = time(NULL);
>
> - printf(wiki_header, ctime(&t), flashrom_version);
> + msg_ginfo(wiki_header, ctime(&t), flashrom_version);
> print_supported_chips_wiki();
> print_supported_chipsets_wiki();
> print_supported_boards_wiki();
> - printf("%s", programmer_section);
> + msg_ginfo("%s", programmer_section);
> #if NIC3COM_SUPPORT == 1
> print_supported_pcidevs_wiki(nics_3com);
> #endif
> #if GFXNVIDIA_SUPPORT == 1
> print_supported_pcidevs_wiki(gfx_nvidia);
> #endif
> #if DRKAISER_SUPPORT == 1
> print_supported_pcidevs_wiki(drkaiser_pcidev);
> #endif
> #if SATASII_SUPPORT == 1
> print_supported_pcidevs_wiki(satas_sii);
> #endif
> #if ATAHPT_SUPPORT == 1
> print_supported_pcidevs_wiki(ata_hpt);
> #endif
> - printf("\n|}\n");
> + msg_ginfo("\n|}\n");
> }
>
>
My eyes!
Yes, this review raises more questions than it gives answers. No ack
yet, we need to discuss this.
Sean, I do _not_ expect you to fix the code bugs I found (not in this
patch anyway). Someone (heh!) should send a separate patch for those
bugs so we can roll back bugfixes and msg_* conversion separately in
case this is ever needed.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list