Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls

[Peter Maydell]

On 3 September 2015 at 00:58, Laurent Vivier <address@hidden> wrote:
> This patch introduces a system very similar to the one used in the kernel
> to attach specific functions to a given file descriptor.
>
> In this case, we attach a specific "host_to_target()" translator to the fd
> returned by signalfd() to be able to byte-swap the signalfd_siginfo
> structure provided by read().
>
> This patch allows to execute the example program given by
> man signalfd(2):
>
>  #define _GNU_SOURCE
>  #define _FILE_OFFSET_BITS 64
>  #include <stdio.h>
>  #include <time.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <sys/resource.h>
>
>  #define errExit(msg)     do { perror(msg); exit(EXIT_FAILURE); } while (0)
>
>  int
>  main(int argc, char *argv[])
>  {
>      sigset_t mask;
>      int sfd;
>      struct signalfd_siginfo fdsi;
>      ssize_t s;
>
>      sigemptyset(&mask);
>      sigaddset(&mask, SIGINT);
>      sigaddset(&mask, SIGQUIT);
>
>      /* Block signals so that they aren't handled
>         according to their default dispositions */
>
>      if (sigprocmask(SIG_BLOCK, &mask, NULL) == -1)
>          handle_error("sigprocmask");
>
>      sfd = signalfd(-1, &mask, 0);
>      if (sfd == -1)
>          handle_error("signalfd");
>
>      for (;;) {
>          s = read(sfd, &fdsi, sizeof(struct signalfd_siginfo));
>          if (s != sizeof(struct signalfd_siginfo))
>              handle_error("read");
>
>          if (fdsi.ssi_signo == SIGINT) {
>              printf("Got SIGINT\n");
>          } else if (fdsi.ssi_signo == SIGQUIT) {
>              printf("Got SIGQUIT\n");
>              exit(EXIT_SUCCESS);
>          } else {
>              printf("Read unexpected signal\n");
>          }
>      }
>  }
>
>  $ ./signalfd_demo
>  ^CGot SIGINT
>  ^CGot SIGINT
>  ^\Got SIGQUIT
>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
> v2: Update commit message with example from man page
>     Use CamelCase for struct
>     Allocate entries in the fd array on demand
>     Clear fd entries on open(), close(),...
>     Swap ssi_errno
>     Try to manage dup() and O_CLOEXEC cases
>     Fix signalfd() parameters
>     merge signalfd() and signalfd4()
>     Change the API to only provide functions to process
>     data stream.
>
>     I don't add ssi_addr_lsb in host_to_target_signalfd_siginfo()
>     because it is not in /usr/include/sys/signalfd.h
>
>     TargetFdTrans has an unused field, target_to_host, for symmetry
>     and which could used later with netlink() functions.
>
>  linux-user/syscall.c | 182 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 182 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f62c698..a1cacea 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -60,6 +60,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
>  #include <sys/statfs.h>
>  #include <utime.h>
>  #include <sys/sysinfo.h>
> +#include <sys/signalfd.h>
>  //#include <sys/user.h>
>  #include <netinet/ip.h>
>  #include <netinet/tcp.h>
> @@ -294,6 +295,67 @@ static bitmask_transtbl fcntl_flags_tbl[] = {
>    { 0, 0, 0, 0 }
>  };
>
> +typedef abi_long (*TargetFdFunc)(void *, size_t);
> +struct TargetFdTrans {
> +    TargetFdFunc host_to_target;
> +    TargetFdFunc target_to_host;
> +};
> +typedef struct TargetFdTrans TargetFdTrans;

It's more usual to just combine the struct definition with
the typedef:
  typedef struct TargetFdTrans {
      ...
  } TargetFdTrans;

> +struct TargetFdEntry {
> +    TargetFdTrans *trans;
> +    int flags;
> +};
> +typedef struct TargetFdEntry TargetFdEntry;
> +
> +static TargetFdEntry *target_fd_trans;
> +
> +static int target_fd_max;
> +
> +static TargetFdFunc fd_trans_host_to_target(int fd)
> +{
> +    return fd < target_fd_max && target_fd_trans[fd].trans ?
> +           target_fd_trans[fd].trans->host_to_target : NULL;

I think if you have to split a ?: expression onto two lines it's
a sign it would be clearer as an if ().

> +}
> +
> +static void fd_trans_register(int fd, int flags, TargetFdTrans *trans)
> +{
> +    if (fd >= target_fd_max) {
> +        target_fd_max = ((fd >> 6) + 1) << 6; /* by slice of 64 entries */
> +        target_fd_trans = g_realloc(target_fd_trans,
> +                                    target_fd_max * sizeof(TargetFdEntry));

g_realloc() doesn't zero the extra allocated memory, so you need
to do it manually here.

> +    }
> +    target_fd_trans[fd].flags = flags;
> +    target_fd_trans[fd].trans = trans;
> +}
> +
> +static void fd_trans_unregister(int fd)
> +{
> +    if (fd < target_fd_max) {
> +        target_fd_trans[fd].trans = NULL;
> +        target_fd_trans[fd].flags = 0;
> +    }
> +}
> +
> +static void fd_trans_dup(int oldfd, int newfd, int flags)
> +{
> +    fd_trans_unregister(newfd);
> +    if (oldfd < target_fd_max && target_fd_trans[oldfd].trans) {
> +        fd_trans_register(newfd, flags, target_fd_trans[oldfd].trans);
> +    }
> +}
> +
> +static void fd_trans_close_on_exec(void)
> +{
> +    int fd;
> +
> +    for (fd = 0; fd < target_fd_max; fd++) {
> +        if (target_fd_trans[fd].flags & O_CLOEXEC) {
> +            fd_trans_unregister(fd);
> +        }
> +    }
> +}

