> > diff --git a/include/qemu/time.h b/include/qemu/time.h
> > new file mode 100644
> > index 0000000..f70739b
> > --- /dev/null
> > +++ b/include/qemu/time.h
> > @@ -0,0 +1,11 @@
> > +#ifndef TIME_H
> > +#define TIME_H
>
> I wonder if TIME_H is maybe a bit too nondescript and could conflict
> with other guards.
>
> The guards currently used in "include/qemu/*.h" files are inconsistent
> -- some use a QEMU_ prefix, some a __QEMU_ one, and others use none.
>
> Don't respin just because of this, but maybe it's something to consider.
This should be discussed in other thread...
>
>
> > +
> > +#include "qemu-common.h"
> > +
> > +/* "1970-01-01T00:00:00.999999Z" + '\0' */
> > +#define TIMESTAMP_LEN 28
> > +extern void qemu_get_timestamp_str(char (*timestr)[]);
I will change to "extern void qemu_get_timestamp_str(char *timestr, size_t len)"
as Eric pointed out.
>
>
> > +extern bool enable_timestamp_msg;
> > +
> > +#endif /* !TIME_H */
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index ca6fdf6..7890921 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -3102,3 +3102,15 @@ HXCOMM This is the last statement. Insert new
> > options before this line!
> > STEXI
> > @end table
> > ETEXI
> > +
> > +DEF("msg", HAS_ARG, QEMU_OPTION_msg,
> > + "-msg [timestamp=on|off]\n"
> > + " output message with timestamp (default: off)\n",
> > + QEMU_ARCH_ALL)
> > +STEXI
> > +@item -msg timestamp=on|off
> > +@findex - msg
>
> A space has snuck in between "-" and "msg". Perhaps this is the only
> note that would warrant a respin (or rather a maintainer fixup at commit).
I will fix it.
>
>
> > +Output message with timestamp.
>
> As a non-native speaker, I propose rewording this as "prepend a
> timestamp to each log message" (in the prior occurrence as well) if you
> decided to repost.
Will fix as well.
> > void error_report(const char *fmt, ...)
> > {
> > va_list ap;
> > + char timestr[TIMESTAMP_LEN];
> > +
> > + if (enable_timestamp_msg) {
> > + qemu_get_timestamp_str(×tr);
> > + error_printf("%s ", timestr);
> > + }
> >
> > error_print_loc();
> > va_start(ap, fmt);
>
> Does this print the timestamp to all kinds of monitors too? On stderr,
> the timestamp is great. When printed to an "interactive monitor", it
> could also help post-mortem debugging. But would it not confuse libvirt
> eg.? (Apologies if this has been discussed before.)
I will try to add timestamp to monitor_printf().
(In the long term, I would like to add it to all fprintf() in qemu.)
>
>
> > diff --git a/util/qemu-time.c b/util/qemu-time.c
> > new file mode 100644
> > index 0000000..37f7b9e
> > --- /dev/null
> > +++ b/util/qemu-time.c
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Time handling
> > + *
> > + * Copyright (C) 2013 Hitachi Data Systems Corp.
> > + *
> > + * Authors:
> > + * Seiji Aguchi <seiji.agu...@hds.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#include "qemu/time.h"
> > +
> > +void qemu_get_timestamp_str(char (*timestr)[])
> > +{
> > + GTimeVal tv;
> > + gchar *tmp_str = NULL;
> > +
> > + g_get_current_time(&tv);
>
> Hm. There's also g_get_monotonic_time(), but (a) that's less portable
> (available since 2.28), (b) this is simply good enough IMO, in practice. OK.
>
>
> > + tmp_str = g_time_val_to_iso8601(&tv);
> > + g_strlcpy((gchar *)*timestr, tmp_str, TIMESTAMP_LEN);
> > + g_free(tmp_str);
> > + return;
>
> You're not returning a value so the last statement is superfluous.
I will remove the unnecessary "return".
> > +
> > +static void configure_msg(QemuOpts *opts)
> > +{
> > + enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true);
> > +}
>
> I think the default value doesn't match the docs, which say "deafult:
> off". As far as I recall (from Tomoki's "-realtime [mlock=on|off]"
> patch), statements about defaults in the option docs don't describe how
> qemu works by default (ie. when you omit the option altogether), they
> describe what happens if you specify the option but omit its arguments
> (ie. with "-msg" only.)
>
> In that case, the docs should state something like:
>
> DEF("msg", HAS_ARG, QEMU_OPTION_msg,
> "-msg [timestamp=on|off]\n"
> " change the format of error messages\n"
> " timestamp=on|off enables leading timestamps (default: on)\n",
> QEMU_ARCH_ALL)
Currently, this patch add timestamp to just error_report().
But, we may need it for both error and normal messages.
So, I will change the sentence "change the format of error messages"
to "change the format of messages".
I appreciate your review.
Seiji
