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

[Carl-Daniel Hailfinger]

Am 08.05.2012 19:48 schrieb Idwer Vollering:
> 2012/5/3 Carl-Daniel Hailfinger :
> > I have decided to discard most of the logging code and rewrite it to be
> > more readable. It even worked in preliminary tests.
> > TODO:
> > Should "-o -" be a no-op which just redirects stderr to stdout? Right
> >
> > now "-" is treated as regular file name, but most Unix utilities are
> > unable to handle a file with that name (try "less -" for amusement).
>
> Is this a part of your long-term plan?

Some people might want all messages (even error messages) to end up on
stdout, but the more I think about it, the less I believe supporting -
as placeholder for stdout makes sense. One of the biggest problems with
that idea is that most messages would appear twice on screen, and that's
just stupid.
Unless someone objects, I'll remove this point from the TODO list.
> > Add man page entry
> > Is the log level difference for screen/logfile a good thing, and
> >
> > should we default the logfile level to dbg2 instead of dbg?
> >
> > Add log file support to flashrom.
> >
> > If you use cli_classic, the log file will always contain messages at
> > a level which is one higher than the one specified. That way we get
> > all verbose messages in the log even if the user doesn't specify -V.
>
> This handles -VV and -VVV too, correct?

Yes. If a user specifies -VV, the log file will get -VVV output, but the
on-screen info will stay manageable. Not sure how we should handle it
when the user specifies -VVV... for now, that would set msglevel to 5
(outside the defined range of the enum, but handled implicitly by the
code like msglevel 4).

We could make the log file always store logs at least at level -VV (to
get all dbg2 output) regardless of whether -V was specified or not, and
only if -VV or greater was specified, add 1 to the loglevel.
What do you think?
> > Convert all printf() after start of logging to msg_*().
> >
> > Allow separate control of log level for screen and logfile in case
> > other frontends (e.g. GUI, cli_mfg) want that.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: flashrom-logfile/flash.h
> >
> > --- flashrom-logfile/flash.h    (Revision 1528)
> > +++ flashrom-logfile/flash.h    (Arbeitskopie)
> > @@ -268,13 +269,20 @@
> >  #define ERROR_FLASHROM_LIMIT -201
> >
> > /* cli_output.c */
> > +#ifndef STANDALONE
> > +int open_logfile(const char * const filename);
> > +int close_logfile(void);
> > +void start_logging(void);
> > +#endif
> >  /* Let gcc and clang check for correct printf-style format strings. */
> > -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
>
> http://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/include/consol...

I think the coreboot loglevels have nice names, but coreboot has too
many loglevels for an application like flashrom.
> #define MSG_NOTICE 2
> #define MSG_DEBUG 3
> > -#define MSG_BARF       4
>
> #define MSG_SPEW 4

BARF->SPEW changed everywhere.
> > +enum msglevel {
> >   MSG_ERROR       = 0,
> >   MSG_INFO        = 1,
> >   MSG_DEBUG       = 2,
> >   MSG_DEBUG2      = 3,
> >   MSG_BARF        = 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 */
> >  #define msg_cerr(...)  print(MSG_ERROR, __VA_ARGS__)   /* chip errors */
> > Index: flashrom-logfile/cli_output.c
> > ===================================================================
> > --- flashrom-logfile/cli_output.c       (Revision 1528)
> > +++ flashrom-logfile/cli_output.c       (Arbeitskopie)
> > @@ -20,36 +21,80 @@
> >
> > #include <stdio.h>
> >  #include <stdarg.h>
> > +#include <string.h>
> > +#include <errno.h>
> >  #include "flash.h"
> >
> > -int print(int type, const char *fmt, ...)
> > +static FILE *logfile = NULL;
> >
> > +#ifndef STANDALONE
> > +int close_logfile(void)
> >  {
> >   if (logfile && fclose(logfile)) {
>
> No sync() / fsync() ?
> From http://linux.die.net/man/3/fclose "Note that fclose() only
> flushes the user space buffers provided by the C library. To ensure
> that the data is physically stored on disk the kernel buffers must be
> flushed too, for example, with sync(2) or fsync(2)."

Good point, will do.

I'll probably have to change the file open mode as well... Uwe mentioned
that "wb" is needed for Windows to behave sanely.
> >           /* fclose returned an error. Stop writing to be safe. */
> >           logfile = NULL;
> >           msg_perr("Closing the log file returned error %s\n",
> >                    strerror(errno));
> >           return 1;
> >   }
> >   logfile = NULL;
> >   return 0;
> >
> > +}
> > Index: flashrom-logfile/cli_classic.c
> >
> > --- flashrom-logfile/cli_classic.c      (Revision 1528)
> > +++ flashrom-logfile/cli_classic.c      (Arbeitskopie)
> > @@ -377,20 +394,40 @@
> >                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 filename specified.\n");
>
> "No log filename specified.\n" ?

No, if anything, it should be "No image filename specified".
Thoughts?
> >           cli_classic_abort_usage();
> >  }
> >   if (filename && (filename[0] == '\0')) {
> >           fprintf(stderr, "Error: No filename specified.\n");
>
> dito

dito.
> >           cli_classic_abort_usage();
> >   }
> >   if (filename && (filename[0] == '-'))
> >           fprintf(stderr, "Warning: Supplied file name starts with -\n");
> >
> > +#ifndef STANDALONE
> >   if (log_name && (log_name[0] == '-'))
> >           fprintf(stderr, "Warning: Supplied file name starts with -\n");
> >   if (log_name && open_logfile(log_name))
>
> WARNING or Warning? Maybe add a line that says that this will change
> in the future?

I'd say "Warning" it totally OK... it's an indicator that the user did
something unintended, but it's not an error per se.
> >           return 1;
> >
> > +#endif /* !STANDALONE */

Regards,
Carl-Daniel