[libvirt] [PATCH] Avoid pthread_sigmask on Win32 platforms
Win32 doesn't have a concept of signal masks so disable that code. It is unclear how SIGINT is delivered (if at all) on Win32, so this might further work to provide an alternative to pthread_sigmask * tools/virsh.c: Avoid pthread_sigmask on Win32 --- tools/virsh.c | 20 +++++++++++++++----- 1 files changed, 15 insertions(+), 5 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index be2cd67..6d9861f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3489,15 +3489,17 @@ doMigrate (void *opaque) const char *migrateuri; const char *dname; int flags = 0, found; - sigset_t sigmask, oldsigmask; vshCtrlData *data = opaque; vshControl *ctl = data->ctl; const vshCmd *cmd = data->cmd; +#if HAVE_PTHREAD_SIGMASK + sigset_t sigmask, oldsigmask; sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) goto out_sig; +#endif if (!vshConnectionUsability (ctl, ctl->conn)) goto out; @@ -3563,8 +3565,10 @@ doMigrate (void *opaque) } out: +#if HAVE_PTHREAD_SIGMASK pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); out_sig: +#endif if (dom) virDomainFree (dom); ignore_value(safewrite(data->writefd, &ret, sizeof(ret))); } @@ -3607,12 +3611,16 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) struct sigaction old_sig_action; virDomainJobInfo jobinfo; bool verbose = false; - sigset_t sigmask, oldsigmask; int timeout; struct timeval start, curr; bool live_flag = false; - vshCtrlData data; +#if HAVE_PTHREAD_SIGMASK + sigset_t sigmask, oldsigmask; + + sigemptyset(&sigmask); + sigaddset(&sigmask, SIGINT); +#endif if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; @@ -3665,8 +3673,6 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) pollfd.fd = p[0]; pollfd.events = POLLIN; pollfd.revents = 0; - sigemptyset(&sigmask); - sigaddset(&sigmask, SIGINT); GETTIMEOFDAY(&start); while (1) { @@ -3709,9 +3715,13 @@ repoll: } if (verbose) { +#if HAVE_PTHREAD_SIGMASK pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); +#endif ret = virDomainGetJobInfo(dom, &jobinfo); +#if HAVE_PTHREAD_SIGMASK pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); +#endif if (ret == 0) print_job_progress(jobinfo.dataRemaining, jobinfo.dataTotal); } -- 1.7.4
2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
Win32 doesn't have a concept of signal masks so disable that code. It is unclear how SIGINT is delivered (if at all) on Win32, so this might further work to provide an alternative to pthread_sigmask
* tools/virsh.c: Avoid pthread_sigmask on Win32 ---
ACK. This fixes one of the current problems on Win32. Matthias
On Thu, Feb 10, 2011 at 03:20:37PM +0100, Matthias Bolte wrote:
2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
Win32 doesn't have a concept of signal masks so disable that code. It is unclear how SIGINT is delivered (if at all) on Win32, so this might further work to provide an alternative to pthread_sigmask
* tools/virsh.c: Avoid pthread_sigmask on Win32 ---
ACK. This fixes one of the current problems on Win32.
This was the only problem I see with Win32. What others do you see ? Regards, 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 :|
2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
On Thu, Feb 10, 2011 at 03:20:37PM +0100, Matthias Bolte wrote:
2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
Win32 doesn't have a concept of signal masks so disable that code. It is unclear how SIGINT is delivered (if at all) on Win32, so this might further work to provide an alternative to pthread_sigmask
* tools/virsh.c: Avoid pthread_sigmask on Win32 ---
ACK. This fixes one of the current problems on Win32.
This was the only problem I see with Win32. What others do you see ?
Regards, Daniel
I get errors like this related to %lld in format strings: virsh.c: In function 'cmdDomblkstat': virsh.c:990:9: warning: unknown conversion type character 'l' in format [-Wformat] The problem goes away when I replace the define for vshPrint #define vshPrint(ctl, ...) fprintf(stdout, __VA_ARGS__) by this function static void vshPrint(vshControl *ctl ATTRIBUTE_UNUSED, const char *format, ...) { va_list ap; va_start(ap, format); vfprintf(stdout, format, ap); va_end(ap); } I'm not sure why this became a problem now, as vshPrint is a define since 2006 and virsh used to compile before. Maybe this is an issue with gnulib in the current libvirt-0.8.8-rc1 tarball, as I'm testing based on this tarball it. Are you compiling this from a git checkout or a tarball? Matthias
On Thu, Feb 10, 2011 at 03:38:08PM +0100, Matthias Bolte wrote:
2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
On Thu, Feb 10, 2011 at 03:20:37PM +0100, Matthias Bolte wrote:
2011/2/10 Daniel P. Berrange <berrange@redhat.com>:
Win32 doesn't have a concept of signal masks so disable that code. It is unclear how SIGINT is delivered (if at all) on Win32, so this might further work to provide an alternative to pthread_sigmask
* tools/virsh.c: Avoid pthread_sigmask on Win32 ---
ACK. This fixes one of the current problems on Win32.
This was the only problem I see with Win32. What others do you see ?
Regards, Daniel
I get errors like this related to %lld in format strings:
virsh.c: In function 'cmdDomblkstat': virsh.c:990:9: warning: unknown conversion type character 'l' in format [-Wformat]
The problem goes away when I replace the define for vshPrint
#define vshPrint(ctl, ...) fprintf(stdout, __VA_ARGS__)
by this function
static void vshPrint(vshControl *ctl ATTRIBUTE_UNUSED, const char *format, ...)
I'm surprised you didn't need to annotate this with ATTRIBUTE_FMT_PRINTF, otherwise gcc would assume win32 printf style, rather than gnu IIUC.
{ va_list ap;
va_start(ap, format); vfprintf(stdout, format, ap); va_end(ap); }
I'm not sure why this became a problem now, as vshPrint is a define since 2006 and virsh used to compile before. Maybe this is an issue with gnulib in the current libvirt-0.8.8-rc1 tarball, as I'm testing based on this tarball it.
Are you compiling this from a git checkout or a tarball?
I'm using GIT. 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 :|
On 02/10/2011 07:38 AM, Matthias Bolte wrote:
I get errors like this related to %lld in format strings:
virsh.c: In function 'cmdDomblkstat': virsh.c:990:9: warning: unknown conversion type character 'l' in format [-Wformat]
The problem goes away when I replace the define for vshPrint
#define vshPrint(ctl, ...) fprintf(stdout, __VA_ARGS__)
by this function
static void vshPrint(vshControl *ctl ATTRIBUTE_UNUSED, const char *format, ...) { va_list ap;
va_start(ap, format); vfprintf(stdout, format, ap); va_end(ap); }
Well yeah, writing a wrapper functions makes the warnings go away if the wrapper doesn't have the write __attribute__ attached to it; but it doesn't fix the real problem, which is that we aren't using the fprintf-posix gnulib module, and therefore, we will still cause issues on mingw trying to format with %lld. A better alternative is to instead use [v]asprintf (where we ARE using the gnulib module, and %lld works), and print that resulting string. I'll propose a patch.
I'm not sure why this became a problem now, as vshPrint is a define since 2006 and virsh used to compile before. Maybe this is an issue with gnulib in the current libvirt-0.8.8-rc1 tarball, as I'm testing based on this tarball it.
I'm not sure either, unless you have recently upgraded mingw headers and the headers recently added a gcc attribute to fprintf where it used to not have one. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 02/11/2011 01:42 PM, Eric Blake wrote:
Well yeah, writing a wrapper functions makes the warnings go away if the wrapper doesn't have the write __attribute__ attached to it; but it
s/write/right (aka __gnu_printf__)/ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* tools/virsh.c (vshPrint): Change redirect. (vshPrintExtra): Allow use within vshPrint. Avoid fprintf on arbitrary formats, since we aren't using gnulib module; instead, use virVasprintf to pre-format. (vshError): Likewise. Reported by Matthias Bolte. --- This passed for my fedora->mingw cross compile, but I'd like to make sure it worked for your native mingw build setup. tools/virsh.c | 21 ++++++++++++++++----- 1 files changed, 16 insertions(+), 5 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 6d9861f..c2d165d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -325,7 +325,7 @@ static void vshDebug(vshControl *ctl, int level, const char *format, ...) ATTRIBUTE_FMT_PRINTF(3, 4); /* XXX: add batch support */ -#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) +#define vshPrint(_ctl, ...) vshPrintExtra(NULL, __VA_ARGS__) static const char *vshDomainStateToString(int state); static const char *vshDomainVcpuStateToString(int state); @@ -11574,13 +11574,20 @@ static void vshPrintExtra(vshControl *ctl, const char *format, ...) { va_list ap; + char *str; - if (ctl->quiet == TRUE) + if (ctl && ctl->quiet == TRUE) return; va_start(ap, format); - vfprintf(stdout, format, ap); + if (virVasprintf(&str, format, ap) < 0) { + vshError(ctl, "%s", _("Out of memory")); + va_end(ap); + return; + } va_end(ap); + fprintf(stdout, "%s", str); + VIR_FREE(str); } @@ -11588,6 +11595,7 @@ static void vshError(vshControl *ctl, const char *format, ...) { va_list ap; + char *str; if (ctl != NULL) { va_start(ap, format); @@ -11598,10 +11606,13 @@ vshError(vshControl *ctl, const char *format, ...) fputs(_("error: "), stderr); va_start(ap, format); - vfprintf(stderr, format, ap); + /* We can't recursively call vshError on an OOM situation, so ignore + failure here. */ + ignore_value(virVasprintf(&str, format, ap)); va_end(ap); - fputc('\n', stderr); + fprintf(stderr, "%s\n", NULLSTR(str)); + VIR_FREE(str); } /* -- 1.7.4
2011/2/12 Eric Blake <eblake@redhat.com>:
* tools/virsh.c (vshPrint): Change redirect. (vshPrintExtra): Allow use within vshPrint. Avoid fprintf on arbitrary formats, since we aren't using gnulib module; instead, use virVasprintf to pre-format. (vshError): Likewise. Reported by Matthias Bolte. ---
This passed for my fedora->mingw cross compile, but I'd like to make sure it worked for your native mingw build setup.
It does, ACK. Matthias
On 02/12/2011 04:54 AM, Matthias Bolte wrote:
2011/2/12 Eric Blake <eblake@redhat.com>:
* tools/virsh.c (vshPrint): Change redirect. (vshPrintExtra): Allow use within vshPrint. Avoid fprintf on arbitrary formats, since we aren't using gnulib module; instead, use virVasprintf to pre-format. (vshError): Likewise. Reported by Matthias Bolte. ---
This passed for my fedora->mingw cross compile, but I'd like to make sure it worked for your native mingw build setup.
It does, ACK.
Thanks. Since this commit was about mingw compiler warnings, I amended it to also include a gnulib update - there was a single gnulib commit since last update, which solves a compiler warning present on mingw when compiling gnulib's strptime. Then I pushed it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Matthias Bolte