qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Laurent Vivier
Subject: Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls
Date: Mon, 28 Sep 2015 18:23:56 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0


Le 28/09/2015 15:57, Riku Voipio a écrit :
> On Sat, Sep 05, 2015 at 12:03:11AM +0200, Laurent Vivier wrote:
>>
>>
>> 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 ?
> 
> Was a conclusion reached here? A quick codesearch.debian.net search 
> doesn't find any userspace code using ssi_addr_lsb.

No conclusion was reached here.
I'm ready to send a patch to fix other comments.

Peter, if you really want this field, I can play with the padfields to
convert it. Just say.

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]