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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Jul 01, 2013 at 02:54:07PM -0400, Seiji Aguchi wrote:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ca6fdf6..a6dac1a 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"
> +    "  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?

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.

> 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 <seiji.aguchi@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, 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 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);
}