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

[Stefan Tauner]

On Wed, 09 May 2012 15:16:04 +0200
Carl-Daniel Hailfinger  wrote:
> 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.

useless imho. what's a plausible use case? as long as we log the same
stuff (minus verbosity) to stdout and the log, this does not make sense.
stderr -> stdout redirecting is something the underlying shell should
support not every app on top. also this is mostly used for log file
generation anyway... imho ;)

NB: although i dont like to obstruct users by introducing unneeded
hurdles in general i want to mention the possibility of rejecting
"-" (and other similar stuff on !unix) as a filename. iirc we dont do
that in other cases where we write files so i would not introduce it
with this patch for log files only anyway...
> > > 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?

i dont like that implicit "+1" verbosity in the current implementation.
while non-verbose output is almost useless in the logfile, -VV and
especially -VVV hides the interesting parts in most cases and should
only be logged if the user really wants that much detail. i guess it
might also create quite large log files in some cases.

i think the best solution would be a minimum verbosity of MSG_DEBUG in
the log file, but without any other escalations.

an alternative would be a mandatory log file verbosity switch.
carl-daniel once told me that he would like to see a UI to change the
verbosity of the different log types (programmer, general, chip etc).
so a switch for the log file itself does not seem to be that much out
of proportion (although it makes the previously mentioned UI problem
harder).
> […]
> > >           /* 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?

if we ever merge the "per layout range image" patch we should
differentiate between the "master" image here and the range images.
adding "image" here now makes sense imho.
> > >           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.

we use both - WARNING and Warning - throughout the code. is that
intended? if so what's the policy? if not then we should fix it. i
think (without looking at any specific case) that WARNING is warranted
because it sticks out more (especially in verbose outputs). as long as
we dont want to play with bold or colored text... :)