I think this one's going to turn out to be unneeded -- see
comment later on.

> +
>  static int sys_getcwd1(char *buf, size_t size)
>  {
>    if (getcwd(buf, size) == NULL) {
> @@ -5246,6 +5308,78 @@ static int do_futex(target_ulong uaddr, int op, int 
> val, target_ulong timeout,
>          return -TARGET_ENOSYS;
>      }
>  }
> +#if defined(TARGET_NR_signalfd) || defined(TARGET_NR_signalfd4)
> +
> +/* signalfd siginfo conversion */
> +
> +static void
> +host_to_target_signalfd_siginfo(struct signalfd_siginfo *tinfo,
> +                                const struct signalfd_siginfo *info)
> +{
> +    int sig = host_to_target_signal(info->ssi_signo);
> +    tinfo->ssi_signo = tswap32(sig);
> +    tinfo->ssi_errno = tswap32(tinfo->ssi_errno);
> +    tinfo->ssi_code = tswap32(info->ssi_code);
> +    tinfo->ssi_pid =  tswap32(info->ssi_pid);
> +    tinfo->ssi_uid =  tswap32(info->ssi_uid);
> +    tinfo->ssi_fd =  tswap32(info->ssi_fd);
> +    tinfo->ssi_tid =  tswap32(info->ssi_tid);
> +    tinfo->ssi_band =  tswap32(info->ssi_band);
> +    tinfo->ssi_overrun =  tswap32(info->ssi_overrun);
> +    tinfo->ssi_trapno =  tswap32(info->ssi_trapno);
> +    tinfo->ssi_status =  tswap32(info->ssi_status);
> +    tinfo->ssi_int =  tswap32(info->ssi_int);
> +    tinfo->ssi_ptr =  tswap64(info->ssi_ptr);
> +    tinfo->ssi_utime =  tswap64(info->ssi_utime);
> +    tinfo->ssi_stime =  tswap64(info->ssi_stime);
> +    tinfo->ssi_addr =  tswap64(info->ssi_addr);

Some of these lines have a stray extra space after the '='.

I said in review on v1 that you were missing
   tinfo->ssi_addr_lsb = tswap16(info->ssi_addr_lsb);
and it's still not here.

Or are you worried about older system include headers not having
that field? (looks like it got added to the kernel in 2010 or so).
If so we could swap it manually, though that would be a bit tedious.

> +}
> +
> +static abi_long host_to_target_signalfd(void *buf, size_t len)
> +{
> +    int i;
> +
> +    for (i = 0; i < len; i += sizeof(struct signalfd_siginfo)) {
> +        host_to_target_signalfd_siginfo(buf + i, buf + i);
> +    }
> +
> +    return len;
> +}
> +
> +static TargetFdTrans target_signalfd_trans = {
> +    .host_to_target = host_to_target_signalfd,
> +};
> +
> +static abi_long do_signalfd4(int fd, abi_long mask, int flags)
> +{
> +    int host_flags = flags & (~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC));

This doesn't look right -- we shouldn't be just passing
through target flags we don't recognise. There are only
two flags we know about and we should just deal with those,
something like:

     if (flags & ~(TARGET_O_NONBLOCK | TARGET_CLOEXEC)) {
         return -TARGET_EINVAL;
     }
     host_flags = target_to_host_bitmask(flags, fcntl_flags_tbl);

> +    target_sigset_t *target_mask;
> +    sigset_t host_mask;
> +    abi_long ret;
> +
> +    if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) {
> +        return -TARGET_EFAULT;
> +    }
> +
> +    target_to_host_sigset(&host_mask, target_mask);
> +
> +    if (flags & TARGET_O_NONBLOCK) {
> +        host_flags |= O_NONBLOCK;
> +    }
> +    if (flags & TARGET_O_CLOEXEC) {
> +        host_flags |= O_CLOEXEC;
> +    }
> +
> +    ret = get_errno(signalfd(fd, &host_mask, host_flags));
> +    if (ret >= 0) {
> +        fd_trans_register(ret, host_flags, &target_signalfd_trans);
> +    }
> +
> +    unlock_user_struct(target_mask, mask, 0);
> +
> +    return ret;
> +}
> +#endif

> @@ -5830,6 +5978,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>                      break;
>                  unlock_user(*q, addr, 0);
>              }
> +            if (ret >= 0) {
> +                fd_trans_close_on_exec();
> +            }

This is execve, right? We can't possibly get here if exec succeeded...

>          }
>          break;

thanks
-- PMM