On 01/25/2012 01:13 PM, Marc-André Lureau wrote:

windows.h is included by threads.h. winsock2.h should be included before.

Avoid the following warning:

In file included from ../gnulib/lib/unistd.h:51:0, from ../src/util/util.h:30, from rpc/virkeepalive.c:29: /usr/x86_64-w64-mingw32/sys-root/mingw/include/winsock2.h:15:2: warning: #warning Please include winsock2.h before windows.h [-Wcpp] --- src/rpc/virkeepalive.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)

NACK. I agree that things should be fixed, but the fix should live in the problematic "threads.h" inclusion pattern, not in the downstream virkeepalive.c. Does this alternate patch work for you? (One of these days, I need to install the mingw64 cross-compiler, so I can test it myself.) diff --git i/src/util/threads-win32.h w/src/util/threads-win32.h index 8109afd..1b23402 100644 --- i/src/util/threads-win32.h +++ w/src/util/threads-win32.h @@ -1,7 +1,7 @@ /* * threads-win32.h basic thread synchronization primitives * - * Copyright (C) 2009, 2011 Red Hat, Inc. + * Copyright (C) 2009, 2011-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -21,6 +21,9 @@ #include "internal.h" +#ifdef HAVE_WINSOCK2_H +# include <winsock2.h> +#endif #include <windows.h> struct virMutex { -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Define PID_FORMAT and fix warnings for mingw64 x86_64 build. --- configure.ac | 1 + src/rpc/virnetsocket.c | 4 ++-- src/util/command.c | 10 +++++----- src/util/util.h | 7 +++++++ src/util/virpidfile.c | 6 +++--- 5 files changed, 18 insertions(+), 10 deletions(-) diff --git a/configure.ac b/configure.ac index 9fb7bfc..0dd4a70 100644 --- a/configure.ac +++ b/configure.ac @@ -128,6 +128,7 @@ fi AC_MSG_RESULT([$have_cpuid]) AC_CHECK_SIZEOF([long]) +AC_CHECK_SIZEOF([pid_t]) dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 67d33b7..20bee4b 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -114,7 +114,7 @@ static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr, virNetSocketPtr sock; int no_slow_start = 1; - VIR_DEBUG("localAddr=%p remoteAddr=%p fd=%d errfd=%d pid=%d", + VIR_DEBUG("localAddr=%p remoteAddr=%p fd=%d errfd=%d pid=%" PID_FORMAT, localAddr, remoteAddr, fd, errfd, pid); @@ -174,7 +174,7 @@ static virNetSocketPtr virNetSocketNew(virSocketAddrPtr localAddr, sock->client = isClient; PROBE(RPC_SOCKET_NEW, - "sock=%p refs=%d fd=%d errfd=%d pid=%d localAddr=%s, remoteAddr=%s", + "sock=%p refs=%d fd=%d errfd=%d pid=%" PID_FORMAT " localAddr=%s, remoteAddr=%s", sock, sock->refs, fd, errfd, pid, NULLSTR(sock->localAddrStr), NULLSTR(sock->remoteAddrStr)); diff --git a/src/util/command.c b/src/util/command.c index e3a8371..1e4c206 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -2138,7 +2138,7 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) if (cmd->pid != -1) { virCommandError(VIR_ERR_INTERNAL_ERROR, - _("command is already running as pid %d"), + _("command is already running as pid %" PID_FORMAT), cmd->pid); return -1; } @@ -2214,7 +2214,7 @@ virPidWait(pid_t pid, int *exitstatus) int status; if (pid <= 0) { - virReportSystemError(EINVAL, _("unable to wait for process %d"), pid); + virReportSystemError(EINVAL, _("unable to wait for process %" PID_FORMAT), pid); return -1; } @@ -2223,7 +2223,7 @@ virPidWait(pid_t pid, int *exitstatus) errno == EINTR); if (ret == -1) { - virReportSystemError(errno, _("unable to wait for process %d"), pid); + virReportSystemError(errno, _("unable to wait for process %" PID_FORMAT), pid); return -1; } @@ -2231,7 +2231,7 @@ virPidWait(pid_t pid, int *exitstatus) if (status != 0) { char *st = virCommandTranslateStatus(status); virCommandError(VIR_ERR_INTERNAL_ERROR, - _("Child process (%d) status unexpected: %s"), + _("Child process (%" PID_FORMAT ") status unexpected: %s"), pid, NULLSTR(st)); VIR_FREE(st); return -1; @@ -2387,7 +2387,7 @@ void virPidAbort(pid_t pid) { /* Not yet ported to mingw. Any volunteers? */ - VIR_DEBUG("failed to reap child %d, abandoning it", pid); + VIR_DEBUG("failed to reap child %" PID_FORMAT ", abandoning it", pid); } void diff --git a/src/util/util.h b/src/util/util.h index f62cb42..3e8510d 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -31,6 +31,7 @@ # include <sys/select.h> # include <sys/types.h> # include <stdarg.h> +# include <inttypes.h> # ifndef MIN # define MIN(a, b) ((a) < (b) ? (a) : (b)) @@ -39,6 +40,12 @@ # define MAX(a, b) ((a) > (b) ? (a) : (b)) # endif +#if SIZEOF_PID_T == 4 +# define PID_FORMAT "d" +#elif SIZEOF_PID_T == 8 +# define PID_FORMAT "lld" +#endif + ssize_t saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; ssize_t safewrite(int fd, const void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 1fd6318..f2532d4 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -69,7 +69,7 @@ int virPidFileWritePath(const char *pidfile, goto cleanup; } - if (fprintf(file, "%d", pid) < 0) { + if (fprintf(file, "%" PID_FORMAT, pid) < 0) { rc = -errno; goto cleanup; } @@ -127,7 +127,7 @@ int virPidFileReadPath(const char *path, goto cleanup; } - if (fscanf(file, "%d", pid) != 1) { + if (fscanf(file, "%" PID_FORMAT, pid) != 1) { rc = -EINVAL; VIR_FORCE_FCLOSE(file); goto cleanup; @@ -209,7 +209,7 @@ int virPidFileReadPathIfAlive(const char *path, } #endif - if (virAsprintf(&procpath, "/proc/%d/exe", *pid) < 0) { + if (virAsprintf(&procpath, "/proc/%" PID_FORMAT "/exe", *pid) < 0) { *pid = -1; return -1; } -- 1.7.7.6

