Patchwork [v2,05/39] fdsets: use weak aliases instead of qemu-tool.c/qemu-user.c

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 31, 2012, 3:30 p.m.
Message ID <1351697456-16107-6-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/195946/
State New
Headers show

Comments

Paolo Bonzini - Oct. 31, 2012, 3:30 p.m.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cutils.c      |  5 -----
 osdep.c       | 30 ++++++++++++++++++++++++++++++
 qemu-common.h |  1 -
 qemu-tool.c   | 20 --------------------
 qemu-user.c   | 20 --------------------
 5 file modificati, 30 inserzioni(+), 46 rimozioni(-)
Stefan Weil - Nov. 15, 2012, 6:01 p.m.
Am 31.10.2012 16:30, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   cutils.c      |  5 -----
>   osdep.c       | 30 ++++++++++++++++++++++++++++++
>   qemu-common.h |  1 -
>   qemu-tool.c   | 20 --------------------
>   qemu-user.c   | 20 --------------------
>   5 file modificati, 30 inserzioni(+), 46 rimozioni(-)
>
> diff --git a/cutils.c b/cutils.c
> index 6f9f799..4f0692f 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -280,11 +280,6 @@ int qemu_parse_fd(const char *param)
>       return fd;
>   }
>
> -int qemu_parse_fdset(const char *param)
> -{
> -    return qemu_parse_fd(param);
> -}
> -
>   /* round down to the nearest power of 2*/
>   int64_t pow2floor(int64_t value)
>   {
> diff --git a/osdep.c b/osdep.c
> index 3b25297..0061f74 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -144,6 +144,11 @@ fail:
>       errno = serrno;
>       return -1;
>   }
> +
> +static int qemu_parse_fdset(const char *param)
> +{
> +    return qemu_parse_fd(param);
> +}
>   #endif
>
>   /*
> @@ -404,3 +409,28 @@ bool fips_get_state(void)
>   {
>       return fips_enabled;
>   }
> +
> +
> +static int default_fdset_get_fd(int64_t fdset_id, int flags)
> +{
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_fdset_get_fd, default_fdset_get_fd);
> +
> +static int default_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> +{
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_add, default_fdset_dup_fd_add);
> +
> +static int default_fdset_dup_fd_remove(int dup_fd)
> +{
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_remove);
> +
> +static int default_fdset_dup_fd_find(int dup_fd)
> +{
> +    return -1;
> +}
> +QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_find, default_fdset_dup_fd_find);
> diff --git a/qemu-common.h b/qemu-common.h
> index b54612b..36ce522 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -167,7 +167,6 @@ int qemu_fls(int i);
>   int qemu_fdatasync(int fd);
>   int fcntl_setfl(int fd, int flag);
>   int qemu_parse_fd(const char *param);
> -int qemu_parse_fdset(const char *param);
>
>   /*
>    * strtosz() suffixes used to specify the default treatment of an
> diff --git a/qemu-tool.c b/qemu-tool.c
> index f2f9813..84273ae 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -68,26 +68,6 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>   {
>   }
>
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> -{
> -    return -1;
> -}
> -
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> -{
> -    return -1;
> -}
> -
> -int monitor_fdset_dup_fd_remove(int dup_fd)
> -{
> -    return -1;
> -}
> -
> -int monitor_fdset_dup_fd_find(int dup_fd)
> -{
> -    return -1;
> -}
> -
>   int64_t cpu_get_clock(void)
>   {
>       return qemu_get_clock_ns(rt_clock);
> diff --git a/qemu-user.c b/qemu-user.c
> index 13fb9ae..08ccb0f 100644
> --- a/qemu-user.c
> +++ b/qemu-user.c
> @@ -35,23 +35,3 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>   void monitor_set_error(Monitor *mon, QError *qerror)
>   {
>   }
> -
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> -{
> -    return -1;
> -}
> -
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> -{
> -    return -1;
> -}
> -
> -int monitor_fdset_dup_fd_remove(int dup_fd)
> -{
> -    return -1;
> -}
> -
> -int monitor_fdset_dup_fd_find(int dup_fd)
> -{
> -    return -1;
> -}


Hi Paolo,

this patch breaks QEMU on 32 and 64 bit hosts, native and with Wine.
It's easy to reproduce the SIGSEGV crash: just add a -snapshot option.
Obviously the critical code is executed only when this option was used.

Here is a simple command line using Wine:

wine i386-softmmu/qemu-system-i386 -L pc-bios -snapshot Makefile

The disk image does not matter, so I just selected QEMU's Makefile.

It looks like weak symbols are not really working with MinGW
(Blue Swirl previously pointed out that only ELF and a.out are
officially supported).

I can see in the debugger that QEMU wants to call monitor_fdset_dup_fd_find
from qemu_close.

In previous versions, this was just a dummy function returning 0.
Now, it is the function in monitor.c, but the address does not match
exactly, so the code addresses lines near the beginning of
monitor_fdset_dup_fd_find which does not work of course.

A trivial workaround is calling default_fdset_dup_fd_find which
restores the old behaviour. I expect that all other weak functions
would show the same problem if they were used.

Regards,

Stefan
Paolo Bonzini - Nov. 15, 2012, 8:52 p.m.
Il 15/11/2012 19:01, Stefan Weil ha scritto:
> Hi Paolo,
> 
> this patch breaks QEMU on 32 and 64 bit hosts, native and with Wine.
> It's easy to reproduce the SIGSEGV crash: just add a -snapshot option.
> Obviously the critical code is executed only when this option was used.

I cannot reproduce this, so it must be an assembler or linker bug.

Can you try the alternative code that is used for Mac OS X?

Paolo

> Here is a simple command line using Wine:
> 
> wine i386-softmmu/qemu-system-i386 -L pc-bios -snapshot Makefile
> 
> The disk image does not matter, so I just selected QEMU's Makefile.
> 
> It looks like weak symbols are not really working with MinGW
> (Blue Swirl previously pointed out that only ELF and a.out are
> officially supported).
> 
> I can see in the debugger that QEMU wants to call monitor_fdset_dup_fd_find
> from qemu_close.
> 
> In previous versions, this was just a dummy function returning 0.
> Now, it is the function in monitor.c, but the address does not match
> exactly, so the code addresses lines near the beginning of
> monitor_fdset_dup_fd_find which does not work of course.
> 
> A trivial workaround is calling default_fdset_dup_fd_find which
> restores the old behaviour. I expect that all other weak functions
> would show the same problem if they were used.
> 
> Regards,
> 
> Stefan
> 
>

Patch

diff --git a/cutils.c b/cutils.c
index 6f9f799..4f0692f 100644
--- a/cutils.c
+++ b/cutils.c
@@ -280,11 +280,6 @@  int qemu_parse_fd(const char *param)
     return fd;
 }
 
-int qemu_parse_fdset(const char *param)
-{
-    return qemu_parse_fd(param);
-}
-
 /* round down to the nearest power of 2*/
 int64_t pow2floor(int64_t value)
 {
diff --git a/osdep.c b/osdep.c
index 3b25297..0061f74 100644
--- a/osdep.c
+++ b/osdep.c
@@ -144,6 +144,11 @@  fail:
     errno = serrno;
     return -1;
 }
