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

[Seiji Aguchi]

> > +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
> > address@hidden -msg timestamp=on|off
> > address@hidden -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.

> 
> > diff --git a/util/qemu-time.c b/util/qemu-time.c
> > new file mode 100644
> > index 0000000..3862788
> > --- /dev/null
> > +++ b/util/qemu-time.c
> > @@ -0,0 +1,26 @@
> > +/*
> > + * 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, size_t len)
> > +{
> > +    GTimeVal tv;
> > +    gchar *tmp_str = NULL;
> > +
> > +    /* Size of len should be at least TIMESTAMP_LEN to avoid truncation */
> > +    assert(len >= TIMESTAMP_LEN);
> > +
> > +    g_get_current_time(&tv);
> > +    tmp_str = g_time_val_to_iso8601(&tv);
> > +    g_strlcpy((gchar *)timestr, tmp_str, len);
> > +    g_free(tmp_str);
> > +}
> 
> Why strcpy into a fixed-size buffer when glib gives us a heap-allocated
> string?
> 
> This patch would be simpler by dropping qemu-time.c/time.h 

I plan to introduce timestamp to monitor_printf() and fprintf()
near future.

In this case, it is better from a maintenance POV to keep time-handling
 functionality  in a file that's separate from the qemu error file, so it can 
be reused
elsewhere in QEMU if needed.

It is suggested by Daniel.
http://marc.info/?l=qemu-devel&m=136741113218944&w=2

> and simply
> doing:
> 
> if (enable_timestamp_msg) {
>     GTimeVal tv;
>     gchar *timestamp;
> 
>     g_get_current_time(&tv);
>     timestamp = g_time_val_to_iso8601(&tv);
>     error_printf("%s ", timestamp);
>     g_free(timestamp);

I'm concerned that we may have potential memory leak
If someone neglect to call the g_free().
That is why I use the fixed-size buffer.

Seiji