From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52399) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgYvl-00028w-7b for qemu-devel@nongnu.org; Mon, 28 Sep 2015 09:57:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZgYvh-00052s-V7 for qemu-devel@nongnu.org; Mon, 28 Sep 2015 09:57:25 -0400 Received: from jessie.kos.to ([212.47.231.226]:45170 helo=pilvi.kos.to) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgYvh-00052g-L3 for qemu-devel@nongnu.org; Mon, 28 Sep 2015 09:57:21 -0400 Date: Mon, 28 Sep 2015 16:57:19 +0300 From: Riku Voipio Message-ID: <20150928135719.GF20506@kos.to> References: <1441238315-18409-1-git-send-email-laurent@vivier.eu> <55EA151F.1090703@vivier.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <55EA151F.1090703@vivier.eu> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: Peter Maydell , QEMU Developers On Sat, Sep 05, 2015 at 12:03:11AM +0200, Laurent Vivier wrote: >=20 >=20 > Le 04/09/2015 15:35, Peter Maydell a =C3=A9crit : > > On 3 September 2015 at 00:58, Laurent Vivier wrot= e: > >> This patch introduces a system very similar to the one used in the k= ernel > >> 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 > >> #include > >> #include > >> #include > >> #include > >> > >> #define errExit(msg) do { perror(msg); exit(EXIT_FAILURE); } wh= ile (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) =3D=3D -1) > >> handle_error("sigprocmask"); > >> > >> sfd =3D signalfd(-1, &mask, 0); > >> if (sfd =3D=3D -1) > >> handle_error("signalfd"); > >> > >> for (;;) { > >> s =3D read(sfd, &fdsi, sizeof(struct signalfd_siginfo)); > >> if (s !=3D sizeof(struct signalfd_siginfo)) > >> handle_error("read"); > >> > >> if (fdsi.ssi_signo =3D=3D SIGINT) { > >> printf("Got SIGINT\n"); > >> } else if (fdsi.ssi_signo =3D=3D 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 > >> --- > >> 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 > >> #include > >> #include > >> +#include > >> //#include > >> #include > >> #include > >> @@ -294,6 +295,67 @@ static bitmask_transtbl fcntl_flags_tbl[] =3D { > >> { 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; > >=20 > > It's more usual to just combine the struct definition with > > the typedef: > > typedef struct TargetFdTrans { > > ... > > } TargetFdTrans; >=20 > ok >=20 > >> +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; > >=20 > > I think if you have to split a ?: expression onto two lines it's > > a sign it would be clearer as an if (). >=20 > ok >=20 > >=20 > >> +} > >> + > >> +static void fd_trans_register(int fd, int flags, TargetFdTrans *tra= ns) > >> +{ > >> + if (fd >=3D target_fd_max) { > >> + target_fd_max =3D ((fd >> 6) + 1) << 6; /* by slice of 64 e= ntries */ > >> + target_fd_trans =3D g_realloc(target_fd_trans, > >> + target_fd_max * sizeof(TargetFd= Entry)); > >=20 > > g_realloc() doesn't zero the extra allocated memory, so you need > > to do it manually here. >=20 > ok >=20 > >> + } > >> + target_fd_trans[fd].flags =3D flags; > >> + target_fd_trans[fd].trans =3D trans; > >> +} > >> + > >> +static void fd_trans_unregister(int fd) > >> +{ > >> + if (fd < target_fd_max) { > >> + target_fd_trans[fd].trans =3D NULL; > >> + target_fd_trans[fd].flags =3D 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].tran= s); > >> + } > >> +} > >> + > >> +static void fd_trans_close_on_exec(void) > >> +{ > >> + int fd; > >> + > >> + for (fd =3D 0; fd < target_fd_max; fd++) { > >> + if (target_fd_trans[fd].flags & O_CLOEXEC) { > >> + fd_trans_unregister(fd); > >> + } > >> + } > >> +} > >=20 > > I think this one's going to turn out to be unneeded -- see > > comment later on. > >=20 > >> + > >> static int sys_getcwd1(char *buf, size_t size) > >> { > >> if (getcwd(buf, size) =3D=3D NULL) { > >> @@ -5246,6 +5308,78 @@ static int do_futex(target_ulong uaddr, int o= p, 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 =3D host_to_target_signal(info->ssi_signo); > >> + tinfo->ssi_signo =3D tswap32(sig); > >> + tinfo->ssi_errno =3D tswap32(tinfo->ssi_errno); > >> + tinfo->ssi_code =3D tswap32(info->ssi_code); > >> + tinfo->ssi_pid =3D tswap32(info->ssi_pid); > >> + tinfo->ssi_uid =3D tswap32(info->ssi_uid); > >> + tinfo->ssi_fd =3D tswap32(info->ssi_fd); > >> + tinfo->ssi_tid =3D tswap32(info->ssi_tid); > >> + tinfo->ssi_band =3D tswap32(info->ssi_band); > >> + tinfo->ssi_overrun =3D tswap32(info->ssi_overrun); > >> + tinfo->ssi_trapno =3D tswap32(info->ssi_trapno); > >> + tinfo->ssi_status =3D tswap32(info->ssi_status); > >> + tinfo->ssi_int =3D tswap32(info->ssi_int); > >> + tinfo->ssi_ptr =3D tswap64(info->ssi_ptr); > >> + tinfo->ssi_utime =3D tswap64(info->ssi_utime); > >> + tinfo->ssi_stime =3D tswap64(info->ssi_stime); > >> + tinfo->ssi_addr =3D tswap64(info->ssi_addr); > >=20 > > Some of these lines have a stray extra space after the '=3D'. > >=20 > > I said in review on v1 that you were missing > > tinfo->ssi_addr_lsb =3D tswap16(info->ssi_addr_lsb); > > and it's still not here. > >=20 > > 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. >=20 > My fedora 22 (2015) doesn't have this field in > /usr/include/sys/signalfd.h, but it is in /usr/include/linux/signalfd.h= . =20 > 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=20 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 =3D 0; i < len; i +=3D sizeof(struct signalfd_siginfo)) = { > >> + host_to_target_signalfd_siginfo(buf + i, buf + i); > >> + } > >> + > >> + return len; > >> +} > >> + > >> +static TargetFdTrans target_signalfd_trans =3D { > >> + .host_to_target =3D host_to_target_signalfd, > >> +}; > >> + > >> +static abi_long do_signalfd4(int fd, abi_long mask, int flags) > >> +{ > >> + int host_flags =3D flags & (~(TARGET_O_NONBLOCK | TARGET_O_CLOE= XEC)); > >=20 > > 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: > >=20 > > if (flags & ~(TARGET_O_NONBLOCK | TARGET_CLOEXEC)) { > > return -TARGET_EINVAL; > > } > > host_flags =3D target_to_host_bitmask(flags, fcntl_flags_tbl); > >=20 > >> + 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 |=3D O_NONBLOCK; > >> + } > >> + if (flags & TARGET_O_CLOEXEC) { > >> + host_flags |=3D O_CLOEXEC; > >> + } > >> + > >> + ret =3D get_errno(signalfd(fd, &host_mask, host_flags)); > >> + if (ret >=3D 0) { > >> + fd_trans_register(ret, host_flags, &target_signalfd_trans); > >> + } > >> + > >> + unlock_user_struct(target_mask, mask, 0); > >> + > >> + return ret; > >> +} > >> +#endif > >=20 > >> @@ -5830,6 +5978,9 @@ abi_long do_syscall(void *cpu_env, int num, ab= i_long arg1, > >> break; > >> unlock_user(*q, addr, 0); > >> } > >> + if (ret >=3D 0) { > >> + fd_trans_close_on_exec(); > >> + } > >=20 > > This is execve, right? We can't possibly get here if exec succeeded..= . >=20 > You're right... >=20 > >=20 > >> } > >> break; > >=20 > > thanks >=20 > Thank you for the review... >=20 > > -- PMM > >=20 >=20