[flashrom] [PATCH] Add logfile support to flashrom
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Wed Jan 4 02:08:06 CET 2012
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 at 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 at 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 at 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 at 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/
More information about the flashrom
mailing list