Le 04/09/2015 15:35, Peter Maydell a écrit :
> On 3 September 2015 at 00:58, Laurent Vivier <laur...@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 <laur...@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
>
