* [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls
@ 2015-09-02 23:58 Laurent Vivier
2015-09-04 13:35 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2015-09-02 23:58 UTC (permalink / raw)
To: peter.maydell, riku.voipio; +Cc: qemu-devel, Laurent Vivier
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 <laurent@vivier.eu>
---
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;
+
+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;
+}
+
+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));
+ }
+ 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);
+ }
+ }
+}
+
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);
+}
+
+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));
+ 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
/* Map host to target signal numbers for the wait family of syscalls.
Assume all other status bits are the same. */
@@ -5630,6 +5764,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
if (!(p = lock_user(VERIFY_WRITE, arg2, arg3, 0)))
goto efault;
ret = get_errno(read(arg1, p, arg3));
+ if (ret >= 0 &&
+ fd_trans_host_to_target(arg1)) {
+ ret = fd_trans_host_to_target(arg1)(p, ret);
+ }
unlock_user(p, arg2, ret);
}
break;
@@ -5645,6 +5783,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
ret = get_errno(do_openat(cpu_env, AT_FDCWD, p,
target_to_host_bitmask(arg2, fcntl_flags_tbl),
arg3));
+ if (ret >= 0) {
+ fd_trans_unregister(ret);
+ }
unlock_user(p, arg1, 0);
break;
case TARGET_NR_openat:
@@ -5653,9 +5794,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
ret = get_errno(do_openat(cpu_env, arg1, p,
target_to_host_bitmask(arg3, fcntl_flags_tbl),
arg4));
+ if (ret >= 0) {
+ fd_trans_unregister(ret);
+ }
unlock_user(p, arg2, 0);
break;
case TARGET_NR_close:
+ fd_trans_unregister(arg1);
ret = get_errno(close(arg1));
break;
case TARGET_NR_brk:
@@ -5695,6 +5840,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
if (!(p = lock_user_string(arg1)))
goto efault;
ret = get_errno(creat(p, arg2));
+ if (ret >= 0) {
+ fd_trans_unregister(ret);
+ }
unlock_user(p, arg1, 0);
break;
#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();
+ }
}
break;
case TARGET_NR_chdir:
@@ -6130,6 +6281,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
break;
case TARGET_NR_dup:
ret = get_errno(dup(arg1));
+ if (ret >= 0) {
+ fd_trans_dup(arg1, ret, 0);
+ }
break;
case TARGET_NR_pipe:
ret = do_pipe(cpu_env, arg1, 0, 0);
@@ -6222,10 +6376,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
goto unimplemented;
case TARGET_NR_dup2:
ret = get_errno(dup2(arg1, arg2));
+ if (ret >= 0) {
+ fd_trans_dup(arg1, arg2, 0);
+ }
break;
#if defined(CONFIG_DUP3) && defined(TARGET_NR_dup3)
case TARGET_NR_dup3:
ret = get_errno(dup3(arg1, arg2, arg3));
+ if (ret >= 0) {
+ fd_trans_dup(arg1, arg2, arg3);
+ }
break;
#endif
#ifdef TARGET_NR_getppid /* not on alpha */
@@ -7215,6 +7375,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
#ifdef TARGET_NR_socket
case TARGET_NR_socket:
ret = do_socket(arg1, arg2, arg3);
+ if (ret >= 0) {
+ fd_trans_unregister(ret);
+ }
break;
#endif
#ifdef TARGET_NR_socketpair
@@ -9454,6 +9617,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
#if defined(TARGET_NR_eventfd)
case TARGET_NR_eventfd:
ret = get_errno(eventfd(arg1, 0));
+ if (ret >= 0) {
+ fd_trans_unregister(ret);
+ }
break;
#endif
#if defined(TARGET_NR_eventfd2)
@@ -9467,6 +9633,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
host_flags |= O_CLOEXEC;
}
ret = get_errno(eventfd(arg1, host_flags));
+ if (ret >= 0) {
+ fd_trans_unregister(ret);
+ }
break;
}
#endif
@@ -9509,6 +9678,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
break;
#endif
#endif
+#if defined(TARGET_NR_signalfd4)
+ case TARGET_NR_signalfd4:
+ ret = do_signalfd4(arg1, arg2, arg4);
+ break;
+#endif
+#if defined(TARGET_NR_signalfd)
+ case TARGET_NR_signalfd:
+ ret = do_signalfd4(arg1, arg2, 0);
+ break;
+#endif
#if defined(CONFIG_EPOLL)
#if defined(TARGET_NR_epoll_create)
case TARGET_NR_epoll_create:
@@ -9780,6 +9959,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
timer_t htimer = g_posix_timers[timerid];
ret = get_errno(timer_getoverrun(htimer));
}
+ if (ret >= 0) {
+ fd_trans_unregister(ret);
+ }
break;
}
#endif
--
2.4.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls
2015-09-02 23:58 [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls Laurent Vivier
@ 2015-09-04 13:35 ` Peter Maydell
2015-09-04 22:03 ` Laurent Vivier
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2015-09-04 13:35 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Riku Voipio, QEMU Developers
On 3 September 2015 at 00:58, Laurent Vivier <laurent@vivier.eu> 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 <laurent@vivier.eu>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls
2015-09-04 13:35 ` Peter Maydell
@ 2015-09-04 22:03 ` Laurent Vivier
2015-09-28 13:57 ` Riku Voipio
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2015-09-04 22:03 UTC (permalink / raw)
To: Peter Maydell; +Cc: Riku Voipio, QEMU Developers
Le 04/09/2015 15:35, Peter Maydell a écrit :
> On 3 September 2015 at 00:58, Laurent Vivier <laurent@vivier.eu> 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 <laurent@vivier.eu>
>> ---
>> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls
2015-09-04 22:03 ` Laurent Vivier
@ 2015-09-28 13:57 ` Riku Voipio
2015-09-28 16:23 ` Laurent Vivier
0 siblings, 1 reply; 6+ messages in thread
From: Riku Voipio @ 2015-09-28 13:57 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Peter Maydell, QEMU Developers
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 <laurent@vivier.eu> 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 <laurent@vivier.eu>
> >> ---
> >> 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.
> >> +}
> >> +
> >> +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
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls
2015-09-28 13:57 ` Riku Voipio
@ 2015-09-28 16:23 ` Laurent Vivier
2015-09-28 18:48 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2015-09-28 16:23 UTC (permalink / raw)
To: Riku Voipio; +Cc: Peter Maydell, QEMU Developers
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 <laurent@vivier.eu> 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 <laurent@vivier.eu>
>>>> ---
>>>> 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
>>>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls
2015-09-28 16:23 ` Laurent Vivier
@ 2015-09-28 18:48 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2015-09-28 18:48 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Riku Voipio, QEMU Developers
On 28 September 2015 at 17:23, Laurent Vivier <laurent@vivier.eu> wrote:
> 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.
If it's not too painful I think it would be preferable to convert it.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-28 18:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-02 23:58 [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls Laurent Vivier
2015-09-04 13:35 ` Peter Maydell
2015-09-04 22:03 ` Laurent Vivier
2015-09-28 13:57 ` Riku Voipio
2015-09-28 16:23 ` Laurent Vivier
2015-09-28 18:48 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).