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:
- patchset voltage printing 2.1
- rebased add log file support
- 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;-#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;@@ -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;
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; }