Replace calls to fwrite() and fscanf() with more portable-friendly version, such as snprintf() and virStrToLong(). --- src/util/virpidfile.c | 42 +++++++++++++++++++++++++----------------- 1 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index f2532d4..bbcdd53 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -54,7 +54,7 @@ int virPidFileWritePath(const char *pidfile, { int rc; int fd; - FILE *file = NULL; + char pidstr[INT_BUFSIZE_BOUND(pid)]; if ((fd = open(pidfile, O_WRONLY | O_CREAT | O_TRUNC, @@ -63,21 +63,18 @@ int virPidFileWritePath(const char *pidfile, goto cleanup; } - if (!(file = VIR_FDOPEN(fd, "w"))) { - rc = -errno; - VIR_FORCE_CLOSE(fd); - goto cleanup; - } + snprintf(pidstr, sizeof(pidstr), "%" PID_FORMAT, pid); - if (fprintf(file, "%" PID_FORMAT, pid) < 0) { + if (safewrite(fd, pidstr, strlen(pidstr)) < 0) { rc = -errno; + VIR_FORCE_CLOSE(fd); goto cleanup; } rc = 0; cleanup: - if (VIR_FCLOSE(file) < 0) + if (VIR_CLOSE(fd) < 0) rc = -errno; return rc; @@ -117,30 +114,41 @@ cleanup: int virPidFileReadPath(const char *path, pid_t *pid) { - FILE *file; + int fd; int rc; + ssize_t bytes; + char pidstr[INT_BUFSIZE_BOUND(*pid)]; *pid = 0; - if (!(file = fopen(path, "r"))) { + if ((fd = open(path, O_RDONLY)) < 0) { rc = -errno; goto cleanup; } - if (fscanf(file, "%" PID_FORMAT, pid) != 1) { - rc = -EINVAL; - VIR_FORCE_FCLOSE(file); + bytes = saferead(fd, pidstr, sizeof(pidstr)); + if (bytes < 0) { + rc = -errno; + VIR_FORCE_CLOSE(fd); goto cleanup; } + pidstr[bytes] = '\0'; - if (VIR_FCLOSE(file) < 0) { - rc = -errno; +#if SIZEOF_PID_T == 8 + if (virStrToLong_ll(pidstr, NULL, 10, pid) < 0) { +#elif SIZEOF_PID_T == 4 + if (virStrToLong_i(pidstr, NULL, 10, pid) < 0) { +#endif + rc = -1; goto cleanup; } rc = 0; - cleanup: +cleanup: + if (VIR_CLOSE(fd) < 0) + rc = -errno; + return rc; } @@ -367,7 +375,7 @@ int virPidFileAcquirePath(const char *path, /* Someone else must be racing with us, so try agin */ } - snprintf(pidstr, sizeof(pidstr), "%u", (unsigned int)pid); + snprintf(pidstr, sizeof(pidstr), "%" PID_FORMAT, pid); if (safewrite(fd, pidstr, strlen(pidstr)) < 0) { virReportSystemError(errno, -- 1.7.7.6

It seems all the code use pid_t type properly, so it is safe to drop that check now. --- src/util/command.c | 3 --- src/util/virpidfile.c | 2 -- 2 files changed, 0 insertions(+), 5 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 1e4c206..ce992a6 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -50,9 +50,6 @@ virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) -/* We have quite a bit of changes to make if this doesn't hold. */ -verify(sizeof(pid_t) <= sizeof(int)); - /* Flags for virExecWithHook */ enum { VIR_EXEC_NONE = 0, diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index bbcdd53..1fa2de9 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -311,8 +311,6 @@ cleanup: } -verify(sizeof(pid_t) <= sizeof(unsigned int)); - int virPidFileAcquirePath(const char *path, pid_t pid) { -- 1.7.7.6

On 02/01/2012 05:28 PM, Marc-André Lureau wrote:

--- .gnulib | 2 +- bootstrap | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/.gnulib b/.gnulib index dd6b2d7..2f0ed93 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit dd6b2d751b3c6ad417f6a4c48f2adb9d27cc59d2 +Subproject commit 2f0ed93050ab6d858738d36addd8320889e4a0e0

You beat me to this. :) There have indeed been several good upstream fixes in gnulib worth pulling in: * .gnulib dd6b2d7...2f0ed93 (55):

typo autoupdate popen: Make more robust on Windows. Fix date of recent ChangeLog entries. pclose: Fix typo. doc about getlogin_r, setstate. poll tests: Make test more robust. sys_stat: Fix support for mingw64 and MSVC. wcwidth: Work around bug in UTF-8 locale on OpenBSD 5.0. quotearg: Fix test failure on MacOS X 10.5. maint.mk: sc_prohibit_canonicalize_without_use: avoid false positive autoupdate test-framework-sh: Fix test failure with AIX 7.1 diff. strtoimax: eliminate need for stdint.h, inttypes.h checks sys_time: Override 'struct timeval' on some native Windows platforms. accept4, fcntl, socket modules: Avoid warnings on x86_64 mingw64. fcntl: Avoid compilation error on native Windows. select, poll, isatty: Avoid warnings on x86_64 mingw64. doc: clarify README-release maint.mk: use more readable (yet functionally equivalent) quoting stdalign: relax _Alignof and tighten _Alignas test stdalign: Document the last change. Fix bug# typo in previous patch. stdalign: check that alignof and offsetof are consistent update-copyright: accept new option: UPDATE_COPYRIGHT_USE_INTERVALS=2 build-aux/ylwrap: restore x bit pipe2: refine doc about thread-safety posix_spawn_file_actions_addopen: Fix 2012-01-08 commit. pipe2, assign4: document lack of thread-safety in replacement malloca: Avoid warnings on x86_64 mingw64. obstack: remove __STDC__ conditionals havelib: Modern quoting. stdint: Improve support for Android. doc: omit trailing empty lines from INSTALL etc. tests: avoid spurious warnings about gl_sockets_startup locale-fr.m4: Fix for Android. bootstrap: fail when bootstrap_post_import_hook fails update from texinfo maint: enable sc_trailing_blank maint: enable sc_prohibit_openat_without_use maint: enable sc_prohibit_cloexec_without_use maint: enable sc_prohibit_intprops_without_use maint: enable sc_prohibit_hash_pjw_without_use maint: enable double-word-prohibiting rule maint: remove empty lines at EOF, but excluding modules/* maint: add framework to run syntax-check rules against gnulib sources stdint: Add support for Android. autoupdate bootstrap: add bootstrap_post_import_hook gitlog-to-changelog: don't use "no_"-prefixed variable name gitlog-to-changelog: use "||", not "or" in expressions gitlog-to-changelog: new option --no-cluster maint: spell file systems with two words, not one fix a typo bootstrap: add a FIXME comment to ensure we eventually remove the hack

ACK and pushed. I'll review the rest of your series later (ran out of time today); I suspect there's still some pid_t mis-uses to be cleaned up first. Fortunately, beyond the gnulib update, I don't mind making the rest of these sort of changes even after the freeze. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

[adding bug-gnulib] On 01/25/2012 01:13 PM, Marc-André Lureau wrote:

--- tools/virsh.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 999941c..246e638 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3703,7 +3703,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((time_t *)&cur_time.tv_sec, &time_info);

NAK. tv_sec should already be time_t. This is a bug in your system header definition of 'struct timespec', or in something that gnulib has done to that struct in the meantime; and we should make gnulib work around it if we can't get if fixed in mingw. Can you help us identify why we are getting a type mismatch warning from the compiler in the first place? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org