[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