Re: [Qemu-devel] [PATCH v4] Add timestamp to error message

[Seiji Aguchi]

> > 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
> > address@hidden -msg timestamp=on|off
> > address@hidden - 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(&timestr);
> > +        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 <address@hidden>
> > + *
> > + * 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