Re: [flashrom] [PATCH] Add logfile support to flashrom

[Uwe Hermann]

On Thu, Jun 09, 2011 at 10:32:30PM +0200, Carl-Daniel Hailfinger wrote:
> Add log file support to flashrom.
>
> The log file will always contain all verbose messages even if the user
> doesn't specify -V. If the user specifies -VV, SPEW messages will be
> logged as well.
>
> Proof of concept, comments welcome.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-logfile/flash.h
>
> --- flashrom-logfile/flash.h	(Revision 1326)
> +++ flashrom-logfile/flash.h	(Arbeitskopie)
> @@ -229,6 +229,8 @@
>  #define ERROR_NONFATAL 0x100
>
> /* cli_output.c */
> +int open_logfile(char *filename);

Can be "const char" too, I guess.
> +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 1326)
> +++ flashrom-logfile/cli_output.c	(Arbeitskopie)
> @@ -2,6 +2,7 @@
> This file is part of the flashrom project.
> Copyright (C) 2009 Sean Nelson
> 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,30 +23,70 @@
>  #include <stdarg.h>
>  #include "flash.h"
>
> -int print(int type, const char *fmt, ...)
> +static FILE *logfile = NULL;
>
> +int open_logfile(char *filename)
>  {
> if (!filename) {
> msg_gerr("No filename specified.\n");
> return 1;
> }
> if ((logfile = fopen(filename, "w")) == NULL) {

Use "wb" please, the "b" is required to avoid issues on Windows. There's
another occurence in flashrom of fopen() withouth "b", that should be
fixed too.
> +int print(int type, const char *fmt, ...)
> +{
> va_list ap;
> int ret = 0;
> int want_screen = 1;
> int want_file = 1;

Can be merged in one line for more compact code.

There's a small issue with the patch, it double-prints some lines on stdout
now (in the logfile they appear only once if -o is used):

$ ./flashrom 
  flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian
  flashrom is free software, get the source code at http://www.flashrom.org

flashrom v0.9.3-r1331 on Linux 2.6.38-2-amd64 (x86_64), built with libpci 3.1.7, GCC 4.5.2, little endian
  Calibrating delay loop... OK.
  ERROR: Could not get I/O privileges (Operation not permitted).
  You need to be root.

Also, not all whitespace seems to be the same as in the logfile, e.g. for

$ ./flashrom -L -o foo

Uwe.