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