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 <[email protected]> >> > 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 <[email protected]> >> > >> > 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 <[email protected]> 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 <[email protected]> + * 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/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
