Re: [flashrom] [PATCH] common print infrastructure

[Carl-Daniel Hailfinger]

On 07.01.2010 05:35, Sean Nelson wrote:
> On 1/6/2010 8:31 PM, Sean Nelson wrote:
> > Updated patch against r834. Minimized the changes so we can commit
> > now and convert all printf functions later. Added MSG_SPEW.
> > Signed-off-by: Sean Nelson
>
> Fixed patch so the print() function has MSG_SPEW.
> Still.
> Signed-off-by: Sean Nelson 
> diff --git a/Makefile b/Makefile
> index a0bf1ec..8a9c13f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -48,7 +48,7 @@ CHIP_OBJS = jedec.o stm50flw0x0x.o w39v080fa.o sharplhf00l04.o w29ee011.o \
>
> LIB_OBJS = layout.o
>
> -CLI_OBJS = flashrom.o cli_classic.o print.o
> +CLI_OBJS = flashrom.o cli_classic.o cli_output.o print.o
>
> PROGRAMMER_OBJS = udelay.o programmer.o
>
> diff --git a/cli_output.c b/cli_output.c
> new file mode 100644
> index 0000000..00103d3
> --- /dev/null
> +++ b/cli_output.c

The name is debatable because other frontends may want to use the same
functions. I am unable to quickly come up with a better name, so your
choice it is.
> @@ -0,0 +1,55 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2009 Sean Nelson
> 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
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include "flash.h"
>
> +int print(int type, const char *fmt, ...)
> +{
> va_list ap;
> int ret;
> FILE *output_type;
> switch (type)
> {
> case MSG_SPEW:
> if (!spew) return 0;
> output_type = stdout;
> break;
> case MSG_DEBUG:
> if (!verbose) return 0;

Hm. This means generic code has to make sure that verbose is set if spew
is set. Otherwise we'd get nonsensical output (error+info+spew without
debug).
> output_type = stdout;
> break;
> case MSG_ERROR:
> output_type = stderr;
> break;
> case MSG_INFO:
> default:
> output_type = stdout;
> break;
> }
> va_start(ap, fmt);
> ret = vfprintf(output_type, fmt, ap);
> va_end(ap);
> return ret;
>
> +}
>
> diff --git a/flash.h b/flash.h
> index 648dc83..56c7588 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -504,6 +504,14 @@ int doit(struct flashchip *flash, int force, char *filename, int read_it, int wr
>  #define OK 0
>  #define NT 1    /* Not tested */
>
> +/* cli_print.c */
> +int print(int type, const char *fmt, ...);
>
> +#define MSG_INFO 0
> +#define MSG_DEBUG 1
> +#define MSG_SPEW 2
> +#define MSG_ERROR 3

Maybe reorder them a bit. ERROR, INFO, DEBUG, SPEW or the reverse variant.
Besides that, we're free to use MSG_BARF instead of MSG_SPEW ;-)
> /* cli_classic.c */
>  int cli_classic(int argc, char *argv[]);

Please don't kill me for this comment, but can we pick names which are
shorter, at least for the functions used from other files? The generic
print(...) code looks nice, but if you look at the length, the 80 column
limit is our enemy. Below is a comparison between old style, the style
of this patch, and my proposed alternative (which consists mainly of
wrappers, I know).

printf("foo")
print(MSG_INFO, "foo")
msg_info("foo")

printf_debug("foo")
print(MSG_DEBUG, "foo")
msg_debug("foo")

#define msg_debug(...) print(MSG_DEBUG, ...)
(the syntax is incorrect, but you get the point)

And while we're at it, I'd like to differentiate between generic
messages, programmer messages and chip messages.

msg_ginfo("This is a generic info message\n");
msg_pinfo("This is a programmer info message\n");
msg_cinfo("This is a chip info message\n");
msg_perror("This is a programmer error message\n");

It seems that msg_perror is a function name which turns up on google,
but I have no idea if it is a library function that could collide with
our code.

I'm also not sure whether we should shorten these names a bit more to
msg_[gpc]{err,inf,dbg,spw}
to give messages more breathing space at higher indentation levels.

Regards,
Carl-Daniel