Am 10.05.2012 00:48 schrieb Carl-Daniel Hailfinger:
Am 09.05.2012 15:54 schrieb Stefan Tauner:
On Wed, 09 May 2012 15:16:04 +0200 Carl-Daniel Hailfinger wrote:
Am 08.05.2012 19:48 schrieb Idwer Vollering:
2012/5/3 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net:
- 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?
Yes. If a user specifies -VV, the log file will get -VVV output, but the on-screen info will stay manageable. Not sure how we should handle it when the user specifies -VVV... for now, that would set msglevel to 5 (outside the defined range of the enum, but handled implicitly by the code like msglevel 4).
We could make the log file always store logs at least at level -VV (to get all dbg2 output) regardless of whether -V was specified or not, and only if -VV or greater was specified, add 1 to the loglevel. What do you think?
i dont like that implicit "+1" verbosity in the current implementation. while non-verbose output is almost useless in the logfile, -VV and especially -VVV hides the interesting parts in most cases and should only be logged if the user really wants that much detail. i guess it might also create quite large log files in some cases.
i think the best solution would be a minimum verbosity of MSG_DEBUG in the log file, but without any other escalations.
Not DEBUG2? IIRC you once said that for ICHSPI debugging DEBUG2 produces better logs. And since we introduced DEBUG2, tha amount of msg_*dbg2 messages was kept pretty low. Of course SPEW is overkill for pretty much every log. I like your minimum verbosity proposal, but I'd pick DEBUG2 and provide SPEW both on-screen and in the log file only in case the user specifies -VVV.
This patch doesn't have the minimum verbosity handling yet, but I plan to change that.
an alternative would be a mandatory log file verbosity switch. carl-daniel once told me that he would like to see a UI to change the verbosity of the different log types (programmer, general, chip etc). so a switch for the log file itself does not seem to be that much out of proportion (although it makes the previously mentioned UI problem harder).
Log level control UI is an issue conceptually separate from log writing and I'd like to postpone this until the log writing review is pretty much done.
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?
I'd say "Warning" it totally OK... it's an indicator that the user did something unintended, but it's not an error per se.
we use both - WARNING and Warning - throughout the code. is that intended? if so what's the policy? if not then we should fix it. i think (without looking at any specific case) that WARNING is warranted because it sticks out more (especially in verbose outputs). as long as we dont want to play with bold or colored text... :)
<blink>WARNING</blink>
The point about "Warning" vs. "WARNING" is intricately linked to whether you believe there should be one or two levels of warnings ("retrying a different erase command" vs "your EC is stuck, and we just erased its firmware"). Even a really serious warning is not an error because flashrom may be theoretically able to fix this while it is still running. That's largely nitpicking, though. I have no really strong feelings about this.
Besides that, we need msg_*warn in addition to msg_*err.
Anyway, here is a new log file patch. It should work, and I hope I killed most/all of the controversial points. Well, except the programmer_shutdown changes which might be unnecessary with the new code flow introduced in the msg_* cleanup. The print_version() change is in this patch because it's a behavioural change needed for reasonable log file writing.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-logfile/flash.h =================================================================== --- flashrom-logfile/flash.h (Revision 1536) +++ 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,6 +269,11 @@ #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 enum msglevel { MSG_ERROR = 0, MSG_INFO = 1, Index: flashrom-logfile/cli_output.c =================================================================== --- flashrom-logfile/cli_output.c (Revision 1536) +++ 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,8 +21,53 @@
#include <stdio.h> #include <stdarg.h> +#include <string.h> +#include <errno.h> #include "flash.h"
+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, ...) { @@ -32,7 +78,7 @@ if (level == MSG_ERROR) output_type = stderr;
- if (level <= verbose) { + if (level <= verbose_screen) { va_start(ap, fmt); ret = vfprintf(output_type, fmt, ap); va_end(ap); @@ -42,5 +88,12 @@ if (level != MSG_SPEW) fflush(output_type); } + if ((level <= verbose_logfile) && logfile) { + va_start(ap, fmt); + ret = vfprintf(logfile, fmt, ap); + va_end(ap); + if (level != MSG_SPEW) + fflush(logfile); + } return ret; } Index: flashrom-logfile/cli_classic.c =================================================================== --- flashrom-logfile/cli_classic.c (Revision 1536) +++ 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 " @@ -189,7 +190,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'}, @@ -206,11 +207,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;
@@ -272,7 +275,8 @@ chip_to_probe = strdup(optarg); break; case 'V': - verbose++; + verbose_screen++; + verbose_logfile++; break; case 'E': if (++operation_specified > 1) { @@ -378,6 +382,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; @@ -396,6 +413,13 @@ cli_classic_abort_usage(); }
+#ifndef STANDALONE + if (log_name && check_filename(log_name, "log")) + cli_classic_abort_usage(); + if (log_name && open_logfile(log_name)) + return 1; +#endif /* !STANDALONE */ + #if CONFIG_PRINT_WIKI == 1 if (list_supported_wiki) { print_supported_wiki(); @@ -410,6 +434,10 @@ goto out; }
+#ifndef STANDALONE + start_logging(); +#endif /* STANDALONE */ + msg_gdbg("Command line (%i args):", argc - 1); for (i = 0; i < argc; i++) { msg_gdbg(" %s", argv[i]); @@ -546,11 +574,12 @@ */ programmer_delay(100000); ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); - /* Note: doit() already calls programmer_shutdown(). */ - goto out;
out_shutdown: programmer_shutdown(); out: +#ifndef STANDALONE + ret |= close_logfile(); +#endif return ret; } Index: flashrom-logfile/flashrom.c =================================================================== --- flashrom-logfile/flashrom.c (Revision 1536) +++ flashrom-logfile/flashrom.c (Arbeitskopie) @@ -40,7 +40,8 @@
const char flashrom_version[] = FLASHROM_VERSION; char *chip_to_probe = NULL; -int verbose = MSG_INFO; +int verbose_screen = MSG_INFO; +int verbose_logfile = MSG_DEBUG;
static enum programmer programmer = PROGRAMMER_INVALID;
@@ -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"); } @@ -1840,6 +1841,5 @@ free(oldcontents); free(newcontents); out_nofree: - programmer_shutdown(); return ret; }