Re: [flashrom] [PATCH] Clean up msg_*

[Stefan Tauner]

On Fri, 11 May 2012 07:10:08 +0200
Carl-Daniel Hailfinger  wrote:
> Am 09.05.2012 12:28 schrieb Carl-Daniel Hailfinger:
> > Convert printf to msg_* where appropriate.
> > Clean up cli_output.c to be more readable.
> > Use enum instead of #define for message levels.
> > Kill a few exit(0) calls.
> > Print the command line arguments in verbose mode.
> > Move actions (--list-supported etc.) after argument sanity checks.
> > Reduce the number of code paths which have their own programmer_shutdown().
> >
> > Note: This patch was formerly part of the logfile patch, but that made
> > review harder.
>
> New version.
> The only remaining issue mentioned in review is "WARNING" vs. "Warning"
> capitalization. We could postpone that decision, or decide now.
>
> Signed-off-by: Carl-Daniel Hailfinger

comments starting with * are mandatory for the ack below.
> Index: flashrom-message_reorg/flash.h
>
> --- flashrom-message_reorg/flash.h	(Revision 1534)
> +++ flashrom-message_reorg/flash.h	(Arbeitskopie)
> @@ -269,12 +269,14 @@
>
> /* cli_output.c */
>  /* Let gcc and clang check for correct printf-style format strings. */

* that comment should move with int print(…) i guess
> -int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
> -#define MSG_ERROR	0
> -#define MSG_INFO	1
> -#define MSG_DEBUG	2
> -#define MSG_DEBUG2	3
> -#define MSG_BARF	4
> +enum msglevel {
> MSG_ERROR	= 0,
> MSG_INFO	= 1,
> MSG_DEBUG	= 2,
> MSG_DEBUG2	= 3,
> MSG_SPEW	= 4,
>
> +};
> +int print(enum msglevel level, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
>  #define msg_gerr(...)	print(MSG_ERROR, __VA_ARGS__)	/* general errors */
>  #define msg_perr(...)	print(MSG_ERROR, __VA_ARGS__)	/* programmer errors */
> […]
>
> /* layout.c */
>  int register_include_arg(char *name);
> Index: flashrom-message_reorg/cli_output.c
> ===================================================================
> --- flashrom-message_reorg/cli_output.c	(Revision 1534)
> +++ flashrom-message_reorg/cli_output.c	(Arbeitskopie)
> @@ -22,34 +22,25 @@
>  #include <stdarg.h>
>  #include "flash.h"
>
> -int print(int type, const char *fmt, ...)
> +/* Please note that level is the verbosity, not the importance of the message. */
> +int print(enum msglevel level, const char *fmt, ...)
>  {
>    va_list ap;
> int ret;
> FILE *output_type;
> int ret = 0;
> FILE *output_type = stdout;
> switch (type) {
> case MSG_ERROR:
> if (level == MSG_ERROR)
> output_type = stderr;
> break;
>
> case MSG_BARF:
> if (verbose < 3)
> 	return 0;
>
> case MSG_DEBUG2:
> if (verbose < 2)
> 	return 0;
>
> case MSG_DEBUG:
> if (verbose < 1)
> 	return 0;
>
> case MSG_INFO:
>
> default:
> output_type = stdout;
> break;
>
> }
>
> va_start(ap, fmt);
>
> ret = vfprintf(output_type, fmt, ap);
>
> va_end(ap);
>
> fflush(output_type);
> if (level <= verbose) {
> va_start(ap, fmt);
> ret = vfprintf(output_type, fmt, ap);
> va_end(ap);
> /* msg_*spew usually happens inside chip accessors in possibly
>  * time-critical operations. Don't slow them down by flushing.

counter-proposal:
+		/* msg_*spew * * happens inside chip accessors in possibly
+		 * time-critical operations *too*. Don't slow them down by flushing.

is it really *usually*?
hm most often in the resulting output, yes maybe...
>  */
> if (level != MSG_SPEW)
> 	fflush(output_type);
> }
> return ret;
>
> }
> Index: flashrom-message_reorg/cli_classic.c
> ===================================================================
> --- flashrom-message_reorg/cli_classic.c	(Revision 1534)
> +++ flashrom-message_reorg/cli_classic.c	(Arbeitskopie)
> @@ -377,20 +377,31 @@
>    	cli_classic_abort_usage();
>    }
> /* FIXME: Print the actions flashrom will take. */
> if (list_supported) {
> print_supported();
> exit(0);
> if ((read_it | write_it | verify_it) && !filename) {
> fprintf(stderr, "Error: No image file specified.\n");
> cli_classic_abort_usage();
> }
> if (filename && (filename[0] == '\0')) {
> fprintf(stderr, "Error: No image file specified.\n");
> cli_classic_abort_usage();
> }
> if (filename && (filename[0] == '-'))
> fprintf(stderr, "Warning: Supplied image file name starts with -\n");

you said you want to do this with other files written by flashrom
in the future too. doing this kind of checks in an
int check_filename(const char const *filename)
would probably makes sense then...
* if you agree please add at least a FIXME comment
> #if CONFIG_PRINT_WIKI == 1
>    if (list_supported_wiki) {
>    	print_supported_wiki();
> exit(0);
> ret = 0;
> goto out;
> }
>
> #endif
> if (list_supported) {
> print_supported();
> ret = 0;
> goto out;
> }
> if (layoutfile && read_romlayout(layoutfile))
>   cli_classic_abort_usage();
>  if (process_include_args())
>
> @@ -415,11 +426,17 @@
>    if (prog == PROGRAMMER_INVALID)
>    	prog = default_programmer;
> msg_gdbg("Command line:");
> for (i = 0; i < argc; i++) {
> msg_gdbg(" %s", argv[i]);

additional " " would ease debugging really stupid shell problems, but
would make it less readable in almost all circumstances.
making them appear in -VVV output would be an option but waaaay too
overkill... sorry that little prefectionist[1] crept out again ;)

[1]: http://www.thinkgeek.com/tshirts-apparel/unisex/generic/894a/

hm that little guy just showed me this example though:

./flashrom -p dummy:emulate=MX25L6436,spi_status=0x1 -V -c "SFDP-capable chip" -w bla
[…]
Command line: ./flashrom -p dummy:emulate=MX25L6436,spi_status=0x1 -V -c SFDP-capable chip -w bla

note the missing " on the -c option.
> }
>
> 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,25 +459,25 @@
>    }

