On Tue, Sep 04, 2012 at 09:20:24AM -0600, Eric Blake wrote:

[adding bug-gnulib]

On 09/04/2012 08:49 AM, Jasper Lievisse Adriaanse wrote:

...

...

From b53dc971cc50b5ac397e4568449d25041477c8d6 Mon Sep 17 00:00:00 2001 From: Jasper Lievisse Adriaanse <jasper@humppa.nl> Date: Tue, 4 Sep 2012 16:47:26 +0200 Subject: [PATCH] Pass a correct pointer type to localtime_r(3).

Fixes a warning: warning: passing argument 1 of 'localtime_r' from incompatible pointer type --- tools/virsh-domain.c | 3 ++- tools/virsh.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)

NACK from the libvirt point of view. tv_sec is required by POSIX to be of type time_t; so this is a bug in the OpenBSD header, and gnulib should be working around this bug. OpenBSD's sys/time.h has this:

/* * Structure returned by gettimeofday(2) system call, * and used in other calls. */ struct timeval { long tv_sec; /* seconds */ long tv_usec; /* and microseconds */ }; #ifndef _TIMESPEC_DECLARED #define _TIMESPEC_DECLARED /* * Structure defined by POSIX.1b to be like a timeval. */ struct timespec { time_t tv_sec; /* seconds */ long tv_nsec; /* and nanoseconds */ }; #endif

...

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f0ec742..535779c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3711,6 +3711,7 @@ vshGenFileName(vshControl *ctl, virDomainPtr dom, const char *mime) struct tm time_info; const char *ext = NULL; char *ret = NULL; + time_t sec = (time_t) cur_time.tv_sec;

if (!dom) { vshError(ctl, "%s", _("Invalid domain supplied")); @@ -3724,7 +3725,7 @@ vshGenFileName(vshControl *ctl, virDomainPtr dom, const char *mime) /* add mime type here */

gettimeofday(&cur_time, NULL); - localtime_r(&cur_time.tv_sec, &time_info); + localtime_r(&sec, &time_info); strftime(timestr, sizeof(timestr), "%Y-%m-%d-%H:%M:%S", &time_info);

...

+++ b/tools/virsh.c @@ -2189,6 +2189,7 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, const char *lvl = ""; struct timeval stTimeval; struct tm *stTm; + time_t sec = stTimeval.tv_sec;

if (ctl->log_fd == -1) return; @@ -2199,7 +2200,7 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, * [YYYY.MM.DD HH:MM:SS SIGNATURE PID] LOG_LEVEL message */ gettimeofday(&stTimeval, NULL); - stTm = localtime(&stTimeval.tv_sec); + stTm = localtime(&sec);

Even grosser - why is virsh using localtime() instead of localtime_r()? Oversight probably..

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

-- Cheers, Jasper "Stay Hungry. Stay Foolish"

On 09/04/2012 08:20 AM, Eric Blake wrote:

tv_sec is required by POSIX to be of type time_t; so this is a bug in the OpenBSD header

Most likely this problem arose because of the patch I pushed in gnulib commit e07d7c40f3ca5ec410cf5aa6fa03cfe51e712039. Previously, gnulib required timeval's tv_sec to be the same size as time_t. But now, it requires only that tv_sec be big enough to hold a time_t. This patch was needed for Emacs. Without the patch, gnulib replaced struct timeval on OpenBSD, and this messed up utimens.c, and Emacs wouldn't build. Alternatively, gnulib could substitute its own struct timeval for the system's, wrapping every struct timeval-using function (gettimeofday, futimesat, futimes, lutimes, etc. That'd be more work, though. And it would introduce some performance issues with gettimeofday, which is supposed to be fast. I've been trying to get away from using struct timeval, and to use the higher-resolution struct timespec instead, so messing with these obsolescent interfaces has been lower priority for me. But if someone wants to take the more-ambitious approach that'd be fine, I expect. For this particular case, though, how about if we avoid the problem entirely? libvirt doesn't need to use struct timeval here at all. I'd use the following (untested) patch: it makes libvirt smaller and probably faster, and it ports to OpenBSD without messing with gnulib: diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f0ec742..3cef782 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3707,7 +3707,7 @@ static char * vshGenFileName(vshControl *ctl, virDomainPtr dom, const char *mime) { char timestr[100]; - struct timeval cur_time; + time_t cur_time; struct tm time_info; const char *ext = NULL; char *ret = NULL; @@ -3723,8 +3723,8 @@ vshGenFileName(vshControl *ctl, virDomainPtr dom, const char *mime) ext = ".png"; /* add mime type here */ - gettimeofday(&cur_time, NULL); - localtime_r(&cur_time.tv_sec, &time_info); + time (&cur_time); + localtime_r(&cur_time, &time_info); strftime(timestr, sizeof(timestr), "%Y-%m-%d-%H:%M:%S", &time_info); if (virAsprintf(&ret, "%s-%s%s", virDomainGetName(dom), diff --git a/tools/virsh.c b/tools/virsh.c index 5cf3237..88da429 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2187,7 +2187,7 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, char *str; size_t len; const char *lvl = ""; - struct timeval stTimeval; + time_t stTime; struct tm *stTm; if (ctl->log_fd == -1) @@ -2198,8 +2198,8 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, * * [YYYY.MM.DD HH:MM:SS SIGNATURE PID] LOG_LEVEL message */ - gettimeofday(&stTimeval, NULL); - stTm = localtime(&stTimeval.tv_sec); + time (&stTime); + stTm = localtime(&stTime); virBufferAsprintf(&buf, "[%d.%02d.%02d %02d:%02d:%02d %s %d] ", (1900 + stTm->tm_year), (1 + stTm->tm_mon),