Re: [Qemu-devel] [PATCH 6/6] linux-user: Fix siginfo handling

[Andreas Färber]

Am 15.09.2012 22:24, schrieb Richard Henderson:
> Compare signal numbers in the proper domain.
> Convert all of the fields for SIGIO and SIGCHLD.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  linux-user/qemu.h    |  3 +++
>  linux-user/signal.c  | 59 
> +++++++++++++++++++++++++++++++++++-----------------
>  linux-user/syscall.c |  2 +-
>  3 files changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 69b27d7..8f871eb 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -219,6 +219,9 @@ unsigned long init_guest_space(unsigned long host_start,
>  
>  #include "qemu-log.h"
>  
> +/* syscall.c */
> +int host_to_target_waitstatus(int status);
> +
>  /* strace.c */
>  void print_syscall(int num,
>                     abi_long arg1, abi_long arg2, abi_long arg3,
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index bf2dfb8..9842ba6 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -202,46 +202,67 @@ void target_to_host_old_sigset(sigset_t *sigset,
>  static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
>                                                   const siginfo_t *info)
>  {
> -    int sig;
> -    sig = host_to_target_signal(info->si_signo);
> +    int sig = host_to_target_signal(info->si_signo);
>      tinfo->si_signo = sig;
>      tinfo->si_errno = 0;
>      tinfo->si_code = info->si_code;
> -    if (sig == SIGILL || sig == SIGFPE || sig == SIGSEGV ||
> -        sig == SIGBUS || sig == SIGTRAP) {
> -        /* should never come here, but who knows. The information for
> -           the target is irrelevant */
> +
> +    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> +        || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> +        /* Should never come here, but who knows. The information for
> +           the target is irrelevant.  */
>          tinfo->_sifields._sigfault._addr = 0;
> -    } else if (sig == SIGIO) {
> +    } else if (sig == TARGET_SIGIO) {
> +        tinfo->_sifields._sigpoll._band = info->si_band;
>       tinfo->_sifields._sigpoll._fd = info->si_fd;
> +    } else if (sig == TARGET_SIGCHLD) {
> +        tinfo->_sifields._sigchld._pid = info->si_pid;
> +        tinfo->_sifields._sigchld._uid = info->si_uid;
> +        tinfo->_sifields._sigchld._status
> +            = host_to_target_waitstatus(info->si_status);
> +        tinfo->_sifields._sigchld._utime = info->si_utime;
> +        tinfo->_sifields._sigchld._stime = info->si_stime;
>      } else if (sig >= TARGET_SIGRTMIN) {
>          tinfo->_sifields._rt._pid = info->si_pid;
>          tinfo->_sifields._rt._uid = info->si_uid;
>          /* XXX: potential problem if 64 bit */
> -        tinfo->_sifields._rt._sigval.sival_ptr =
> -            (abi_ulong)(unsigned long)info->si_value.sival_ptr;
> +        tinfo->_sifields._rt._sigval.sival_ptr
> +            = (abi_ulong)(unsigned long)info->si_value.sival_ptr;

I don't spot any functional change here ...

>      }
>  }
>  
>  static void tswap_siginfo(target_siginfo_t *tinfo,
>                            const target_siginfo_t *info)
>  {
> -    int sig;
> -    sig = info->si_signo;
> +    int sig = info->si_signo;
>      tinfo->si_signo = tswap32(sig);
>      tinfo->si_errno = tswap32(info->si_errno);
>      tinfo->si_code = tswap32(info->si_code);
> -    if (sig == SIGILL || sig == SIGFPE || sig == SIGSEGV ||
> -        sig == SIGBUS || sig == SIGTRAP) {
> -        tinfo->_sifields._sigfault._addr =
> -            tswapal(info->_sifields._sigfault._addr);
> -    } else if (sig == SIGIO) {
> -     tinfo->_sifields._sigpoll._fd = tswap32(info->_sifields._sigpoll._fd);
> +
> +    if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> +        || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> +        tinfo->_sifields._sigfault._addr
> +            = tswapal(info->_sifields._sigfault._addr);
> +    } else if (sig == TARGET_SIGIO) {
> +        tinfo->_sifields._sigpoll._band
> +            = tswap32(info->_sifields._sigpoll._band);
> +        tinfo->_sifields._sigpoll._fd = 
> tswap32(info->_sifields._sigpoll._fd);
> +    } else if (sig == TARGET_SIGCHLD) {
> +        tinfo->_sifields._sigchld._pid
> +            = tswap32(info->_sifields._sigchld._pid);
> +        tinfo->_sifields._sigchld._uid
> +            = tswap32(info->_sifields._sigchld._uid);
> +        tinfo->_sifields._sigchld._status
> +            = tswap32(info->_sifields._sigchld._status);
> +        tinfo->_sifields._sigchld._utime
> +            = tswapal(info->_sifields._sigchld._utime);
> +        tinfo->_sifields._sigchld._stime
> +            = tswapal(info->_sifields._sigchld._stime);
>      } else if (sig >= TARGET_SIGRTMIN) {
>          tinfo->_sifields._rt._pid = tswap32(info->_sifields._rt._pid);
>          tinfo->_sifields._rt._uid = tswap32(info->_sifields._rt._uid);
> -        tinfo->_sifields._rt._sigval.sival_ptr =
> -            tswapal(info->_sifields._rt._sigval.sival_ptr);
> +        tinfo->_sifields._rt._sigval.sival_ptr
> +            = tswapal(info->_sifields._rt._sigval.sival_ptr);

... or here. Does Coding Style mandate this? Would be nice to separate
from functional changes then or to leave out.

Otherwise no functional issue spotted, using TARGET_SIG* after
conversion certainly makes sense. :)

Andreas

>      }
>  }
>  
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 925e579..3676c72 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4920,7 +4920,7 @@ static int do_futex(target_ulong uaddr, int op, int 
> val, target_ulong timeout,
>  
>  /* Map host to target signal numbers for the wait family of syscalls.
>     Assume all other status bits are the same.  */
> -static int host_to_target_waitstatus(int status)
> +int host_to_target_waitstatus(int status)
>  {
>      if (WIFSIGNALED(status)) {
>          return host_to_target_signal(WTERMSIG(status)) | (status & ~0x7f);
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg