Patchwork Add logfile support to flashrom

login
register
about
Submitter Stefan Tauner
Date 2012-02-09 20:13:54
Message ID <201202092013.q19KDFpe031614@mail2.student.tuwien.ac.at>
Download mbox | patch
Permalink /patch/3524/
State Superseded
Headers show

Comments

Stefan Tauner - 2012-02-09 20:13:54
On Wed, 04 Jan 2012 02:08:06 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> Oh well... new iteration, with some of the suggestions merged.
> This still is not final, but it should be a bit closer.
> 
> @@ -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:

^ unused (hence breaking compilation)

> +#ifndef STANDALONE
> +	ret |= close_logfile();
> +#endif
>  	return ret;

also i see a little problem with this... ;)
./flashrom 
flashrom v0.9.4-r1483 on Linux 2.6.35-32-generic (x86_64)
flashrom is free software. Get the source code at http://www.flashrom.org

No filename specified.

attached is your patch rebased to r1490 without "out:"
maybe we can get this into 0.9.5 after some rework? do you have a todo
for it?

Patch

From af484e8544dfb066183b3a8c39d14600c34f9b0f Mon Sep 17 00:00:00 2001
From: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Date: Wed, 4 Jan 2012 00:08:06 +0000
Subject: [PATCH] Add logfile support to flashrom

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@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.

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@gmx.net>
---
 cli_classic.c |   90 +++++++++++++++++++++++++++++++++++++-----------------
 cli_output.c  |   94 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 flash.h       |    6 ++++
 flashrom.c    |   43 +++++++++++++-------------
 4 files changed, 170 insertions(+), 63 deletions(-)

diff --git a/cli_classic.c b/cli_classic.c
index 7661612..d7a0b66 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -106,7 +106,7 @@  static void cli_classic_usage(const char *name)
 	         "-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 @@  static void cli_classic_usage(const char *name)
 	         "<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 @@  int main(int argc, char *argv[])
 	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'},
@@ -194,11 +195,13 @@  int main(int argc, char *argv[])
 		{"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;
 
@@ -366,6 +369,19 @@  int main(int argc, char *argv[])
 			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;
@@ -377,6 +393,16 @@  int main(int argc, char *argv[])
 		cli_classic_abort_usage();
 	}
 
+	if (filename && (filename[0] == '\0')) {
+		fprintf(stderr, "Error: No filename specified.\n");
+		cli_classic_abort_usage();
+	}
+
+#ifndef STANDALONE
+	if (open_logfile(log_name))
+		return 1;
+#endif /* !STANDALONE */
+
 	/* FIXME: Print the actions flashrom will take. */
 
 	if (list_supported) {
@@ -415,11 +441,21 @@  int main(int argc, char *argv[])
 	if (prog == PROGRAMMER_INVALID)
 		prog = default_programmer;
 
+#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;
 	}
@@ -442,34 +478,35 @@  int main(int argc, char *argv[])
 	}
 
 	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;
@@ -490,20 +527,14 @@  int main(int argc, char *argv[])
 	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");
-		goto out_shutdown;
-	}
-
-	if (!filename && !erase_it) {
-		printf("Error: No filename specified.\n");
-		ret = 1;
+		msg_ginfo("No operations were specified.\n");
 		goto out_shutdown;
 	}
 
@@ -516,9 +547,12 @@  int main(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);
 
 out_shutdown:
 	programmer_shutdown();
+#ifndef STANDALONE
+	ret |= close_logfile();
+#endif
 	return ret;
 }
diff --git a/cli_output.c b/cli_output.c
index 2a67bea..2e1f651 100644
--- a/cli_output.c
+++ b/cli_output.c
@@ -2,6 +2,7 @@ 
  * This file is part of the flashrom project.
  *
  * Copyright (C) 2009 Sean Nelson <audiohacked@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;
-		break;
 	case MSG_BARF:
-		if (verbose < 3)
-			return 0;
+		if (verbose < 3) {
+			want_screen = 0;
+			want_file = 0;
+		}
+		break;
 	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;
 }
diff --git a/flash.h b/flash.h
index e51b6d4..56d75c6 100644
--- a/flash.h
+++ b/flash.h
@@ -265,6 +265,12 @@  int write_buf_to_file(unsigned char *buf, unsigned long size, const char *filena
 #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
diff --git a/flashrom.c b/flashrom.c
index ee68344..12ff73e 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1422,27 +1422,27 @@  void list_programmers_linebreak(int startcol, int cols, int paren)
 			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 @@  void print_sysinfo(void)
 #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_version(void)
 
 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 @@  out:
 	free(oldcontents);
 	free(newcontents);
 out_nofree:
-	programmer_shutdown();
 	return ret;
 }
-- 
1.7.1