Re: [Dle-develop] [PATCH v3] Add timestamp to error message

[Eric B. <eb...@re...>]

On 06/26/2013 04:20 PM, Seiji Aguchi wrote:
> [Issue]
> When we offer a customer support service and a problem happens
> in a customer's system, we try to understand the problem by
> comparing what the customer reports with message logs of the
> customer's system.
> 

> +#include "qemu-common.h"
> +
> +/*  "1970-01-01T00:00:00.999999Z" + '\0' */
> +#define TIMESTAMP_LEN 28

Self-documenting constants are nicer:

#define TIMESTAMP_LEN (sizeof("1970-01-01T00:00:00.999999Z")+1)

> +extern void qemu_get_timestamp_str(char (*timestr)[]);

Eww.  I had a tough time parsing this signature.  Isn't it the same as
the more legible:

extern void qemu_get_timestamp_str(char **timestr);

Furthermore, isn't the idea that you DON'T want to return a malloc'd
string, but require that the user pre-allocate storage that we write
into (since malloc during a log message reporting OOM is liable to fail,
but we still want the log to be best effort)?  But then why do you need
a pointer to an array?  Wouldn't it be better as:

extern void qemu_get_timestamp_str(char *timestr, size_t len)

where len is the length of timestr, and where the comments document that
len should be at least TIMESTAMP_LEN to avoid truncation (or even
assert() if it is not)?

> +++ 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)

Did you test that the existing query-command-line-options QMP command
will list this option (just making sure that libvirt will be able to
know when to use this option).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org