Re: [Qemu-devel] [PATCH v5] Add timestamp to error_report()

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Jul 04, 2013 at 02:57:13AM +0000, Seiji Aguchi wrote:
> 
> 
> > -----Original Message-----
> > From: Stefan Hajnoczi [mailto:stefanha@gmail.com]
> > Sent: Wednesday, July 03, 2013 5:14 AM
> > To: Seiji Aguchi
> > Cc: qemu-devel@nongnu.org; aliguori@us.ibm.com; berrange@redhat.com; kwolf@redhat.com; mtosatti@redhat.com;
> > armbru@redhat.com; Tomoki Sekiyama; pbonzini@redhat.com; lcapitulino@redhat.com; lersek@redhat.com; eblake@redhat.com;
> > dle-develop@lists.sourceforge.net
> > 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.)

Yes, I think you are correct.  I thought previously that "-msg" works
but it seems an option is always required.

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

Yep.

Stefan