[PATCH] common print infrastructure

[Carl-Daniel Hailfinger]

Awesome stuff. A must-have for 0.9.2. It touches a lot of code, so I'd
like to merge it as the last patch before 0.9.2 to minimize conflicts.

Minor comments:
- MSG_NORMAL: Not 100% sure if this name is best (_INFO or _NOTICE or
_DEFAULT maybe).
- flashrom_print.c: Maybe rename it to flashrom_output.c and move stuff
like progress printing in there, but I'm open to leaving it as is.
- Conditional compilation: Should the code in flashrom_print.c be #ifdef
STANDALONE or somesuch?
- Makefile: Did you forget to hook up the new file in Makefile?

Regards,
Carl-Daniel

[Sean Nelson]

On 12/11/09 6:58 PM, Carl-Daniel Hailfinger wrote:
> Awesome stuff. A must-have for 0.9.2. It touches a lot of code, so I'd
> like to merge it as the last patch before 0.9.2 to minimize conflicts.
>
> Minor comments:
> MSG_NORMAL: Not 100% sure if this name is best (_INFO or _NOTICE or
>
> _DEFAULT maybe).
> flashrom_print.c: Maybe rename it to flashrom_output.c and move stuff
>
> like progress printing in there, but I'm open to leaving it as is.
> Conditional compilation: Should the code in flashrom_print.c be #ifdef
>
> STANDALONE or somesuch?
> Makefile: Did you forget to hook up the new file in Makefile?
>
> Regards,
> Carl-Daniel

Changed MSG_NORMAL to MSG_INFO.
Renamed the new file to flashrom_output.c
Hooked up the new file in Makefile.

If someone more experienced with va_list could help me; currently the 
output is all funky, please fix it. I really have no idea what I'm doing 
with the va_list code in print(). The Problem could be my macbook, but 
everything compiles just fine. As long as someone could fix my little 
bug and/or no one has a problem with my patch:

Signed-off-by: Sean Nelson

[Sean Nelson]

Figured out the problem: fprintf vs vfprintf. Also added a check in 
print(MSG_DEBUG, ...) for verbose mode. This should be the last version 
of the patch, unless another version is requested. Also, I think we need 
to figure out why patchwork thinks my patches are unrelated and creates 
separate entries for each changed patch.

Signed-off-by: Sean Nelson

[Carl-Daniel Hailfinger]

On 12.12.2009 08:04, Sean Nelson wrote:
> [...] we need to figure out why patchwork thinks my patches are
> unrelated and creates separate entries for each changed patch.

Actually, that's expected because patchwork can't know if two patches
are related. I'm thinking of a Supersedes: tag, but right now I'm just
marking the old patch as superseded in the web interface.

Regards,
Carl-Daniel

[Sean Nelson]

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

[Sean Nelson]

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

[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

[Sean Nelson]

Use MSG_BARF instead of MSG_SPEW. Make Barf mode depend on verbose 
level. Reorder the levels. Add message variants like
msg_{g,p,c}{err,info,dbg,spw}

[Sean Nelson]

On 1/7/10 10:17 AM, Sean Nelson wrote:
> Use MSG_BARF instead of MSG_SPEW. Make Barf mode depend on verbose 
> level. Reorder the levels. Add message variants like
> msg_{g,p,c}{err,info,dbg,spw}

Change spw based functions to spew. Explain {g,p,c} near functions.

[Carl-Daniel Hailfinger]

On 07.01.2010 20:05, Sean Nelson wrote:
> On 1/7/10 10:17 AM, Sean Nelson wrote:
> > Use MSG_BARF instead of MSG_SPEW. Make Barf mode depend on verbose
> > level. Reorder the levels. Add message variants like
> > msg_{g,p,c}{err,info,dbg,spw}
>
> Change spw based functions to spew. Explain {g,p,c} near functions.

Thanks. Add a signoff (as reply to this, no need to repost the patch)
and a nice changelog and this is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel