Re: [Dle-develop] [PATCH v5] Add timestamp to error_report()

[Seiji A. <sei...@hd...>]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:ste...@gm...]
> Sent: Wednesday, July 03, 2013 5:14 AM
> To: Seiji Aguchi
> Cc: qem...@no...; ali...@us...; ber...@re...; kw...@re...; mto...@re...;
> ar...@re...; Tomoki Sekiyama; pbo...@re...; lca...@re...; le...@re...; eb...@re...;
> dle...@li...
> Subject: Re: [PATCH v5] Add timestamp to error_report()
> 
> On Tue, Jul 02, 2013 at 02:09:24PM +0000, Seiji Aguchi wrote:
> >
> >
> > > > +DEF("msg", HAS_ARG, QEMU_OPTION_msg,
> > > > +    "-msg [timestamp=on|off]\n"
> > > > +    "  change the format of messages\n"
> > > > +    "  timestamp=on|off enables leading timestamps (default:on)\n",
> > > > +    QEMU_ARCH_ALL)
> > > > +STEXI
> > > > +@item -msg timestamp=on|off
> > > > +@findex -msg
> > > > +prepend a timestamp to each log message.
> > > > +(disabled by default)
> > > > +ETEXI
> > >
> > > I am confused.  If the user specifies -msg then enable_timestamp_msg is
> > > on by default.  If the user does not specify -msg then
> > > enable_timestmap_msg is off.  Did I get that right?
> >
> > Yes.
> >
> > >
> > > This means that the default behavior of QEMU does not change but you can
> > > add -msg to enable timestamps.
> > >
> > > I'm happy with this but find the documentation confusing.
> >
> > I can remove "(disabled by default)" if needed.
> 
> Perhaps the simplest solution is timestamp=off by default.  Then there
> can be no confusion and users must do -msg timestamp=on to enable
> timestamps.
> 
> If you really want to keep -msg as a shortcut for -msg timestamp=on,
> then please document explicitly that:
> 1. Without -msg timestamps are off.
> 1. With -msg timestamps are on.
> 2. -msg timestamp=off can be used to turn timestamps off again.

My apologies for the confusion.

The syntax, "-msg [timestamp=on|off]", was wrong.
"-msg timestamp[=on|off]" is correct.

And there is no way to make "timestamp" optional, as far as I looked into a source code.
Therefore, the explanation should be as below.
(I think it is reasonable to keep "-msg timestamp" as a shortcut for -msg timestamp=on.)

<snip>
+DEF("msg", HAS_ARG, QEMU_OPTION_msg,
+    "-msg timestamp[=on|off]\n"
+    "                change the format of messages\n"
+    "                on|off controls leading timestamps (default:on)\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -msg timestamp[=on|off]
+@findex -msg
+prepend a timestamp to each log message.(default:on)
+ETEXI
<snip>

To be simpler, we may be able to introduce just a single -msg-timestamp.
But I think current "-msg timestamp[=on|off]" is reasonable
because other options may be introduced to msg, like log_level or debug.

Seiji