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

[Laurent Vivier]

Le 04/09/2015 15:35, Peter Maydell a écrit :
> 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;

ok

>> +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 ().

ok

> 
>> +}
>> +
>> +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.

ok

>> +    }
>> +    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.

My fedora 22 (2015) doesn't have this field in
/usr/include/sys/signalfd.h, but it is in /usr/include/linux/signalfd.h.

But unfortunately, the first file is the one we use (the second, I
guess, is for the kernel). Or did I miss something ?

>> +}
>> +
>> +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...

You're right...

> 
>>          }
>>          break;
> 
> thanks

Thank you for the review...

> -- PMM
>