+
+static int qemu_parse_fdset(const char *param)
+{
+    return qemu_parse_fd(param);
+}
 #endif
 
 /*
@@ -404,3 +409,28 @@  bool fips_get_state(void)
 {
     return fips_enabled;
 }
+
+
+static int default_fdset_get_fd(int64_t fdset_id, int flags)
+{
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_fdset_get_fd, default_fdset_get_fd);
+
+static int default_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
+{
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_add, default_fdset_dup_fd_add);
+
+static int default_fdset_dup_fd_remove(int dup_fd)
+{
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_remove, default_fdset_dup_fd_remove);
+
+static int default_fdset_dup_fd_find(int dup_fd)
+{
+    return -1;
+}
+QEMU_WEAK_ALIAS(monitor_fdset_dup_fd_find, default_fdset_dup_fd_find);
diff --git a/qemu-common.h b/qemu-common.h
index b54612b..36ce522 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -167,7 +167,6 @@  int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
 int qemu_parse_fd(const char *param);
-int qemu_parse_fdset(const char *param);
 
 /*
  * strtosz() suffixes used to specify the default treatment of an
diff --git a/qemu-tool.c b/qemu-tool.c
index f2f9813..84273ae 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -68,26 +68,6 @@  void monitor_protocol_event(MonitorEvent event, QObject *data)
 {
 }
 
-int monitor_fdset_get_fd(int64_t fdset_id, int flags)
-{
-    return -1;
-}
-
-int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
-{
-    return -1;
-}
-
-int monitor_fdset_dup_fd_remove(int dup_fd)
-{
-    return -1;
-}
-
-int monitor_fdset_dup_fd_find(int dup_fd)
-{
-    return -1;
-}
-
 int64_t cpu_get_clock(void)
 {
     return qemu_get_clock_ns(rt_clock);
diff --git a/qemu-user.c b/qemu-user.c
index 13fb9ae..08ccb0f 100644
--- a/qemu-user.c
+++ b/qemu-user.c
@@ -35,23 +35,3 @@  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 void monitor_set_error(Monitor *mon, QError *qerror)
 {
 }
-
-int monitor_fdset_get_fd(int64_t fdset_id, int flags)
-{
-    return -1;
-}
-
-int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
-{
-    return -1;
-}
-
-int monitor_fdset_dup_fd_remove(int dup_fd)
-{
-    return -1;
-}
-
-int monitor_fdset_dup_fd_find(int dup_fd)
-{
-    return -1;
-}