[PATCH] Add logfile support to flashrom
Add log file support to flashrom. The log file will always contain all verbose messages even if the user doesn't specify -V. If the user specifies -VV, SPEW messages will be logged as well. Proof of concept, comments welcome. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-logfile/flash.h =================================================================== --- flashrom-logfile/flash.h (Revision 1326) +++ flashrom-logfile/flash.h (Arbeitskopie) @@ -229,6 +229,8 @@ #define ERROR_NONFATAL 0x100 /* cli_output.c */ +int open_logfile(char *filename); +int msg_log(const char *fmt, ...); /* Let gcc and clang check for correct printf-style format strings. */ int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); #define MSG_ERROR 0 Index: flashrom-logfile/cli_output.c =================================================================== --- flashrom-logfile/cli_output.c (Revision 1326) +++ flashrom-logfile/cli_output.c (Arbeitskopie) @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright (C) 2009 Sean Nelson <audiohacked@gmail.com> + * Copyright (C) 2011 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -22,30 +23,70 @@ #include <stdarg.h> #include "flash.h" -int print(int type, const char *fmt, ...) +static FILE *logfile = NULL; + +int open_logfile(char *filename) { + if (!filename) { + msg_gerr("No filename specified.\n"); + return 1; + } + if ((logfile = fopen(filename, "w")) == NULL) { + perror(filename); + return 1; + } + return 0; +} + +int msg_log(const char *fmt, ...) +{ va_list ap; int ret; - FILE *output_type; + if (!logfile) + return -1; + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + return ret; +} + +int print(int type, const char *fmt, ...) +{ + va_list ap; + int ret = 0; + int want_screen = 1; + int want_file = 1; + FILE *output_type = stdout; + switch (type) { case MSG_ERROR: output_type = stderr; break; case MSG_BARF: - if (verbose < 2) - return 0; + if (verbose < 2) { + want_screen = 0; + want_file = 0; + } + break; case MSG_DEBUG: if (verbose < 1) - return 0; + want_screen = 0; + break; case MSG_INFO: default: - output_type = stdout; break; } - va_start(ap, fmt); - ret = vfprintf(output_type, fmt, ap); - va_end(ap); + if (want_screen) { + va_start(ap, fmt); + ret = vfprintf(output_type, fmt, ap); + va_end(ap); + } + if (want_file && logfile) { + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + } return ret; } Index: flashrom-logfile/cli_classic.c =================================================================== --- flashrom-logfile/cli_classic.c (Revision 1326) +++ flashrom-logfile/cli_classic.c (Arbeitskopie) @@ -38,7 +38,8 @@ "-z|" #endif "-E|-r <file>|-w <file>|-v <file>]\n" - " [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>]\n" + " [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>] " + "[-o <file>]\n" " [-i <image>] [-p <programmername>[:<parameters>]]\n\n"); printf("Please note that the command line interface for flashrom has " @@ -72,6 +73,7 @@ "<file>\n" " -i | --image <name> only flash image <name> " "from flash layout\n" + " -o | --output <name> log to file <name>\n" " -L | --list-supported print supported devices\n" #if CONFIG_PRINT_WIKI == 1 " -z | --list-supported-wiki print supported devices " @@ -118,7 +120,7 @@ int operation_specified = 0; int i; - static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh"; + static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzho:"; static const struct option long_options[] = { {"read", 1, NULL, 'r'}, {"write", 1, NULL, 'w'}, @@ -136,6 +138,7 @@ {"programmer", 1, NULL, 'p'}, {"help", 0, NULL, 'h'}, {"version", 0, NULL, 'R'}, + {"output", 1, NULL, 'o'}, {NULL, 0, NULL, 0} }; @@ -307,14 +310,17 @@ cli_classic_usage(argv[0]); exit(0); break; + case 'o': + tempstr = strdup(optarg); + if (open_logfile(tempstr)) + cli_classic_abort_usage(); + break; default: cli_classic_abort_usage(); break; } } - /* FIXME: Print the actions flashrom will take. */ - if (list_supported) { print_supported(); exit(0); @@ -355,6 +361,10 @@ flash = NULL; } + /* FIXME: Print the actions flashrom will take. */ + /* For the log. FIXME: Print this only to the log file. */ + print_version(); + /* FIXME: Delay calibration should happen in programmer code. */ myusec_calibrate_delay(); -- http://www.hailfinger.org/
On Thu, Jun 09, 2011 at 10:32:30PM +0200, Carl-Daniel Hailfinger wrote:
Add log file support to flashrom.
The log file will always contain all verbose messages even if the user doesn't specify -V. If the user specifies -VV, SPEW messages will be logged as well.
Proof of concept, comments welcome.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Index: flashrom-logfile/flash.h =================================================================== --- flashrom-logfile/flash.h (Revision 1326) +++ flashrom-logfile/flash.h (Arbeitskopie) @@ -229,6 +229,8 @@ #define ERROR_NONFATAL 0x100
/* cli_output.c */ +int open_logfile(char *filename);
Can be "const char" too, I guess.
+int msg_log(const char *fmt, ...); /* Let gcc and clang check for correct printf-style format strings. */ int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); #define MSG_ERROR 0 Index: flashrom-logfile/cli_output.c =================================================================== --- flashrom-logfile/cli_output.c (Revision 1326) +++ flashrom-logfile/cli_output.c (Arbeitskopie) @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright (C) 2009 Sean Nelson <audiohacked@gmail.com> + * Copyright (C) 2011 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -22,30 +23,70 @@ #include <stdarg.h> #include "flash.h"
-int print(int type, const char *fmt, ...) +static FILE *logfile = NULL; + +int open_logfile(char *filename) { + if (!filename) { + msg_gerr("No filename specified.\n"); + return 1; + } + if ((logfile = fopen(filename, "w")) == NULL) {
Use "wb" please, the "b" is required to avoid issues on Windows. There's another occurence in flashrom of fopen() withouth "b", that should be fixed too.
+int print(int type, const char *fmt, ...) +{ + va_list ap;
+ int ret = 0; + int want_screen = 1; + int want_file = 1;
Can be merged in one line for more compact code. There's a small issue with the patch, it double-prints some lines on stdout now (in the logfile they appear only once if -o is used): $ ./flashrom flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian flashrom is free software, get the source code at http://www.flashrom.org flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian Calibrating delay loop... OK. ERROR: Could not get I/O privileges (Operation not permitted). You need to be root. Also, not all whitespace seems to be the same as in the logfile, e.g. for $ ./flashrom -L -o foo Uwe. -- http://hermann-uwe.de | http://sigrok.org http://randomprojects.org | http://unmaintained-free-software.org
Op 11-6-2011 16:21, Uwe Hermann schreef:
On Thu, Jun 09, 2011 at 10:32:30PM +0200, Carl-Daniel Hailfinger wrote:
My apologies to Uwe for sending my reply to him personally, I'm used to lists where "reply" addresses the list rather than sender. Listed some cosmetic issues/ideas, able to repost them if anyone might value my input.
Am 11.06.2011 16:56 schrieb Bernd Blaauw:
Op 11-6-2011 16:21, Uwe Hermann schreef:
On Thu, Jun 09, 2011 at 10:32:30PM +0200, Carl-Daniel Hailfinger wrote:
My apologies to Uwe for sending my reply to him personally, I'm used to lists where "reply" addresses the list rather than sender. Listed some cosmetic issues/ideas, able to repost them if anyone might value my input.
Yes, I'm interested. Please repost them to the list. Regards, Carl-Daniel -- http://www.hailfinger.org/
Op 13-6-2011 0:16, Carl-Daniel Hailfinger schreef:
Yes, I'm interested. Please repost them to the list.
Done, see below. Op 11-6-2011 16:21, Uwe Hermann schreef:
There's a small issue with the patch, it double-prints some lines on stdout now (in the logfile they appear only once if -o is used):
$ ./flashrom flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian flashrom is free software, get the source code at http://www.flashrom.org
On DOS platform, I see this "little endian" text truncated as ", l" "ittle endian" "flashrom is free". must be the 80x25 default screen mode. Anyway to make this pretty instead of cutting words?
flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian Calibrating delay loop... OK. ERROR: Could not get I/O privileges (Operation not permitted). You need to be root.
Also, not all whitespace seems to be the same as in the logfile, e.g. for
$ ./flashrom -L -o foo
Seems like Uwe's right about the empty spaces, experienced the same with Idwer's "latest" DOS binary. Just curious: does FlashROM have any operations that don't require root on Linux? Otherwise you'd simply give the "you need to be root" warning before doing calibration loop, saving a second of waiting time or so? Last cosmetic gripe: "bad command or filename" printed 6 times in case FlashROM can't find the (hopefully optional?) DOS "DMIDECOD" program. Is the logfile purely optional? A plain simple bootCD with DOS, FlashROM and BIOS file means a read-only filesystem. As not all writes might be succesfull, is it an idea to read the BIOS backup in pogram memory (instead of storing on disk) and offer to restore it in case writing new file goes bad? That would be nice on read-only platforms (or those without enough space to write file to)
Am 13.06.2011 00:26 schrieb Bernd Blaauw:
Op 11-6-2011 16:21, Uwe Hermann schreef:
There's a small issue with the patch, it double-prints some lines on stdout now (in the logfile they appear only once if -o is used):
$ ./flashrom flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian flashrom is free software, get the source code at http://www.flashrom.org
On DOS platform, I see this "little endian" text truncated as ", l" "ittle endian" "flashrom is free".
must be the 80x25 default screen mode. Anyway to make this pretty instead of cutting words?
Yes. I decided to kill most of the version message in normal verbosity.
flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian Calibrating delay loop... OK. ERROR: Could not get I/O privileges (Operation not permitted). You need to be root.
Also, not all whitespace seems to be the same as in the logfile, e.g. for
$ ./flashrom -L -o foo
Seems like Uwe's right about the empty spaces, experienced the same with Idwer's "latest" DOS binary.
I "fixed" this by not logging anything if you specify -L (or any other action which won't access the chip).
Just curious: does FlashROM have any operations that don't require root on Linux?
Yes. For tests, we have the dummy programmer and flash chip emulation. The man page has some info about that. flashrom -p dummy
Otherwise you'd simply give the "you need to be root" warning before doing calibration loop, saving a second of waiting time or so?
Dummy wants calibration as well, but I plan to move that into programmer init soon.
Last cosmetic gripe: "bad command or filename" printed 6 times in case FlashROM can't find the (hopefully optional?) DOS "DMIDECOD" program.
Can you start a new thread about that? We plan to disable external dmidecode support soon, but until then we should not annoy people that much.
Is the logfile purely optional? A plain simple bootCD with DOS, FlashROM and BIOS file means a read-only filesystem.
Yes, logfile will be optional.
As not all writes might be succesfull, is it an idea to read the BIOS backup in pogram memory (instead of storing on disk) and offer to restore it in case writing new file goes bad?
We already have parts of such a recovery logic, I hope to finish it for 0.9.4.
That would be nice on read-only platforms (or those without enough space to write file to)
Sure. Regards, Carl-Daniel New version. Add log file support to flashrom. The log file will always contain all verbose messages even if the user doesn't specify -V. If the user specifies -VV, SPEW messages will be logged as well. Please note that the log file will be empty if no programmer is accessed. This affects --list-supported --list-supported-wiki --help --version. Only print flashrom version and OS in default output, downgrade compiler/libpci/endianness info to verbose output. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-logfile/flash.h =================================================================== --- flashrom-logfile/flash.h (Revision 1337) +++ flashrom-logfile/flash.h (Arbeitskopie) @@ -229,6 +229,12 @@ #define ERROR_NONFATAL 0x100 /* cli_output.c */ +#ifndef STANDALONE +int open_logfile(const char * const filename); +int close_logfile(void); +void start_logging(void); +#endif +int msg_log(const char *fmt, ...); /* Let gcc and clang check for correct printf-style format strings. */ int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); #define MSG_ERROR 0 Index: flashrom-logfile/cli_output.c =================================================================== --- flashrom-logfile/cli_output.c (Revision 1337) +++ flashrom-logfile/cli_output.c (Arbeitskopie) @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright (C) 2009 Sean Nelson <audiohacked@gmail.com> + * Copyright (C) 2011 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -22,30 +23,96 @@ #include <stdarg.h> #include "flash.h" -int print(int type, const char *fmt, ...) +static FILE *logfile = NULL; +static int want_log = 0; + +#ifndef STANDALONE +int close_logfile(void) { + if (logfile && fclose(logfile)) + return 1; + return 0; +} + +int open_logfile(const char * const filename) +{ + if (!filename) { + msg_gerr("No filename specified.\n"); + return 1; + } + if ((logfile = fopen(filename, "w")) == NULL) { + perror(filename); + return 1; + } + return 0; +} + +void start_logging(void) +{ + int oldverbose = verbose; + + want_log = 1; + /* Shut up the console. */ + verbose = -2; + print_version(); + verbose = oldverbose; +} +#endif /* STANDALONE */ + +int msg_log(const char *fmt, ...) +{ va_list ap; int ret; - FILE *output_type; + if (!logfile) + return -1; + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + return ret; +} + +int print(int type, const char *fmt, ...) +{ + va_list ap; + int ret = 0; + int want_screen = 1; + int want_file = 1; + FILE *output_type = stdout; + switch (type) { - case MSG_ERROR: - output_type = stderr; + case MSG_BARF: + if (verbose < 2) { + want_screen = 0; + want_file = 0; + } break; - case MSG_BARF: - if (verbose < 2) - return 0; case MSG_DEBUG: if (verbose < 1) - return 0; + want_screen = 0; + break; case MSG_INFO: + if (verbose < 0) + want_screen = 0; + break; + case MSG_ERROR: + if (verbose < -1) + want_screen = 0; + output_type = stderr; + break; default: - output_type = stdout; break; } - va_start(ap, fmt); - ret = vfprintf(output_type, fmt, ap); - va_end(ap); + if (want_screen) { + va_start(ap, fmt); + ret = vfprintf(output_type, fmt, ap); + va_end(ap); + } + if (want_file && logfile && want_log) { + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + } return ret; } Index: flashrom-logfile/cli_classic.c =================================================================== --- flashrom-logfile/cli_classic.c (Revision 1337) +++ flashrom-logfile/cli_classic.c (Arbeitskopie) @@ -38,7 +38,8 @@ "-z|" #endif "-E|-r <file>|-w <file>|-v <file>]\n" - " [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>]\n" + " [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>] " + "[-o <file>]\n" " [-i <image>] [-p <programmername>[:<parameters>]]\n\n"); printf("Please note that the command line interface for flashrom has " @@ -72,6 +73,7 @@ "<file>\n" " -i | --image <name> only flash image <name> " "from flash layout\n" + " -o | --output <name> log to file <name>\n" " -L | --list-supported print supported devices\n" #if CONFIG_PRINT_WIKI == 1 " -z | --list-supported-wiki print supported devices " @@ -118,7 +120,7 @@ int operation_specified = 0; int i; - static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh"; + static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzho:"; static const struct option long_options[] = { {"read", 1, NULL, 'r'}, {"write", 1, NULL, 'w'}, @@ -136,6 +138,7 @@ {"programmer", 1, NULL, 'p'}, {"help", 0, NULL, 'h'}, {"version", 0, NULL, 'R'}, + {"output", 1, NULL, 'o'}, {NULL, 0, NULL, 0} }; @@ -307,14 +310,22 @@ cli_classic_usage(argv[0]); exit(0); break; + case 'o': + tempstr = strdup(optarg); +#ifdef STANDALONE + fprintf(stderr, "Log file not supported in standalone " + "mode. Aborting.\n"); +#else /* STANDALONE */ + if (open_logfile(tempstr)) +#endif /* STANDALONE */ + cli_classic_abort_usage(); + break; default: cli_classic_abort_usage(); break; } } - /* FIXME: Print the actions flashrom will take. */ - if (list_supported) { print_supported(); exit(0); @@ -355,11 +366,21 @@ flash = NULL; } +#ifndef STANDALONE + start_logging(); +#endif /* STANDALONE */ + + msg_gdbg("Command line:"); + for (i = 0; i < argc; i++) { + msg_gdbg(" %s", argv[i]); + } + msg_gdbg("\n"); + /* FIXME: Delay calibration should happen in programmer code. */ myusec_calibrate_delay(); if (programmer_init(pparam)) { - fprintf(stderr, "Error: Programmer initialization failed.\n"); + msg_perr("Error: Programmer initialization failed.\n"); exit(1); } @@ -372,26 +393,31 @@ } if (chipcount > 1) { - printf("Multiple flash chips were detected:"); + msg_cinfo("Multiple flash chips were detected:"); for (i = 0; i < chipcount; i++) - printf(" %s", flashes[i].name); - printf("\nPlease specify which chip to use with the -c <chipname> option.\n"); + msg_cinfo(" %s", flashes[i].name); + msg_cinfo("\nPlease specify which chip to use with the -c " + "<chipname> option.\n"); programmer_shutdown(); exit(1); } else if (!chipcount) { - printf("No EEPROM/flash device found.\n"); + msg_cinfo("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { - printf("Note: flashrom can never write if the flash chip isn't found automatically.\n"); + msg_cinfo("Note: flashrom can never write if the flash " + "chip isn't found automatically.\n"); } if (force && read_it && chip_to_probe) { - printf("Force read (-f -r -c) requested, pretending the chip is there:\n"); + msg_cinfo("Force read (-f -r -c) requested, pretending " + "the chip is there:\n"); startchip = probe_flash(0, &flashes[0], 1); if (startchip == -1) { - printf("Probing for flash chip '%s' failed.\n", chip_to_probe); + msg_cinfo("Probing for flash chip '%s' failed." + "\n", chip_to_probe); programmer_shutdown(); exit(1); } - printf("Please note that forced reads most likely contain garbage.\n"); + msg_cinfo("Please note that forced reads most likely " + "contain garbage.\n"); return read_flash_to_file(&flashes[0], filename); } // FIXME: flash writes stay enabled! @@ -406,21 +432,21 @@ size = fill_flash->total_size * 1024; if (check_max_decode((buses_supported & fill_flash->bustype), size) && (!force)) { - fprintf(stderr, "Chip is too big for this programmer " - "(-V gives details). Use --force to override.\n"); + msg_cerr("Chip is too big for this programmer " + "(-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(0); } if (!filename && !erase_it) { - printf("Error: No filename specified.\n"); + msg_gerr("Error: No filename specified.\n"); // FIXME: flash writes stay enabled! programmer_shutdown(); exit(1); Index: flashrom-logfile/flashrom.c =================================================================== --- flashrom-logfile/flashrom.c (Revision 1337) +++ flashrom-logfile/flashrom.c (Arbeitskopie) @@ -543,13 +543,18 @@ int programmer_shutdown(void) { + int ret = 0; /* Registering shutdown functions is no longer allowed. */ may_register_shutdown = 0; while (shutdown_fn_count > 0) { int i = --shutdown_fn_count; shutdown_fn[i].func(shutdown_fn[i].data); } - return programmer_table[programmer].shutdown(); + ret = programmer_table[programmer].shutdown(); +#ifndef STANDALONE + ret |= close_logfile(); +#endif + return ret; } void *programmer_map_flash_region(const char *descr, unsigned long phys_addr, @@ -1649,37 +1654,37 @@ msg_ginfo(" on %s %s (%s)", osinfo.sysname, osinfo.release, osinfo.machine); #else - msg_ginfo(" on unknown machine"); + msg_gdbg(" on unknown machine"); #endif - msg_ginfo(", built with"); + msg_gdbg(", built with"); #if NEED_PCI == 1 #ifdef PCILIB_VERSION - msg_ginfo(" libpci %s,", PCILIB_VERSION); + msg_gdbg(" libpci %s,", PCILIB_VERSION); #else - msg_ginfo(" unknown PCI library,"); + msg_gdbg(" unknown PCI library,"); #endif #endif #ifdef __clang__ - msg_ginfo(" LLVM Clang"); + msg_gdbg(" LLVM Clang"); #ifdef __clang_version__ - msg_ginfo(" %s,", __clang_version__); + msg_gdbg(" %s,", __clang_version__); #else - msg_ginfo(" unknown version (before r102686),"); + msg_gdbg(" unknown version (before r102686),"); #endif #elif defined(__GNUC__) - msg_ginfo(" GCC"); + msg_gdbg(" GCC"); #ifdef __VERSION__ - msg_ginfo(" %s,", __VERSION__); + msg_gdbg(" %s,", __VERSION__); #else - msg_ginfo(" unknown version,"); + msg_gdbg(" unknown version,"); #endif #else - msg_ginfo(" unknown compiler,"); + msg_gdbg(" unknown compiler,"); #endif #if defined (__FLASHROM_LITTLE_ENDIAN__) - msg_ginfo(" little endian"); + msg_gdbg(" little endian"); #else - msg_ginfo(" big endian"); + msg_gdbg(" big endian"); #endif msg_ginfo("\n"); } -- http://www.hailfinger.org/
On Tue, 14 Jun 2011 01:39:13 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
Am 13.06.2011 00:26 schrieb Bernd Blaauw:
Op 11-6-2011 16:21, Uwe Hermann schreef:
There's a small issue with the patch, it double-prints some lines on stdout now (in the logfile they appear only once if -o is used):
$ ./flashrom flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian flashrom is free software, get the source code at http://www.flashrom.org
On DOS platform, I see this "little endian" text truncated as ", l" "ittle endian" "flashrom is free".
must be the 80x25 default screen mode. Anyway to make this pretty instead of cutting words?
Yes. I decided to kill most of the version message in normal verbosity.
flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian Calibrating delay loop... OK. ERROR: Could not get I/O privileges (Operation not permitted). You need to be root.
Also, not all whitespace seems to be the same as in the logfile, e.g. for
$ ./flashrom -L -o foo
Seems like Uwe's right about the empty spaces, experienced the same with Idwer's "latest" DOS binary.
I "fixed" this by not logging anything if you specify -L (or any other action which won't access the chip).
better fix all occurrences of printf in normal code imho (those in print.c are fixed in my voltage printing patch set as mentioned on irc).
Otherwise you'd simply give the "you need to be root" warning before doing calibration loop, saving a second of waiting time or so?
Dummy wants calibration as well, but I plan to move that into programmer init soon.
yay for removing unnecessary delays ;)
New version.
Add log file support to flashrom.
The log file will always contain all verbose messages even if the user doesn't specify -V. If the user specifies -VV, SPEW messages will be logged as well.
Please note that the log file will be empty if no programmer is accessed. This affects --list-supported --list-supported-wiki --help --version.
Only print flashrom version and OS in default output, downgrade compiler/libpci/endianness info to verbose output.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
the patch needs a rebase due to the shutdown callback patch. i have attached a rebased version, but did not put much thought into conflict resolutions, beware.
Index: flashrom-logfile/flash.h =================================================================== --- flashrom-logfile/flash.h (Revision 1337) +++ flashrom-logfile/flash.h (Arbeitskopie) @@ -229,6 +229,12 @@ #define ERROR_NONFATAL 0x100
/* cli_output.c */ +#ifndef STANDALONE +int open_logfile(const char * const filename); +int close_logfile(void); +void start_logging(void); +#endif +int msg_log(const char *fmt, ...); /* Let gcc and clang check for correct printf-style format strings. */ int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); #define MSG_ERROR 0 Index: flashrom-logfile/cli_output.c =================================================================== --- flashrom-logfile/cli_output.c (Revision 1337) +++ flashrom-logfile/cli_output.c (Arbeitskopie) @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright (C) 2009 Sean Nelson <audiohacked@gmail.com> + * Copyright (C) 2011 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -22,30 +23,96 @@ #include <stdarg.h> #include "flash.h"
-int print(int type, const char *fmt, ...) +static FILE *logfile = NULL; +static int want_log = 0; + +#ifndef STANDALONE +int close_logfile(void) { + if (logfile && fclose(logfile)) + return 1; + return 0; +} + +int open_logfile(const char * const filename) +{ + if (!filename) { + msg_gerr("No filename specified.\n"); + return 1; + } + if ((logfile = fopen(filename, "w")) == NULL) { + perror(filename); + return 1; + } + return 0; +} + +void start_logging(void) +{ + int oldverbose = verbose; + + want_log = 1; + /* Shut up the console. */ + verbose = -2; + print_version(); + verbose = oldverbose; +}
misleading name imho. maybe it could be integrated into print_version itself, but you probably wanted them to be split up. the want_log = 1 may be better in open_logfile (which could/should then be renamed to "start_logging?). on the whole i dont think it is worth it to make a distinction between log and output (besides having different verbose levels). why not just output the compile info in debug mode in addition to the version + link stuff in normal mode? i would like the logging to have the following properties: - the log should contain at least as much info as the output on screen (i.e. verbosity for logging should be >= of the verbosity of the printing). - the log should be as equal as possible to the printing when the same verbosity is used. - the log file is created automatically when not turned off by a command line option (switchable behavior with a macro maybe. it should be enabled by packagers so that we are always able to get logs from end users in case of emergencies. this could/should also be incorporated into the "DONT POWER OFF" message. but it may be nice to turn it off while developing/testing a lot.) - the log file name should contain a timestamp (YYYY-MM-DD hh-mm-ss, maybe even a timezone indicator where available) by default (i.e. if its name is not overridden by the user).
+#endif /* STANDALONE */ + +int msg_log(const char *fmt, ...) +{ va_list ap; int ret; - FILE *output_type;
+ if (!logfile) + return -1; + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + return ret; +} + +int print(int type, const char *fmt, ...) +{ + va_list ap; + int ret = 0; + int want_screen = 1; + int want_file = 1; + FILE *output_type = stdout; + switch (type) { - case MSG_ERROR: - output_type = stderr; + case MSG_BARF: + if (verbose < 2) { + want_screen = 0; + want_file = 0; + } break; - case MSG_BARF: - if (verbose < 2) - return 0; case MSG_DEBUG: if (verbose < 1) - return 0; + want_screen = 0; + break; case MSG_INFO: + if (verbose < 0) + want_screen = 0; + break; + case MSG_ERROR: + if (verbose < -1) + want_screen = 0; + output_type = stderr; + break; default: - output_type = stdout; break; }
- va_start(ap, fmt); - ret = vfprintf(output_type, fmt, ap); - va_end(ap); + if (want_screen) { + va_start(ap, fmt); + ret = vfprintf(output_type, fmt, ap); + va_end(ap); + } + if (want_file && logfile && want_log) { + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + } return ret; } Index: flashrom-logfile/cli_classic.c =================================================================== --- flashrom-logfile/cli_classic.c (Revision 1337) +++ flashrom-logfile/cli_classic.c (Arbeitskopie) @@ -38,7 +38,8 @@ "-z|" #endif "-E|-r <file>|-w <file>|-v <file>]\n" - " [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>]\n" + " [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>] " + "[-o <file>]\n" " [-i <image>] [-p <programmername>[:<parameters>]]\n\n");
printf("Please note that the command line interface for flashrom has " @@ -72,6 +73,7 @@ "<file>\n" " -i | --image <name> only flash image <name> " "from flash layout\n" + " -o | --output <name> log to file <name>\n" " -L | --list-supported print supported devices\n" #if CONFIG_PRINT_WIKI == 1 " -z | --list-supported-wiki print supported devices " @@ -118,7 +120,7 @@ int operation_specified = 0; int i;
- static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh"; + static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzho:"; static const struct option long_options[] = { {"read", 1, NULL, 'r'}, {"write", 1, NULL, 'w'}, @@ -136,6 +138,7 @@ {"programmer", 1, NULL, 'p'}, {"help", 0, NULL, 'h'}, {"version", 0, NULL, 'R'}, + {"output", 1, NULL, 'o'}, {NULL, 0, NULL, 0} };
@@ -307,14 +310,22 @@ cli_classic_usage(argv[0]); exit(0); break; + case 'o': + tempstr = strdup(optarg); +#ifdef STANDALONE + fprintf(stderr, "Log file not supported in standalone " + "mode. Aborting.\n"); +#else /* STANDALONE */ + if (open_logfile(tempstr)) +#endif /* STANDALONE */ + cli_classic_abort_usage(); + break; default: cli_classic_abort_usage(); break; } }
- /* FIXME: Print the actions flashrom will take. */ - if (list_supported) { print_supported(); exit(0); @@ -355,11 +366,21 @@ flash = NULL; }
+#ifndef STANDALONE + start_logging(); +#endif /* STANDALONE */ + + msg_gdbg("Command line:"); + for (i = 0; i < argc; i++) { + msg_gdbg(" %s", argv[i]); + } + msg_gdbg("\n");
yay! in very stupid cases distinguishing between arguments may help debugging. something like msg_gspew(" \"%s\"", argv[i]);
/* FIXME: Delay calibration should happen in programmer code. */ myusec_calibrate_delay();
if (programmer_init(pparam)) { - fprintf(stderr, "Error: Programmer initialization failed.\n"); + msg_perr("Error: Programmer initialization failed.\n"); exit(1); }
@@ -372,26 +393,31 @@ }
if (chipcount > 1) { - printf("Multiple flash chips were detected:"); + msg_cinfo("Multiple flash chips were detected:"); for (i = 0; i < chipcount; i++) - printf(" %s", flashes[i].name); - printf("\nPlease specify which chip to use with the -c <chipname> option.\n"); + msg_cinfo(" %s", flashes[i].name); + msg_cinfo("\nPlease specify which chip to use with the -c " + "<chipname> option.\n"); programmer_shutdown(); exit(1); } else if (!chipcount) { - printf("No EEPROM/flash device found.\n"); + msg_cinfo("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { - printf("Note: flashrom can never write if the flash chip isn't found automatically.\n"); + msg_cinfo("Note: flashrom can never write if the flash " + "chip isn't found automatically.\n"); } if (force && read_it && chip_to_probe) { - printf("Force read (-f -r -c) requested, pretending the chip is there:\n"); + msg_cinfo("Force read (-f -r -c) requested, pretending " + "the chip is there:\n"); startchip = probe_flash(0, &flashes[0], 1); if (startchip == -1) { - printf("Probing for flash chip '%s' failed.\n", chip_to_probe); + msg_cinfo("Probing for flash chip '%s' failed." + "\n", chip_to_probe); programmer_shutdown(); exit(1); } - printf("Please note that forced reads most likely contain garbage.\n"); + msg_cinfo("Please note that forced reads most likely " + "contain garbage.\n"); return read_flash_to_file(&flashes[0], filename); } // FIXME: flash writes stay enabled! @@ -406,21 +432,21 @@ size = fill_flash->total_size * 1024; if (check_max_decode((buses_supported & fill_flash->bustype), size) && (!force)) { - fprintf(stderr, "Chip is too big for this programmer " - "(-V gives details). Use --force to override.\n"); + msg_cerr("Chip is too big for this programmer " + "(-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(0); }
if (!filename && !erase_it) { - printf("Error: No filename specified.\n"); + msg_gerr("Error: No filename specified.\n"); // FIXME: flash writes stay enabled! programmer_shutdown(); exit(1); Index: flashrom-logfile/flashrom.c =================================================================== --- flashrom-logfile/flashrom.c (Revision 1337) +++ flashrom-logfile/flashrom.c (Arbeitskopie) @@ -543,13 +543,18 @@
int programmer_shutdown(void) { + int ret = 0; /* Registering shutdown functions is no longer allowed. */ may_register_shutdown = 0; while (shutdown_fn_count > 0) { int i = --shutdown_fn_count; shutdown_fn[i].func(shutdown_fn[i].data); } - return programmer_table[programmer].shutdown(); + ret = programmer_table[programmer].shutdown(); +#ifndef STANDALONE + ret |= close_logfile(); +#endif + return ret; }
void *programmer_map_flash_region(const char *descr, unsigned long phys_addr, @@ -1649,37 +1654,37 @@ msg_ginfo(" on %s %s (%s)", osinfo.sysname, osinfo.release, osinfo.machine); #else - msg_ginfo(" on unknown machine"); + msg_gdbg(" on unknown machine"); #endif - msg_ginfo(", built with"); + msg_gdbg(", built with"); #if NEED_PCI == 1 #ifdef PCILIB_VERSION - msg_ginfo(" libpci %s,", PCILIB_VERSION); + msg_gdbg(" libpci %s,", PCILIB_VERSION); #else - msg_ginfo(" unknown PCI library,"); + msg_gdbg(" unknown PCI library,"); #endif #endif #ifdef __clang__ - msg_ginfo(" LLVM Clang"); + msg_gdbg(" LLVM Clang"); #ifdef __clang_version__ - msg_ginfo(" %s,", __clang_version__); + msg_gdbg(" %s,", __clang_version__); #else - msg_ginfo(" unknown version (before r102686),"); + msg_gdbg(" unknown version (before r102686),"); #endif #elif defined(__GNUC__) - msg_ginfo(" GCC"); + msg_gdbg(" GCC"); #ifdef __VERSION__ - msg_ginfo(" %s,", __VERSION__); + msg_gdbg(" %s,", __VERSION__); #else - msg_ginfo(" unknown version,"); + msg_gdbg(" unknown version,"); #endif #else - msg_ginfo(" unknown compiler,"); + msg_gdbg(" unknown compiler,"); #endif #if defined (__FLASHROM_LITTLE_ENDIAN__) - msg_ginfo(" little endian"); + msg_gdbg(" little endian"); #else - msg_ginfo(" big endian"); + msg_gdbg(" big endian"); #endif msg_ginfo("\n"); }
i did not rigorously review everything, so no ack (yet). if you wanna commit as is i could look at it more carefully... -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
after an IRC discussion with carldani i propose the following solution. maybe not mergeable, but you will get the idea. the current order of patches in my volt+log branch is: 1. patchset voltage printing 2.1 2. rebased add log file support 3. attached patch. i can repost the whole series if you like. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
Ouch, that one was in my draft folder, and I wondered why I didn't see an answer. Am 19.06.2011 14:44 schrieb Stefan Tauner:
after an IRC discussion with carldani i propose the following solution. maybe not mergeable, but you will get the idea.
the current order of patches in my volt+log branch is: 1. patchset voltage printing 2.1 2. rebased add log file support 3. attached patch.
i can repost the whole series if you like.
From: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> Date: Sun, 19 Jun 2011 14:39:37 +0200 Subject: [PATCH] squash! Add log file support to flashrom.
- instead of the want_log variable "print" will always write to the log if a log file is open (indicated by logfile != NULL)
If we don't have to push screen-invisible messages to the logfile, that's indeed an option.
- print_version is no longer executed "implicitly" before argument parsing
This will cause the version to be printed multiple times if someone specifies -h -R or some other special combination.
- cli_classic uses the "goto cleanup"-pattern where it closes the log file
Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
diff --git a/cli_classic.c b/cli_classic.c index ed36c85..71f8bec 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -147,9 +149,6 @@ int cli_classic(int argc, char *argv[]) char *tempstr = NULL; char *pparam = NULL;
- print_version(); - print_banner(); -
If the selfcheck fails, we won't see any version message.
if (selfcheck()) exit(1);
@@ -311,14 +311,18 @@ int cli_classic(int argc, char *argv[]) exit(0); break; case 'o': - tempstr = strdup(optarg); #ifdef STANDALONE fprintf(stderr, "Log file not supported in standalone " "mode. Aborting.\n"); + cli_classic_abort_usage(); #else /* STANDALONE */ - if (open_logfile(tempstr)) -#endif /* STANDALONE */ + log_name = strdup(optarg);
strdup(NULL) is allowed to explode. However, AFAICS optarg is guaranteed not to be NULL, so it should work out fine.
+ if (log_name == NULL || log_name[0] == '\0') {
This strikes me as odd. The "can we open the file" check is postponed, but the "did the user specify a filename" check is still in here.
+ fprintf(stderr, + "No log file name specified.\n"); cli_classic_abort_usage(); + } +#endif /* STANDALONE */ break; default: cli_classic_abort_usage(); @@ -326,18 +330,6 @@ int cli_classic(int argc, char *argv[]) } }
- if (list_supported) { - print_supported(); - exit(0); - } - -#if CONFIG_PRINT_WIKI == 1 - if (list_supported_wiki) { - print_supported_wiki(); - exit(0); - } -#endif - if (optind < argc) { fprintf(stderr, "Error: Extra parameter found.\n"); cli_classic_abort_usage(); @@ -351,6 +343,26 @@ int cli_classic(int argc, char *argv[]) } #endif
+#if CONFIG_PRINT_WIKI == 1 + if (list_supported_wiki) { + print_supported_wiki(); + return 0; + } +#endif
Why did you place the print_supported_wiki() call before opening the logfile, but print_supported() after opening the logfile? I touched that ordering in r1373, hopefully the current state is more agreeable for you.
+ +#ifndef STANDALONE + if (open_logfile(log_name)) + return 1; +#endif /* !STANDALONE */ + + print_version(); + print_banner(); + + if (list_supported) { + print_supported(); + goto cleanup; + } + if (chip_to_probe) { for (flash = flashchips; flash && flash->name; flash++) if (!strcmp(flash->name, chip_to_probe))
goto cleanup makes sense, but I'd handle only cases which need programmer_shutdown... that would kill quite a few duplicates. This part is now semi-obsolete since r1373. Review of the rest will follow later. Regards, Carl-Daniel
@@ -360,16 +372,13 @@ int cli_classic(int argc, char *argv[]) chip_to_probe); printf("Run flashrom -L to view the hardware supported " "in this flashrom version.\n"); - exit(1); + ret = 1; + goto cleanup; } /* Clean up after the check. */ flash = NULL; }
-#ifndef STANDALONE - start_logging(); -#endif /* STANDALONE */ - msg_gdbg("Command line:"); for (i = 0; i < argc; i++) { msg_gdbg(" %s", argv[i]); @@ -382,7 +391,8 @@ int cli_classic(int argc, char *argv[]) if (programmer_init(pparam)) { msg_perr("Error: Programmer initialization failed.\n"); programmer_shutdown(); - exit(1); + ret = 1; + goto cleanup; }
for (i = 0; i < ARRAY_SIZE(flashes); i++) { @@ -400,7 +410,8 @@ int cli_classic(int argc, char *argv[]) msg_cinfo("\nPlease specify which chip to use with the -c " "<chipname> option.\n"); programmer_shutdown(); - exit(1); + ret = 1; + goto cleanup; } else if (!chipcount) { msg_cinfo("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { @@ -415,15 +426,18 @@ int cli_classic(int argc, char *argv[]) msg_cinfo("Probing for flash chip '%s' failed." "\n", chip_to_probe); programmer_shutdown(); - exit(1); + ret = 1; + goto cleanup; } msg_cinfo("Please note that forced reads most likely " "contain garbage.\n"); - return read_flash_to_file(&flashes[0], filename); + ret |= read_flash_to_file(&flashes[0], filename); + goto cleanup; } // FIXME: flash writes stay enabled! programmer_shutdown(); - exit(1); + ret = 1; + goto cleanup; }
fill_flash = &flashes[0]; @@ -436,21 +450,23 @@ int cli_classic(int argc, char *argv[]) msg_cerr("Chip is too big for this programmer " "(-V gives details). Use --force to override.\n"); programmer_shutdown(); - return 1; + ret = 1; + goto cleanup; }
if (!(read_it | write_it | verify_it | erase_it)) { msg_ginfo("No operations were specified.\n"); // FIXME: flash writes stay enabled! programmer_shutdown(); - exit(0); + goto cleanup; }
if (!filename && !erase_it) { msg_gerr("Error: No filename specified.\n"); // FIXME: flash writes stay enabled! programmer_shutdown(); - exit(1); + ret = 1; + goto cleanup; }
/* Always verify write operations unless -n is used. */ @@ -462,5 +478,11 @@ int cli_classic(int argc, char *argv[]) * Give the chip time to settle. */ programmer_delay(100000); - return doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + +cleanup: +#ifndef STANDALONE + ret |= close_logfile(); +#endif + return ret; } diff --git a/cli_output.c b/cli_output.c index 7d812d6..88738bf 100644 --- a/cli_output.c +++ b/cli_output.c @@ -24,39 +24,26 @@ #include "flash.h"
static FILE *logfile = NULL; -static int want_log = 0;
#ifndef STANDALONE int close_logfile(void) { - if (logfile && fclose(logfile)) - return 1; - return 0; + int ret = 0; + if (logfile) { + ret |= fclose(logfile); + logfile = NULL; + } // else ret = 1;? + return ret; }
int open_logfile(const char * const filename) { - if (!filename) { - msg_gerr("No filename specified.\n"); - return 1; - } if ((logfile = fopen(filename, "w")) == NULL) { perror(filename); return 1; } return 0; } - -void start_logging(void) -{ - int oldverbose = verbose; - - want_log = 1; - /* Shut up the console. */ - verbose = -2; - print_version(); - verbose = oldverbose; -} #endif /* STANDALONE */
int msg_log(const char *fmt, ...) @@ -109,7 +96,7 @@ int print(int type, const char *fmt, ...) ret = vfprintf(output_type, fmt, ap); va_end(ap); } - if (want_file && logfile && want_log) { + if (want_file && logfile) { va_start(ap, fmt); ret = vfprintf(logfile, fmt, ap); va_end(ap); diff --git a/flash.h b/flash.h index 6401b81..9d71229 100644 --- a/flash.h +++ b/flash.h @@ -232,7 +232,6 @@ int write_buf_to_file(unsigned char *buf, unsigned long size, char *filename); #ifndef STANDALONE int open_logfile(const char * const filename); int close_logfile(void); -void start_logging(void); #endif int msg_log(const char *fmt, ...); /* Let gcc and clang check for correct printf-style format strings. */ diff --git a/flashrom.c b/flashrom.c index 55f4942..5daaaaf 100644 --- a/flashrom.c +++ b/flashrom.c @@ -533,9 +533,6 @@ int programmer_shutdown(void) int i = --shutdown_fn_count; ret |= shutdown_fn[i].func(shutdown_fn[i].data); } -#ifndef STANDALONE - ret |= close_logfile(); -#endif return ret; }
The general idea and most of the code is stolen from chromiumos: 8fc0740356ca15d02fb1c65ab43b10844f148c3b bb9049c66ca55e0dc621dd2c70b5d2cb6e5179bf Signed-off-by: Louis Yung-Chieh Lo <yjlou@chromium.org> and the main part: d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8 Signed-off-by: David Hendricks <dhendrix@chromium.org> My implementation does not defer the processing until doit(), but after the argument parsing loop only (doit() should not contain argument checks). This allows to specify -i and -l parameters in any order. I don't like the output in error cases much, any ideas? example of a good run: flashrom -p dummy:emulate=SST25VF032B -i normal -w ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian flashrom is free software, get the source code at http://www.flashrom.org Using region(s): "normal", "gfxrom", "fallback". Calibrating delay loop... OK. Found SST flash chip "SST25VF032B" (4096 kB, SPI) on dummy. Reading old flash chip contents... done. Erasing and writing flash chip... Erase/write done. Verifying flash... VERIFIED. example of a semi-good run: flashrom -p dummy:emulate=SST25VF032B -i normal -w ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian flashrom is free software, get the source code at http://www.flashrom.org Maximum number of ROM images (4) in layout file reached before end of layout file. Ignoring the rest of the layout file. Using region(s): "normal", "gfxrom", "fallback". Calibrating delay loop... OK. Found SST flash chip "SST25VF032B" (4096 kB, SPI) on dummy. Reading old flash chip contents... done. Erasing and writing flash chip... Erase/write done. Verifying flash... VERIFIED. example of a faulty run: flashrom -p dummy:emulate=SST25VF032B -i normal -w ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback -i bios flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian flashrom is free software, get the source code at http://www.flashrom.org Maximum number of ROM images (4) in layout file reached before end of layout file. Ignoring the rest of the layout file. Using region(s): "normal", "gfxrom", "fallback"Invalid region specified: "bios" Please run "flashrom --help" for usage info. Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> --- cli_classic.c | 11 ++++----- flash.h | 2 + layout.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 67 insertions(+), 14 deletions(-) diff --git a/cli_classic.c b/cli_classic.c index 36fe9ad..e44ea86 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -233,14 +233,9 @@ int cli_classic(int argc, char *argv[]) cli_classic_abort_usage(); break; case 'i': - /* FIXME: -l has to be specified before -i. */ tempstr = strdup(optarg); - if (find_romentry(tempstr) < 0) { - fprintf(stderr, "Error: image %s not found in " - "layout file or -i specified before " - "-l\n", tempstr); + if (register_include_arg(tempstr) < 0) cli_classic_abort_usage(); - } break; case 'L': if (++operation_specified > 1) { @@ -325,6 +320,10 @@ int cli_classic(int argc, char *argv[]) cli_classic_abort_usage(); } + if (process_include_args() < 0) { + cli_classic_abort_usage(); + } + /* FIXME: Print the actions flashrom will take. */ if (list_supported) { diff --git a/flash.h b/flash.h index 5b49e9d..0848255 100644 --- a/flash.h +++ b/flash.h @@ -251,6 +251,8 @@ int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); int cli_classic(int argc, char *argv[]); /* layout.c */ +int register_include_arg(char *name); +int process_include_args(void); int read_romlayout(char *name); int find_romentry(char *name); int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents); diff --git a/layout.c b/layout.c index d719a05..936e316 100644 --- a/layout.c +++ b/layout.c @@ -41,6 +41,12 @@ typedef struct { char name[256]; } romlayout_t; +/* include_args lists arguments specified at the command line with -i. They + * must be processed at some point so that desired regions are marked as + * "included" in the rom_entries list. + */ +static char *include_args[MAX_ROMLAYOUT]; +static int num_include_args = 0; /* the number of valid entries. */ static romlayout_t rom_entries[MAX_ROMLAYOUT]; #if CONFIG_INTERNAL == 1 /* FIXME: Move the whole block to cbtable.c? */ @@ -194,6 +200,20 @@ int read_romlayout(char *name) } #endif +/* register an include argument (-i) for later processing */ +int register_include_arg(char *name) +{ + if (num_include_args >= MAX_ROMLAYOUT) { + msg_gerr("Too many regions included (%i).\n", num_include_args); + return -1; + } + + include_args[num_include_args] = name; + num_include_args++; + return num_include_args; +} + +/* returns the index of the entry (or a negative value if it is not found) */ int find_romentry(char *name) { int i; @@ -201,20 +221,49 @@ int find_romentry(char *name) if (!romimages) return -1; - msg_ginfo("Looking for \"%s\"... ", name); - + msg_gspew("Looking for region \"%s\"... ", name); for (i = 0; i < romimages; i++) { if (!strcmp(rom_entries[i].name, name)) { rom_entries[i].included = 1; - msg_ginfo("found.\n"); + msg_gspew("found.\n"); return i; } } - msg_ginfo("not found.\n"); // Not found. Error. - + msg_gspew("not found.\n"); return -1; } +/* process -i arguments + * returns 0 to indicate success, <0 to indicate failure + */ +int process_include_args(void) +{ + int i; + unsigned int found = 0; + for (i = 0; i < num_include_args && include_args[i] != NULL; i++) { + /* User has specified an area, but no layout file is loaded. */ + if (!romimages) { + msg_gerr("Region requested (with -i \"%s\"), " + "but no layout data is available.\n", + include_args[i]); + return -1; + } + + if (find_romentry(include_args[i]) < 0) { + msg_gerr("Invalid region specified: \"%s\"\n", + include_args[i]); + return -1; + } + if (found == 0) + msg_ginfo("Using region(s): \"%s\"", include_args[i]); + else + msg_ginfo(", \"%s\"", include_args[i]); + found++; + } + msg_ginfo(".\n"); + return 0; +} + int find_next_included_romentry(unsigned int start) { int i; @@ -246,11 +295,12 @@ int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *ne int entry; unsigned int size = flash->total_size * 1024; - /* If no layout file was specified or the layout file was empty, assume - * that the user wants to flash the complete new image. + /* If no regions were specified for inclusion, assume + * that the user wants to write the complete new image. */ - if (!romimages) + if (num_include_args == 0) return 0; + /* Non-included romentries are ignored. * The union of all included romentries is used from the new image. */ @@ -262,6 +312,8 @@ int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *ne size - start); break; } + + /* For non-included region, copy from old content. */ if (rom_entries[entry].start > start) memcpy(newcontents + start, oldcontents + start, rom_entries[entry].start - start); -- 1.7.1
Oh well... new iteration, with some of the suggestions merged. This still is not final, but it should be a bit closer. Am 28.07.2011 00:35 schrieb Carl-Daniel Hailfinger:
Ouch, that one was in my draft folder, and I wondered why I didn't see an answer.
Am 19.06.2011 14:44 schrieb Stefan Tauner:
after an IRC discussion with carldani i propose the following solution. maybe not mergeable, but you will get the idea.
the current order of patches in my volt+log branch is: 1. patchset voltage printing 2.1 2. rebased add log file support 3. attached patch.
i can repost the whole series if you like.
From: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> Date: Sun, 19 Jun 2011 14:39:37 +0200 Subject: [PATCH] squash! Add log file support to flashrom.
- instead of the want_log variable "print" will always write to the log if a log file is open (indicated by logfile != NULL)
If we don't have to push screen-invisible messages to the logfile, that's indeed an option.
- print_version is no longer executed "implicitly" before argument parsing
This will cause the version to be printed multiple times if someone specifies -h -R or some other special combination.
- cli_classic uses the "goto cleanup"-pattern where it closes the log file
Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
diff --git a/cli_classic.c b/cli_classic.c index ed36c85..71f8bec 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -147,9 +149,6 @@ int cli_classic(int argc, char *argv[]) char *tempstr = NULL; char *pparam = NULL;
- print_version(); - print_banner(); -
If the selfcheck fails, we won't see any version message.
if (selfcheck()) exit(1);
@@ -311,14 +311,18 @@ int cli_classic(int argc, char *argv[]) exit(0); break; case 'o': - tempstr = strdup(optarg); #ifdef STANDALONE fprintf(stderr, "Log file not supported in standalone " "mode. Aborting.\n"); + cli_classic_abort_usage(); #else /* STANDALONE */ - if (open_logfile(tempstr)) -#endif /* STANDALONE */ + log_name = strdup(optarg);
strdup(NULL) is allowed to explode. However, AFAICS optarg is guaranteed not to be NULL, so it should work out fine.
+ if (log_name == NULL || log_name[0] == '\0') {
This strikes me as odd. The "can we open the file" check is postponed, but the "did the user specify a filename" check is still in here.
+ fprintf(stderr, + "No log file name specified.\n"); cli_classic_abort_usage(); + } +#endif /* STANDALONE */ break; default: cli_classic_abort_usage(); @@ -326,18 +330,6 @@ int cli_classic(int argc, char *argv[]) } }
- if (list_supported) { - print_supported(); - exit(0); - } - -#if CONFIG_PRINT_WIKI == 1 - if (list_supported_wiki) { - print_supported_wiki(); - exit(0); - } -#endif - if (optind < argc) { fprintf(stderr, "Error: Extra parameter found.\n"); cli_classic_abort_usage(); @@ -351,6 +343,26 @@ int cli_classic(int argc, char *argv[]) } #endif
+#if CONFIG_PRINT_WIKI == 1 + if (list_supported_wiki) { + print_supported_wiki(); + return 0; + } +#endif
Why did you place the print_supported_wiki() call before opening the logfile, but print_supported() after opening the logfile? I touched that ordering in r1373, hopefully the current state is more agreeable for you.
+ +#ifndef STANDALONE + if (open_logfile(log_name)) + return 1; +#endif /* !STANDALONE */ + + print_version(); + print_banner(); + + if (list_supported) { + print_supported(); + goto cleanup; + } + if (chip_to_probe) { for (flash = flashchips; flash && flash->name; flash++) if (!strcmp(flash->name, chip_to_probe))
goto cleanup makes sense, but I'd handle only cases which need programmer_shutdown... that would kill quite a few duplicates. This part is now semi-obsolete since r1373.
Current state of this patch straight from my development tree. Note: man page is unchanged, needs to get some documentation as well. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-logfile/flash.h =================================================================== --- flashrom-logfile/flash.h (Revision 1483) +++ flashrom-logfile/flash.h (Arbeitskopie) @@ -265,6 +265,12 @@ #define ERROR_FLASHROM_LIMIT -201 /* cli_output.c */ +#ifndef STANDALONE +int open_logfile(const char * const filename); +int close_logfile(void); +void start_logging(void); +#endif +int msg_log(const char *fmt, ...); /* Let gcc and clang check for correct printf-style format strings. */ int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); #define MSG_ERROR 0 Index: flashrom-logfile/cli_output.c =================================================================== --- flashrom-logfile/cli_output.c (Revision 1483) +++ flashrom-logfile/cli_output.c (Arbeitskopie) @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright (C) 2009 Sean Nelson <audiohacked@gmail.com> + * Copyright (C) 2011 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -22,34 +23,101 @@ #include <stdarg.h> #include "flash.h" -int print(int type, const char *fmt, ...) +static FILE *logfile = NULL; +static int want_log = 0; + +#ifndef STANDALONE +int close_logfile(void) { + if (logfile && fclose(logfile)) + return 1; + return 0; +} + +int open_logfile(const char * const filename) +{ + if (!filename) { + msg_gerr("No filename specified.\n"); + return 1; + } + if ((logfile = fopen(filename, "w")) == NULL) { + perror(filename); + return 1; + } + return 0; +} + +void start_logging(void) +{ + int oldverbose = verbose; + + want_log = 1; + /* Shut up the console. */ + verbose = -2; + print_version(); + verbose = oldverbose; +} +#endif /* STANDALONE */ + +int msg_log(const char *fmt, ...) +{ va_list ap; int ret; - FILE *output_type; + if (!logfile) + return -1; + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + return ret; +} + +int print(int type, const char *fmt, ...) +{ + va_list ap; + int ret = 0; + int want_screen = 1; + int want_file = 1; + FILE *output_type = stdout; + switch (type) { - case MSG_ERROR: - output_type = stderr; + case MSG_BARF: + if (verbose < 3) { + want_screen = 0; + want_file = 0; + } break; - case MSG_BARF: - if (verbose < 3) - return 0; case MSG_DEBUG2: if (verbose < 2) - return 0; + want_screen = 0; + break; case MSG_DEBUG: if (verbose < 1) - return 0; + want_screen = 0; + break; case MSG_INFO: + if (verbose < 0) + want_screen = 0; + break; + case MSG_ERROR: + if (verbose < -1) + want_screen = 0; + output_type = stderr; + break; default: - output_type = stdout; break; } - va_start(ap, fmt); - ret = vfprintf(output_type, fmt, ap); - va_end(ap); + if (want_screen) { + va_start(ap, fmt); + ret = vfprintf(output_type, fmt, ap); + va_end(ap); + } + if (want_file && logfile && want_log) { + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + } fflush(output_type); return ret; } Index: flashrom-logfile/cli_classic.c =================================================================== --- flashrom-logfile/cli_classic.c (Revision 1483) +++ flashrom-logfile/cli_classic.c (Arbeitskopie) @@ -106,7 +106,7 @@ "-z|" #endif "-E|-r <file>|-w <file>|-v <file>]\n" - " [-c <chipname>] [-l <file>]\n" + " [-c <chipname>] [-l <file>] [-o <file>]\n" " [-i <image>] [-p <programmername>[:<parameters>]]\n\n"); printf("Please note that the command line interface for flashrom has " @@ -135,6 +135,7 @@ "<file>\n" " -i | --image <name> only flash image <name> " "from flash layout\n" + " -o | --output <name> log to file <name>\n" " -L | --list-supported print supported devices\n" #if CONFIG_PRINT_WIKI == 1 " -z | --list-supported-wiki print supported devices " @@ -177,7 +178,7 @@ enum programmer prog = PROGRAMMER_INVALID; int ret = 0; - static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh"; + static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzho:"; static const struct option long_options[] = { {"read", 1, NULL, 'r'}, {"write", 1, NULL, 'w'}, @@ -194,10 +195,12 @@ {"programmer", 1, NULL, 'p'}, {"help", 0, NULL, 'h'}, {"version", 0, NULL, 'R'}, + {"output", 1, NULL, 'o'}, {NULL, 0, NULL, 0}, }; char *filename = NULL; + char *log_name = NULL; char *tempstr = NULL; char *pparam = NULL; @@ -362,6 +365,19 @@ cli_classic_usage(argv[0]); exit(0); break; + case 'o': +#ifdef STANDALONE + fprintf(stderr, "Log file not supported in standalone " + "mode. Aborting.\n"); + cli_classic_abort_usage(); +#else /* STANDALONE */ + log_name = strdup(optarg); + if (log_name[0] == '\0') { + fprintf(stderr, "No log filename specified.\n"); + cli_classic_abort_usage(); + } +#endif /* STANDALONE */ + break; default: cli_classic_abort_usage(); break; @@ -373,10 +389,15 @@ cli_classic_abort_usage(); } - if (process_include_args()) + if (filename && (filename[0] == '\0')) { + fprintf(stderr, "Error: No filename specified.\n"); cli_classic_abort_usage(); + } - /* FIXME: Print the actions flashrom will take. */ +#ifndef STANDALONE + if (open_logfile(log_name)) + return 1; +#endif /* !STANDALONE */ if (list_supported) { print_supported(); @@ -390,6 +411,9 @@ } #endif + if (process_include_args()) + cli_classic_abort_usage(); + /* Does a chip with the requested name exist in the flashchips array? */ if (chip_to_probe) { for (flash = flashchips; flash && flash->name; flash++) @@ -409,11 +433,23 @@ if (prog == PROGRAMMER_INVALID) prog = default_programmer; + // FIXME: Kill --mainboard completely +#ifndef STANDALONE + start_logging(); +#endif /* STANDALONE */ + + msg_gdbg("Command line:"); + for (i = 0; i < argc; i++) { + msg_gdbg(" %s", argv[i]); + } + msg_gdbg("\n"); + + /* FIXME: Delay calibration should happen in programmer code. */ myusec_calibrate_delay(); if (programmer_init(prog, pparam)) { - fprintf(stderr, "Error: Programmer initialization failed.\n"); + msg_perr("Error: Programmer initialization failed.\n"); ret = 1; goto out_shutdown; } @@ -436,34 +472,35 @@ } if (chipcount > 1) { - printf("Multiple flash chips were detected: \"%s\"", - flashes[0].name); + msg_cinfo("Multiple flash chips were detected: \"%s\"", + flashes[0].name); for (i = 1; i < chipcount; i++) - printf(", \"%s\"", flashes[i].name); - printf("\nPlease specify which chip to use with the " - "-c <chipname> option.\n"); + msg_cinfo(", \"%s\"", flashes[i].name); + msg_cinfo("\nPlease specify which chip to use with the -c " + "<chipname> option.\n"); ret = 1; goto out_shutdown; } else if (!chipcount) { - printf("No EEPROM/flash device found.\n"); + msg_cinfo("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { - printf("Note: flashrom can never write if the flash " - "chip isn't found automatically.\n"); + msg_cinfo("Note: flashrom can never write if the flash " + "chip isn't found automatically.\n"); } #if 0 // FIXME: What happens for a forced chip read if multiple compatible programmers are registered? if (force && read_it && chip_to_probe) { - printf("Force read (-f -r -c) requested, pretending " - "the chip is there:\n"); + msg_cinfo("Force read (-f -r -c) requested, pretending " + "the chip is there:\n"); startchip = probe_flash(0, &flashes[0], 1); if (startchip == -1) { - printf("Probing for flash chip '%s' failed.\n", - chip_to_probe); + msg_cinfo("Probing for flash chip '%s' failed." + "\n", chip_to_probe); ret = 1; goto out_shutdown; } - printf("Please note that forced reads most likely " - "contain garbage.\n"); - return read_flash_to_file(&flashes[0], filename); + msg_cinfo("Please note that forced reads most likely " + "contain garbage.\n"); + ret = read_flash_to_file(&flashes[0], filename); + goto out_shutdown; } #endif ret = 1; @@ -484,23 +521,17 @@ size = fill_flash->total_size * 1024; if (check_max_decode(fill_flash->pgm->buses_supported & fill_flash->bustype, size) && (!force)) { - fprintf(stderr, "Chip is too big for this programmer " - "(-V gives details). Use --force to override.\n"); + 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)) { - printf("No operations were specified.\n"); + msg_ginfo("No operations were specified.\n"); goto out_shutdown; } - if (!filename && !erase_it) { - printf("Error: No filename specified.\n"); - ret = 1; - goto out_shutdown; - } - /* Always verify write operations unless -n is used. */ if (write_it && !dont_verify_it) verify_it = 1; @@ -510,9 +541,13 @@ * Give the chip time to settle. */ programmer_delay(100000); - return doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); out_shutdown: programmer_shutdown(); +out: +#ifndef STANDALONE + ret |= close_logfile(); +#endif return ret; } Index: flashrom-logfile/flashrom.c =================================================================== --- flashrom-logfile/flashrom.c (Revision 1483) +++ flashrom-logfile/flashrom.c (Arbeitskopie) @@ -1422,27 +1422,27 @@ if (firstline) firstline = 0; else - printf("\n"); + msg_ginfo("\n"); for (i = 0; i < startcol; i++) - printf(" "); + msg_ginfo(" "); remaining = cols - startcol; } else { - printf(" "); + msg_ginfo(" "); remaining--; } if (paren && (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 { if (paren) - printf(")"); - printf("\n"); + msg_ginfo(")"); + msg_ginfo("\n"); } } } @@ -1458,35 +1458,35 @@ #else msg_ginfo(" on unknown machine"); #endif - msg_ginfo(", built with"); + msg_gdbg(", built with"); #if NEED_PCI == 1 #ifdef PCILIB_VERSION - msg_ginfo(" libpci %s,", PCILIB_VERSION); + msg_gdbg(" libpci %s,", PCILIB_VERSION); #else - msg_ginfo(" unknown PCI library,"); + msg_gdbg(" unknown PCI library,"); #endif #endif #ifdef __clang__ - msg_ginfo(" LLVM Clang"); + msg_gdbg(" LLVM Clang"); #ifdef __clang_version__ - msg_ginfo(" %s,", __clang_version__); + msg_gdbg(" %s,", __clang_version__); #else - msg_ginfo(" unknown version (before r102686),"); + msg_gdbg(" unknown version (before r102686),"); #endif #elif defined(__GNUC__) - msg_ginfo(" GCC"); + msg_gdbg(" GCC"); #ifdef __VERSION__ - msg_ginfo(" %s,", __VERSION__); + msg_gdbg(" %s,", __VERSION__); #else - msg_ginfo(" unknown version,"); + msg_gdbg(" unknown version,"); #endif #else - msg_ginfo(" unknown compiler,"); + msg_gdbg(" unknown compiler,"); #endif #if defined (__FLASHROM_LITTLE_ENDIAN__) - msg_ginfo(" little endian"); + msg_gdbg(" little endian"); #else - msg_ginfo(" big endian"); + msg_gdbg(" big endian"); #endif msg_ginfo("\n"); } @@ -1499,7 +1499,7 @@ void print_banner(void) { - msg_ginfo("flashrom is free software, get the source code at " + msg_ginfo("flashrom is free software. Get the source code at " "http://www.flashrom.org\n"); msg_ginfo("\n"); } @@ -1798,6 +1798,5 @@ free(oldcontents); free(newcontents); out_nofree: - programmer_shutdown(); return ret; } -- http://www.hailfinger.org/
On Wed, 04 Jan 2012 02:08:06 +0100 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
Oh well... new iteration, with some of the suggestions merged. This still is not final, but it should be a bit closer.
@@ -510,9 +541,13 @@ * Give the chip time to settle. */ programmer_delay(100000); - return doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it);
out_shutdown: programmer_shutdown(); +out:
^ unused (hence breaking compilation)
+#ifndef STANDALONE + ret |= close_logfile(); +#endif return ret;
also i see a little problem with this... ;) ./flashrom flashrom v0.9.4-r1483 on Linux 2.6.35-32-generic (x86_64) flashrom is free software. Get the source code at http://www.flashrom.org No filename specified. attached is your patch rebased to r1490 without "out:" maybe we can get this into 0.9.5 after some rework? do you have a todo for it? -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
I have decided to discard most of the logging code and rewrite it to be more readable. It even worked in preliminary tests. TODO: - Should "-o -" be a no-op which just redirects stderr to stdout? Right now "-" is treated as regular file name, but most Unix utilities are unable to handle a file with that name (try "less -" for amusement). - Add man page entry - Is the log level difference for screen/logfile a good thing, and should we default the logfile level to dbg2 instead of dbg? Add log file support to flashrom. If you use cli_classic, the log file will always contain messages at a level which is one higher than the one specified. That way we get all verbose messages in the log even if the user doesn't specify -V. Convert all printf() after start of logging to msg_*(). Allow separate control of log level for screen and logfile in case other frontends (e.g. GUI, cli_mfg) want that. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-logfile/flash.h =================================================================== --- flashrom-logfile/flash.h (Revision 1528) +++ flashrom-logfile/flash.h (Arbeitskopie) @@ -228,7 +228,8 @@ write_gran_1byte, write_gran_256bytes, }; -extern int verbose; +extern int verbose_screen; +extern int verbose_logfile; extern const char flashrom_version[]; extern char *chip_to_probe; void map_flash_registers(struct flashctx *flash); @@ -268,13 +269,20 @@ #define ERROR_FLASHROM_LIMIT -201 /* cli_output.c */ +#ifndef STANDALONE +int open_logfile(const char * const filename); +int close_logfile(void); +void start_logging(void); +#endif /* Let gcc and clang check for correct printf-style format strings. */ -int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); -#define MSG_ERROR 0 -#define MSG_INFO 1 -#define MSG_DEBUG 2 -#define MSG_DEBUG2 3 -#define MSG_BARF 4 +enum msglevel { + MSG_ERROR = 0, + MSG_INFO = 1, + MSG_DEBUG = 2, + MSG_DEBUG2 = 3, + MSG_BARF = 4, +}; +int print(enum msglevel level, const char *fmt, ...) __attribute__((format(printf, 2, 3))); #define msg_gerr(...) print(MSG_ERROR, __VA_ARGS__) /* general errors */ #define msg_perr(...) print(MSG_ERROR, __VA_ARGS__) /* programmer errors */ #define msg_cerr(...) print(MSG_ERROR, __VA_ARGS__) /* chip errors */ Index: flashrom-logfile/cli_output.c =================================================================== --- flashrom-logfile/cli_output.c (Revision 1528) +++ flashrom-logfile/cli_output.c (Arbeitskopie) @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright (C) 2009 Sean Nelson <audiohacked@gmail.com> + * Copyright (C) 2011 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -20,36 +21,80 @@ #include <stdio.h> #include <stdarg.h> +#include <string.h> +#include <errno.h> #include "flash.h" -int print(int type, const char *fmt, ...) +static FILE *logfile = NULL; + +#ifndef STANDALONE +int close_logfile(void) { + if (logfile && fclose(logfile)) { + /* fclose returned an error. Stop writing to be safe. */ + logfile = NULL; + msg_perr("Closing the log file returned error %s\n", + strerror(errno)); + return 1; + } + logfile = NULL; + return 0; +} + +int open_logfile(const char * const filename) +{ + if (!filename) { + msg_gerr("No filename specified.\n"); + return 1; + } + if ((logfile = fopen(filename, "w")) == NULL) { + perror(filename); + return 1; + } + return 0; +} + +void start_logging(void) +{ + enum msglevel oldverbose_screen = verbose_screen; + enum msglevel oldverbose_logfile = verbose_logfile; + + /* Shut up the console. */ + verbose_screen = MSG_ERROR; + verbose_logfile = MSG_DEBUG; + print_version(); + verbose_screen = oldverbose_screen; + verbose_logfile = oldverbose_logfile; +} +#endif /* STANDALONE */ + +/* Please note that level is the verbosity, not the importance of the message. + */ +int print(enum msglevel level, const char *fmt, ...) +{ va_list ap; - int ret; - FILE *output_type; + int ret = 0; + FILE *output_level = stdout; - switch (type) { - case MSG_ERROR: - output_type = stderr; - break; - case MSG_BARF: - if (verbose < 3) - return 0; - case MSG_DEBUG2: - if (verbose < 2) - return 0; - case MSG_DEBUG: - if (verbose < 1) - return 0; - case MSG_INFO: - default: - output_type = stdout; - break; + if (level == MSG_ERROR) + output_level = stderr; + + if (level <= verbose_screen) { + va_start(ap, fmt); + ret = vfprintf(output_level, fmt, ap); + va_end(ap); + /* msg_*spew usually happens inside chip accessors in possibly + * time-critical operations. Don't slow them down by flushing. + */ + if (level != MSG_BARF) + fflush(output_level); } - - va_start(ap, fmt); - ret = vfprintf(output_type, fmt, ap); - va_end(ap); - fflush(output_type); + if ((level <= verbose_logfile) && logfile) { + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + if (level != MSG_BARF) + fflush(logfile); + } return ret; } Index: flashrom-logfile/cli_classic.c =================================================================== --- flashrom-logfile/cli_classic.c (Revision 1528) +++ flashrom-logfile/cli_classic.c (Arbeitskopie) @@ -106,7 +106,7 @@ "-z|" #endif "-E|-r <file>|-w <file>|-v <file>]\n" - " [-c <chipname>] [-l <file>]\n" + " [-c <chipname>] [-l <file>] [-o <file>]\n" " [-i <image>] [-p <programmername>[:<parameters>]]\n\n"); printf("Please note that the command line interface for flashrom has " @@ -135,6 +135,7 @@ "<file>\n" " -i | --image <name> only flash image <name> " "from flash layout\n" + " -o | --output <name> log to file <name>\n" " -L | --list-supported print supported devices\n" #if CONFIG_PRINT_WIKI == 1 " -z | --list-supported-wiki print supported devices " @@ -177,7 +178,7 @@ enum programmer prog = PROGRAMMER_INVALID; int ret = 0; - static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzh"; + static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzho:"; static const struct option long_options[] = { {"read", 1, NULL, 'r'}, {"write", 1, NULL, 'w'}, @@ -194,11 +195,13 @@ {"programmer", 1, NULL, 'p'}, {"help", 0, NULL, 'h'}, {"version", 0, NULL, 'R'}, + {"output", 1, NULL, 'o'}, {NULL, 0, NULL, 0}, }; char *filename = NULL; char *layoutfile = NULL; + char *log_name = NULL; char *tempstr = NULL; char *pparam = NULL; @@ -260,7 +263,8 @@ chip_to_probe = strdup(optarg); break; case 'V': - verbose++; + verbose_screen++; + verbose_logfile++; break; case 'E': if (++operation_specified > 1) { @@ -366,6 +370,19 @@ cli_classic_usage(argv[0]); exit(0); break; + case 'o': +#ifdef STANDALONE + fprintf(stderr, "Log file not supported in standalone " + "mode. Aborting.\n"); + cli_classic_abort_usage(); +#else /* STANDALONE */ + log_name = strdup(optarg); + if (log_name[0] == '\0') { + fprintf(stderr, "No log filename specified.\n"); + cli_classic_abort_usage(); + } +#endif /* STANDALONE */ + break; default: cli_classic_abort_usage(); break; @@ -377,20 +394,40 @@ cli_classic_abort_usage(); } - /* FIXME: Print the actions flashrom will take. */ - - if (list_supported) { - print_supported(); - exit(0); + if ((read_it | write_it | verify_it) && !filename) { + fprintf(stderr, "Error: No filename specified.\n"); + cli_classic_abort_usage(); } + if (filename && (filename[0] == '\0')) { + fprintf(stderr, "Error: No filename specified.\n"); + cli_classic_abort_usage(); + } + if (filename && (filename[0] == '-')) + fprintf(stderr, "Warning: Supplied file name starts with -\n"); +#ifndef STANDALONE + if (log_name && (log_name[0] == '-')) + fprintf(stderr, "Warning: Supplied file name starts with -\n"); + if (log_name && open_logfile(log_name)) + return 1; +#endif /* !STANDALONE */ + #if CONFIG_PRINT_WIKI == 1 if (list_supported_wiki) { print_supported_wiki(); - exit(0); + ret = 0; + goto out; } #endif + if (list_supported) { + print_supported(); + ret = 0; + goto out; + } + + // FIXME: cli_classic_abort_usage() does exit(1) and we rely on the OS + // to flush log file buffers. Is that behaviour guaranteed? if (layoutfile && read_romlayout(layoutfile)) cli_classic_abort_usage(); if (process_include_args()) @@ -415,11 +452,22 @@ if (prog == PROGRAMMER_INVALID) prog = default_programmer; +#ifndef STANDALONE + start_logging(); +#endif /* STANDALONE */ + + msg_gdbg("Command line:"); + for (i = 0; i < argc; i++) { + msg_gdbg(" %s", argv[i]); + } + msg_gdbg("\n"); + + /* FIXME: Delay calibration should happen in programmer code. */ myusec_calibrate_delay(); if (programmer_init(prog, pparam)) { - fprintf(stderr, "Error: Programmer initialization failed.\n"); + msg_perr("Error: Programmer initialization failed.\n"); ret = 1; goto out_shutdown; } @@ -442,25 +490,25 @@ } if (chipcount > 1) { - printf("Multiple flash chips were detected: \"%s\"", - flashes[0].name); + msg_cinfo("Multiple flash chips were detected: \"%s\"", + flashes[0].name); for (i = 1; i < chipcount; i++) - printf(", \"%s\"", flashes[i].name); - printf("\nPlease specify which chip to use with the " - "-c <chipname> option.\n"); + msg_cinfo(", \"%s\"", flashes[i].name); + msg_cinfo("\nPlease specify which chip to use with the -c " + "<chipname> option.\n"); ret = 1; goto out_shutdown; } else if (!chipcount) { - printf("No EEPROM/flash device found.\n"); + msg_cinfo("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { - printf("Note: flashrom can never write if the flash " - "chip isn't found automatically.\n"); + msg_cinfo("Note: flashrom can never write if the flash " + "chip isn't found automatically.\n"); } if (force && read_it && chip_to_probe) { struct registered_programmer *pgm; int compatible_programmers = 0; - printf("Force read (-f -r -c) requested, pretending " - "the chip is there:\n"); + msg_cinfo("Force read (-f -r -c) requested, pretending " + "the chip is there:\n"); /* This loop just counts compatible controllers. */ for (j = 0; j < registered_programmer_count; j++) { pgm = ®istered_programmers[j]; @@ -468,9 +516,9 @@ compatible_programmers++; } if (compatible_programmers > 1) - printf("More than one compatible controller " - "found for the requested flash chip, " - "using the first one.\n"); + msg_cinfo("More than one compatible controller " + "found for the requested flash chip, " + "using the first one.\n"); for (j = 0; j < registered_programmer_count; j++) { pgm = ®istered_programmers[j]; startchip = probe_flash(pgm, 0, &flashes[0], 1); @@ -478,14 +526,15 @@ break; } if (startchip == -1) { - printf("Probing for flash chip '%s' failed.\n", - chip_to_probe); + msg_cinfo("Probing for flash chip '%s' failed." + "\n", chip_to_probe); ret = 1; goto out_shutdown; } - printf("Please note that forced reads most likely " - "contain garbage.\n"); - return read_flash_to_file(&flashes[0], filename); + msg_cinfo("Please note that forced reads most likely " + "contain garbage.\n"); + ret = read_flash_to_file(&flashes[0], filename); + goto out_shutdown; } ret = 1; goto out_shutdown; @@ -505,23 +554,17 @@ size = fill_flash->total_size * 1024; if (check_max_decode(fill_flash->pgm->buses_supported & fill_flash->bustype, size) && (!force)) { - fprintf(stderr, "Chip is too big for this programmer " - "(-V gives details). Use --force to override.\n"); + 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)) { - printf("No operations were specified.\n"); + msg_ginfo("No operations were specified.\n"); goto out_shutdown; } - if (!filename && !erase_it) { - printf("Error: No filename specified.\n"); - ret = 1; - goto out_shutdown; - } - /* Always verify write operations unless -n is used. */ if (write_it && !dont_verify_it) verify_it = 1; @@ -531,9 +574,13 @@ * Give the chip time to settle. */ programmer_delay(100000); - return doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); out_shutdown: programmer_shutdown(); +out: +#ifndef STANDALONE + ret |= close_logfile(); +#endif return ret; } Index: flashrom-logfile/flashrom.c =================================================================== --- flashrom-logfile/flashrom.c (Revision 1528) +++ flashrom-logfile/flashrom.c (Arbeitskopie) @@ -40,7 +40,8 @@ const char flashrom_version[] = FLASHROM_VERSION; char *chip_to_probe = NULL; -int verbose = 0; +int verbose_screen = MSG_INFO; +int verbose_logfile = MSG_DEBUG; static enum programmer programmer = PROGRAMMER_INVALID; @@ -1457,27 +1458,27 @@ if (firstline) firstline = 0; else - printf("\n"); + msg_ginfo("\n"); for (i = 0; i < startcol; i++) - printf(" "); + msg_ginfo(" "); remaining = cols - startcol; } else { - printf(" "); + msg_ginfo(" "); remaining--; } if (paren && (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 { if (paren) - printf(")"); - printf("\n"); + msg_ginfo(")"); + msg_ginfo("\n"); } } } @@ -1493,35 +1494,35 @@ #else msg_ginfo(" on unknown machine"); #endif - msg_ginfo(", built with"); + msg_gdbg(", built with"); #if NEED_PCI == 1 #ifdef PCILIB_VERSION - msg_ginfo(" libpci %s,", PCILIB_VERSION); + msg_gdbg(" libpci %s,", PCILIB_VERSION); #else - msg_ginfo(" unknown PCI library,"); + msg_gdbg(" unknown PCI library,"); #endif #endif #ifdef __clang__ - msg_ginfo(" LLVM Clang"); + msg_gdbg(" LLVM Clang"); #ifdef __clang_version__ - msg_ginfo(" %s,", __clang_version__); + msg_gdbg(" %s,", __clang_version__); #else - msg_ginfo(" unknown version (before r102686),"); + msg_gdbg(" unknown version (before r102686),"); #endif #elif defined(__GNUC__) - msg_ginfo(" GCC"); + msg_gdbg(" GCC"); #ifdef __VERSION__ - msg_ginfo(" %s,", __VERSION__); + msg_gdbg(" %s,", __VERSION__); #else - msg_ginfo(" unknown version,"); + msg_gdbg(" unknown version,"); #endif #else - msg_ginfo(" unknown compiler,"); + msg_gdbg(" unknown compiler,"); #endif #if defined (__FLASHROM_LITTLE_ENDIAN__) - msg_ginfo(" little endian"); + msg_gdbg(" little endian"); #else - msg_ginfo(" big endian"); + msg_gdbg(" big endian"); #endif msg_ginfo("\n"); } @@ -1534,7 +1535,7 @@ void print_banner(void) { - msg_ginfo("flashrom is free software, get the source code at " + msg_ginfo("flashrom is free software. Get the source code at " "http://www.flashrom.org\n"); msg_ginfo("\n"); } @@ -1840,6 +1841,5 @@ free(oldcontents); free(newcontents); out_nofree: - programmer_shutdown(); return ret; } -- http://www.hailfinger.org/
2012/5/3 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>:
I have decided to discard most of the logging code and rewrite it to be more readable. It even worked in preliminary tests. TODO: - Should "-o -" be a no-op which just redirects stderr to stdout? Right now "-" is treated as regular file name, but most Unix utilities are unable to handle a file with that name (try "less -" for amusement).
Is this a part of your long-term plan?
- Add man page entry - Is the log level difference for screen/logfile a good thing, and should we default the logfile level to dbg2 instead of dbg?
Add log file support to flashrom.
If you use cli_classic, the log file will always contain messages at a level which is one higher than the one specified. That way we get all verbose messages in the log even if the user doesn't specify -V.
This handles -VV and -VVV too, correct?
Convert all printf() after start of logging to msg_*().
Allow separate control of log level for screen and logfile in case other frontends (e.g. GUI, cli_mfg) want that.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Index: flashrom-logfile/flash.h =================================================================== --- flashrom-logfile/flash.h (Revision 1528) +++ flashrom-logfile/flash.h (Arbeitskopie) @@ -228,7 +228,8 @@ write_gran_1byte, write_gran_256bytes, }; -extern int verbose; +extern int verbose_screen; +extern int verbose_logfile; extern const char flashrom_version[]; extern char *chip_to_probe; void map_flash_registers(struct flashctx *flash); @@ -268,13 +269,20 @@ #define ERROR_FLASHROM_LIMIT -201
/* cli_output.c */ +#ifndef STANDALONE +int open_logfile(const char * const filename); +int close_logfile(void); +void start_logging(void); +#endif /* Let gcc and clang check for correct printf-style format strings. */ -int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); -#define MSG_ERROR 0 -#define MSG_INFO 1 -#define MSG_DEBUG 2 -#define MSG_DEBUG2 3
http://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/include/consol... #define MSG_NOTICE 2 #define MSG_DEBUG 3
-#define MSG_BARF 4
#define MSG_SPEW 4
+enum msglevel { + MSG_ERROR = 0, + MSG_INFO = 1, + MSG_DEBUG = 2, + MSG_DEBUG2 = 3, + MSG_BARF = 4, +}; +int print(enum msglevel level, const char *fmt, ...) __attribute__((format(printf, 2, 3))); #define msg_gerr(...) print(MSG_ERROR, __VA_ARGS__) /* general errors */ #define msg_perr(...) print(MSG_ERROR, __VA_ARGS__) /* programmer errors */ #define msg_cerr(...) print(MSG_ERROR, __VA_ARGS__) /* chip errors */ Index: flashrom-logfile/cli_output.c =================================================================== --- flashrom-logfile/cli_output.c (Revision 1528) +++ flashrom-logfile/cli_output.c (Arbeitskopie) @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright (C) 2009 Sean Nelson <audiohacked@gmail.com> + * Copyright (C) 2011 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -20,36 +21,80 @@
#include <stdio.h> #include <stdarg.h> +#include <string.h> +#include <errno.h> #include "flash.h"
-int print(int type, const char *fmt, ...) +static FILE *logfile = NULL; + +#ifndef STANDALONE +int close_logfile(void) { + if (logfile && fclose(logfile)) {
No sync() / fsync() ? From http://linux.die.net/man/3/fclose "Note that fclose() only flushes the user space buffers provided by the C library. To ensure that the data is physically stored on disk the kernel buffers must be flushed too, for example, with sync(2) or fsync(2)."
+ /* fclose returned an error. Stop writing to be safe. */ + logfile = NULL; + msg_perr("Closing the log file returned error %s\n", + strerror(errno)); + return 1; + } + logfile = NULL; + return 0; +} + +int open_logfile(const char * const filename) +{ + if (!filename) { + msg_gerr("No filename specified.\n"); + return 1; + } + if ((logfile = fopen(filename, "w")) == NULL) { + perror(filename); + return 1; + } + return 0; +} + +void start_logging(void) +{ + enum msglevel oldverbose_screen = verbose_screen; + enum msglevel oldverbose_logfile = verbose_logfile; + + /* Shut up the console. */ + verbose_screen = MSG_ERROR; + verbose_logfile = MSG_DEBUG; + print_version(); + verbose_screen = oldverbose_screen; + verbose_logfile = oldverbose_logfile; +} +#endif /* STANDALONE */ + +/* Please note that level is the verbosity, not the importance of the message. + */ +int print(enum msglevel level, const char *fmt, ...) +{ va_list ap; - int ret; - FILE *output_type; + int ret = 0; + FILE *output_level = stdout;
- switch (type) { - case MSG_ERROR: - output_type = stderr; - break; - case MSG_BARF: - if (verbose < 3) - return 0; - case MSG_DEBUG2: - if (verbose < 2) - return 0; - case MSG_DEBUG: - if (verbose < 1) - return 0; - case MSG_INFO: - default: - output_type = stdout; - break; + if (level == MSG_ERROR) + output_level = stderr; + + if (level <= verbose_screen) { + va_start(ap, fmt); + ret = vfprintf(output_level, fmt, ap); + va_end(ap); + /* msg_*spew usually happens inside chip accessors in possibly + * time-critical operations. Don't slow them down by flushing. + */ + if (level != MSG_BARF) + fflush(output_level); } - - va_start(ap, fmt); - ret = vfprintf(output_type, fmt, ap); - va_end(ap); - fflush(output_type); + if ((level <= verbose_logfile) && logfile) { + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + if (level != MSG_BARF) + fflush(logfile); + } return ret; } Index: flashrom-logfile/cli_classic.c =================================================================== --- flashrom-logfile/cli_classic.c (Revision 1528) +++ flashrom-logfile/cli_classic.c (Arbeitskopie) @@ -106,7 +106,7 @@ "-z|" #endif "-E|-r <file>|-w <file>|-v <file>]\n" - " [-c <chipname>] [-l <file>]\n" + " [-c <chipname>] [-l <file>] [-o <file>]\n" " [-i <image>] [-p <programmername>[:<parameters>]]\n\n");
printf("Please note that the command line interface for flashrom has " @@ -135,6 +135,7 @@ "<file>\n" " -i | --image <name> only flash image <name> " "from flash layout\n" + " -o | --output <name> log to file <name>\n" " -L | --list-supported print supported devices\n" #if CONFIG_PRINT_WIKI == 1 " -z | --list-supported-wiki print supported devices " @@ -177,7 +178,7 @@ enum programmer prog = PROGRAMMER_INVALID; int ret = 0;
- static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzh"; + static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzho:"; static const struct option long_options[] = { {"read", 1, NULL, 'r'}, {"write", 1, NULL, 'w'}, @@ -194,11 +195,13 @@ {"programmer", 1, NULL, 'p'}, {"help", 0, NULL, 'h'}, {"version", 0, NULL, 'R'}, + {"output", 1, NULL, 'o'}, {NULL, 0, NULL, 0}, };
char *filename = NULL; char *layoutfile = NULL; + char *log_name = NULL; char *tempstr = NULL; char *pparam = NULL;
@@ -260,7 +263,8 @@ chip_to_probe = strdup(optarg); break; case 'V': - verbose++; + verbose_screen++; + verbose_logfile++; break; case 'E': if (++operation_specified > 1) { @@ -366,6 +370,19 @@ cli_classic_usage(argv[0]); exit(0); break; + case 'o': +#ifdef STANDALONE + fprintf(stderr, "Log file not supported in standalone " + "mode. Aborting.\n"); + cli_classic_abort_usage(); +#else /* STANDALONE */ + log_name = strdup(optarg); + if (log_name[0] == '\0') { + fprintf(stderr, "No log filename specified.\n"); + cli_classic_abort_usage(); + } +#endif /* STANDALONE */ + break; default: cli_classic_abort_usage(); break; @@ -377,20 +394,40 @@ cli_classic_abort_usage(); }
- /* FIXME: Print the actions flashrom will take. */ - - if (list_supported) { - print_supported(); - exit(0); + if ((read_it | write_it | verify_it) && !filename) { + fprintf(stderr, "Error: No filename specified.\n");
"No log filename specified.\n" ?
+ cli_classic_abort_usage(); } + if (filename && (filename[0] == '\0')) { + fprintf(stderr, "Error: No filename specified.\n");
dito
+ cli_classic_abort_usage(); + } + if (filename && (filename[0] == '-')) + fprintf(stderr, "Warning: Supplied file name starts with -\n");
+#ifndef STANDALONE + if (log_name && (log_name[0] == '-')) + fprintf(stderr, "Warning: Supplied file name starts with -\n"); + if (log_name && open_logfile(log_name))
WARNING or Warning? Maybe add a line that says that this will change in the future?
+ return 1; +#endif /* !STANDALONE */ + #if CONFIG_PRINT_WIKI == 1 if (list_supported_wiki) { print_supported_wiki(); - exit(0); + ret = 0; + goto out; } #endif
+ if (list_supported) { + print_supported(); + ret = 0; + goto out; + } + + // FIXME: cli_classic_abort_usage() does exit(1) and we rely on the OS + // to flush log file buffers. Is that behaviour guaranteed? if (layoutfile && read_romlayout(layoutfile)) cli_classic_abort_usage(); if (process_include_args()) @@ -415,11 +452,22 @@ if (prog == PROGRAMMER_INVALID) prog = default_programmer;
+#ifndef STANDALONE + start_logging(); +#endif /* STANDALONE */ + + msg_gdbg("Command line:"); + for (i = 0; i < argc; i++) { + msg_gdbg(" %s", argv[i]); + } + msg_gdbg("\n"); + + /* FIXME: Delay calibration should happen in programmer code. */ myusec_calibrate_delay();
if (programmer_init(prog, pparam)) { - fprintf(stderr, "Error: Programmer initialization failed.\n"); + msg_perr("Error: Programmer initialization failed.\n"); ret = 1; goto out_shutdown; } @@ -442,25 +490,25 @@ }
if (chipcount > 1) { - printf("Multiple flash chips were detected: \"%s\"", - flashes[0].name); + msg_cinfo("Multiple flash chips were detected: \"%s\"", + flashes[0].name); for (i = 1; i < chipcount; i++) - printf(", \"%s\"", flashes[i].name); - printf("\nPlease specify which chip to use with the " - "-c <chipname> option.\n"); + msg_cinfo(", \"%s\"", flashes[i].name); + msg_cinfo("\nPlease specify which chip to use with the -c " + "<chipname> option.\n"); ret = 1; goto out_shutdown; } else if (!chipcount) { - printf("No EEPROM/flash device found.\n"); + msg_cinfo("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { - printf("Note: flashrom can never write if the flash " - "chip isn't found automatically.\n"); + msg_cinfo("Note: flashrom can never write if the flash " + "chip isn't found automatically.\n"); } if (force && read_it && chip_to_probe) { struct registered_programmer *pgm; int compatible_programmers = 0; - printf("Force read (-f -r -c) requested, pretending " - "the chip is there:\n"); + msg_cinfo("Force read (-f -r -c) requested, pretending " + "the chip is there:\n"); /* This loop just counts compatible controllers. */ for (j = 0; j < registered_programmer_count; j++) { pgm = ®istered_programmers[j]; @@ -468,9 +516,9 @@ compatible_programmers++; } if (compatible_programmers > 1) - printf("More than one compatible controller " - "found for the requested flash chip, " - "using the first one.\n"); + msg_cinfo("More than one compatible controller " + "found for the requested flash chip, " + "using the first one.\n"); for (j = 0; j < registered_programmer_count; j++) { pgm = ®istered_programmers[j]; startchip = probe_flash(pgm, 0, &flashes[0], 1); @@ -478,14 +526,15 @@ break; } if (startchip == -1) { - printf("Probing for flash chip '%s' failed.\n", - chip_to_probe); + msg_cinfo("Probing for flash chip '%s' failed." + "\n", chip_to_probe); ret = 1; goto out_shutdown; } - printf("Please note that forced reads most likely " - "contain garbage.\n"); - return read_flash_to_file(&flashes[0], filename); + msg_cinfo("Please note that forced reads most likely " + "contain garbage.\n"); + ret = read_flash_to_file(&flashes[0], filename); + goto out_shutdown; } ret = 1; goto out_shutdown; @@ -505,23 +554,17 @@ size = fill_flash->total_size * 1024; if (check_max_decode(fill_flash->pgm->buses_supported & fill_flash->bustype, size) && (!force)) { - fprintf(stderr, "Chip is too big for this programmer " - "(-V gives details). Use --force to override.\n"); + 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)) { - printf("No operations were specified.\n"); + msg_ginfo("No operations were specified.\n"); goto out_shutdown; }
- if (!filename && !erase_it) { - printf("Error: No filename specified.\n"); - ret = 1; - goto out_shutdown; - } - /* Always verify write operations unless -n is used. */ if (write_it && !dont_verify_it) verify_it = 1; @@ -531,9 +574,13 @@ * Give the chip time to settle. */ programmer_delay(100000); - return doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); + ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it);
out_shutdown: programmer_shutdown(); +out: +#ifndef STANDALONE + ret |= close_logfile(); +#endif return ret; } Index: flashrom-logfile/flashrom.c =================================================================== --- flashrom-logfile/flashrom.c (Revision 1528) +++ flashrom-logfile/flashrom.c (Arbeitskopie) @@ -40,7 +40,8 @@
const char flashrom_version[] = FLASHROM_VERSION; char *chip_to_probe = NULL; -int verbose = 0; +int verbose_screen = MSG_INFO; +int verbose_logfile = MSG_DEBUG;
static enum programmer programmer = PROGRAMMER_INVALID;
@@ -1457,27 +1458,27 @@ if (firstline) firstline = 0; else - printf("\n"); + msg_ginfo("\n"); for (i = 0; i < startcol; i++) - printf(" "); + msg_ginfo(" "); remaining = cols - startcol; } else { - printf(" "); + msg_ginfo(" "); remaining--; } if (paren && (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 { if (paren) - printf(")"); - printf("\n"); + msg_ginfo(")"); + msg_ginfo("\n"); } } } @@ -1493,35 +1494,35 @@ #else msg_ginfo(" on unknown machine"); #endif - msg_ginfo(", built with"); + msg_gdbg(", built with"); #if NEED_PCI == 1 #ifdef PCILIB_VERSION - msg_ginfo(" libpci %s,", PCILIB_VERSION); + msg_gdbg(" libpci %s,", PCILIB_VERSION); #else - msg_ginfo(" unknown PCI library,"); + msg_gdbg(" unknown PCI library,"); #endif #endif #ifdef __clang__ - msg_ginfo(" LLVM Clang"); + msg_gdbg(" LLVM Clang"); #ifdef __clang_version__ - msg_ginfo(" %s,", __clang_version__); + msg_gdbg(" %s,", __clang_version__); #else - msg_ginfo(" unknown version (before r102686),"); + msg_gdbg(" unknown version (before r102686),"); #endif #elif defined(__GNUC__) - msg_ginfo(" GCC"); + msg_gdbg(" GCC"); #ifdef __VERSION__ - msg_ginfo(" %s,", __VERSION__); + msg_gdbg(" %s,", __VERSION__); #else - msg_ginfo(" unknown version,"); + msg_gdbg(" unknown version,"); #endif #else - msg_ginfo(" unknown compiler,"); + msg_gdbg(" unknown compiler,"); #endif #if defined (__FLASHROM_LITTLE_ENDIAN__) - msg_ginfo(" little endian"); + msg_gdbg(" little endian"); #else - msg_ginfo(" big endian"); + msg_gdbg(" big endian"); #endif msg_ginfo("\n"); } @@ -1534,7 +1535,7 @@
void print_banner(void) { - msg_ginfo("flashrom is free software, get the source code at " + msg_ginfo("flashrom is free software. Get the source code at " "http://www.flashrom.org\n"); msg_ginfo("\n"); } @@ -1840,6 +1841,5 @@ free(oldcontents); free(newcontents); out_nofree: - programmer_shutdown(); return ret; }
_______________________________________________ flashrom mailing list flashrom@flashrom.org http://www.flashrom.org/mailman/listinfo/flashrom
participants (5)
-
Bernd Blaauw -
Carl-Daniel Hailfinger -
Idwer Vollering -
Stefan Tauner -
Uwe Hermann