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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Tue, Jul 02, 2013 at 02:09:24PM +0000, Seiji Aguchi wrote:
> > > 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 
> 
> 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

Daniel's statement was about the code you copied from libvirt.  It was
not about using the glib function, which simplifies things greatly and
avoids the need for a test suite.

Patches need to make sense today, please do not add extra code with
potential future use in mind:

1. Other developers must be able to read and modify the current codebase
   on its own.  They do not know what potential future changes you were
   thinking about.

2. You may never end up submitting or getting the future stuff upstream.
   Then we'd be left with extra layers that are never used.

Stefan