Re: [Qemu-devel] [PATCH v2 05/39] fdsets: use weak aliases instead of qemu-tool.c/qemu-user.c

[Stefan Weil]

Am 31.10.2012 16:30, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini<address@hidden>
> ---
>   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