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