some of the changes below would look different with the new line length
limit, but i guess you dont interpret this changes as "new code" (or
was the mail from uwe being accepted as veto)?
> 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 (force && read_it && chip_to_probe) {
> 	struct registered_programmer *pgm;
> 	int compatible_programmers = 0;
> 	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");
> /* This loop just counts compatible controllers. */
> for (j = 0; j < registered_programmer_count; j++) {
> 	pgm = &registered_programmers[j];
>
> @@ -468,9 +485,9 @@
>    				compatible_programmers++;
>    		}
>    		if (compatible_programmers > 1)
> 		printf("More than one compatible controller "
> 		       "found for the requested flash chip, "
> 		       "using the first one.\n");
> 		msg_cinfo("More than one compatible controller "
> 			  "found for the requested flash chip, "
> 			  "using the first one.\n");
> for (j = 0; j < registered_programmer_count; j++) {
> 	pgm = &registered_programmers[j];
> 	startchip = probe_flash(pgm, 0, &flashes[0], 1);
>
> @@ -478,14 +495,15 @@
>    				break;
>    		}
>    		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);

*sigh* (it's not mandatory that you repeat this yorself for the ack ;)
> 		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;

hm... the programmer shutdown was never called in this code path before
the patch?
if so and you want to revert the shutdown change (see below), please
add an explicit call here.
> }
> ret = 1;
> goto out_shutdown;
>
> @@ -505,23 +523,17 @@
>    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");
> msg_ginfo("No operations were specified.\n");
> goto out_shutdown;
> }
> if (!filename && !erase_it) {
> printf("Error: No filename specified.\n");
> ret = 1;
> goto out_shutdown;
> }
> /* Always verify write operations unless -n is used. */
>  if (write_it && !dont_verify_it)
>   verify_it = 1;
>
> @@ -531,9 +543,10 @@
>     * 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:
>    return ret;
>  }
> Index: flashrom-message_reorg/flashrom.c
> ===================================================================
> --- flashrom-message_reorg/flashrom.c	(Revision 1534)
> +++ flashrom-message_reorg/flashrom.c	(Arbeitskopie)
> @@ -40,7 +40,7 @@
> […]
>  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%5Cn");
> msg_ginfo("\n");
>
> }

i have never understood why flashrom prints that - even with a \n\n
effectively, although the main developer argues about every other
wasteful line printed ;)

could be joined to a single loc (well or at least 2 if you want to
leave the stray \n) due to our new line length limit...
> @@ -1840,6 +1840,5 @@
>    free(oldcontents);
>    free(newcontents);
>  out_nofree:
> programmer_shutdown();

i guess this breaks google's cli?
i dont see why this part should actually move into the cli file(s).
rather the stuff from cli_classic.c starting from the
myusec_calibrate_delay call should move into flashrom.c (or somewhere else).
yes there are some ui-related messages in there, but they are
everywhere... and should be made ui-agnostic or ui-refering
(i.e. printing messages depending on the current ui).

* i think dropping this change and the related ones in cli_classic.c
  for now is the best option. your choice though, since i think it does
  not *break* stuff in the trunk and may ease later refactoring of that
  code to a proper location.
> return ret;
>  }

this was way harder to review than expected and although there is quite
some place for nit-picking and further discussion i think it improves
the code after all, hence this is
Acked-by: Stefan Tauner 
if the notes starting with * are all thought through two or more
times :)