[flashrom] [PATCH] Add logfile support to flashrom

[Carl-Daniel Hailfinger]

Am 09.05.2012 15:54 schrieb Stefan Tauner:
> On Wed, 09 May 2012 15:16:04 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>> Am 08.05.2012 19:48 schrieb Idwer Vollering:
>>> 2012/5/3 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>:
>>>> 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 ;)

Killed.


> 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...

The warning for file names starting with "-" is something I'd like to
introduce for all files accessed by flashrom.


>>>> - 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.

Not DEBUG2? IIRC you once said that for ICHSPI debugging DEBUG2 produces
better logs. And since we introduced DEBUG2, tha amount of msg_*dbg2
messages was kept pretty low. Of course SPEW is overkill for pretty much
every log.
I like your minimum verbosity proposal, but I'd pick DEBUG2 and provide
SPEW both on-screen and in the log file only in case the user specifies
-VVV.


> 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).

Log level control UI is an issue conceptually separate from log writing
and I'd like to postpone this until the log writing review is pretty
much done.


>>>> 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.

Done.

 
>>>> +               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... :)

<blink>WARNING</blink>

The point about "Warning" vs. "WARNING" is intricately linked to whether
you believe there should be one or two levels of warnings ("retrying a
different erase command"  vs "your EC is stuck, and we just erased its
firmware"). Even a really serious warning is not an error because
flashrom may be theoretically able to fix this while it is still running.
That's largely nitpicking, though. I have no really strong feelings
about this.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/