[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
>>>
>>