Re: [Dle-develop] [Qemu-devel] [RFC][PATCH v2]Add timestamp to error message

[Anthony L. <ali...@us...>]

"Daniel P. Berrange" <ber...@re...> writes:

> On Wed, May 01, 2013 at 06:16:33AM -0600, Eric Blake wrote:
>> On 05/01/2013 06:05 AM, Stefan Hajnoczi wrote:
>> 
>> >> +    error_printf(
>> >> +           "%4d-%02d-%02d %02d:%02d:%02d.%03lld+0000 ",
>> >> +           fields.tm_year + 1900, fields.tm_mon + 1, fields.tm_mday,
>> >> +           fields.tm_hour, fields.tm_min, fields.tm_sec,
>> >> +           now % 1000);
>> > 
>> > Can strftime(3) be used instead of copying code from glibc?
>> 
>> No, because glibc's strftime() is not async-signal safe, and therefore
>> is not safe to call between fork() and exec() (libvirt hit actual
>> deadlocks[1] where a child was blocked on a mutex associated with
>> time-related functions that happened to be held by another parent
>> thread, but where the fork nuked that other thread so the mutex would
>> never clear).  This code is copied from libvirt, which copied the
>> no-lock-needed safe portions of glibc for a reason.
>> 
>> [1] https://www.redhat.com/archives/libvir-list/2011-November/msg01609.html
>
> NB the original libvirt code had this & other related functions in
> a standalone file, along with a significant test suite. I think it
> is better from a maintenence POV to keep this functionality in a
> file that's separate from the qemu error file, so it can be reused
> elsewhere in QEMU if needed. It should also copy the test suite,
> since it is bad practice to throw away tests which already exist.

I also don't buy that we can't use strftime.  There are very few places
where we use fork() in QEMU (it doesn't exist on Windows so it can't be
used without protection).  None of those places use the error reporting
infrastructure.

This code is also extremely naive.  It doesn't take into account leap
seconds and makes bad assumptions about leap years.

Regards,

Anthony Liguori

>
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|