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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Jul 28 00:35:33 CEST 2011


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.


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;
>  		}
>  		/* Clean up after the check. */
>  		flash = NULL;
>  	}
>  
> -#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;
>  	} else if (!chipcount) {
>  		msg_cinfo("No EEPROM/flash device found.\n");
>  		if (!force || !chip_to_probe) {
> @@ -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;
>  		}
>  		// FIXME: flash writes stay enabled!
>  		programmer_shutdown();
> -		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;
>  }
>  
>   

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list