| Message ID | 1295812563-7163-1-git-send-email-vapier@gentoo.org |
|---|---|
| State | New |
| Headers | show |
On 23 January 2011 19:56, Mike Frysinger <vapier@gentoo.org> wrote: > Some architectures (like Blackfin) only implement ppoll (and skip poll). > So add support for it using existing poll code. This looks wrong -- ppoll() is supposed to be atomic, but your implementation isn't. Why can't we just implement this by calling the host ppoll? (might need a configure test, but that's straightforward.) -- PMM
On Sun, Jan 23, 2011 at 16:25, Peter Maydell wrote: > On 23 January 2011 19:56, Mike Frysinger wrote: >> Some architectures (like Blackfin) only implement ppoll (and skip poll). >> So add support for it using existing poll code. > > This looks wrong -- ppoll() is supposed to be atomic, but > your implementation isn't. Why can't we just implement this > by calling the host ppoll? (might need a configure test, but > that's straightforward.) it's atomic from the apps' point of view, so what does it matter ? -mike
On 23 January 2011 21:35, Mike Frysinger <vapier@gentoo.org> wrote: > On Sun, Jan 23, 2011 at 16:25, Peter Maydell wrote: >> This looks wrong -- ppoll() is supposed to be atomic, but >> your implementation isn't. Why can't we just implement this >> by calling the host ppoll? (might need a configure test, but >> that's straightforward.) > > it's atomic from the apps' point of view, so what does it matter ? It's not atomic because signals can arrive in the gaps between your calls to sigaction and poll (even if qemu doesn't deliver them to the guest until we're done). ppoll() is supposed to return EINTR if interrupted by a signal, but if a signal arrives before you call poll() then you won't notice it so you won't return, and the app might hang. Looks like this same issue came up with a proposed pselect patch last year: http://www.mail-archive.com/qemu-devel@nongnu.org/msg28339.html (together with some remarks about it being better not to trust buggy libcs and go straight for the host syscall.) -- PMM
On Sun, Jan 23, 2011 at 17:35, Peter Maydell wrote: > On 23 January 2011 21:35, Mike Frysinger wrote: >> On Sun, Jan 23, 2011 at 16:25, Peter Maydell wrote: >>> This looks wrong -- ppoll() is supposed to be atomic, but >>> your implementation isn't. Why can't we just implement this >>> by calling the host ppoll? (might need a configure test, but >>> that's straightforward.) >> >> it's atomic from the apps' point of view, so what does it matter ? > > It's not atomic because signals can arrive in the gaps > between your calls to sigaction and poll (even if qemu doesn't > deliver them to the guest until we're done). ppoll() is supposed > to return EINTR if interrupted by a signal, but if a signal arrives > before you call poll() then you won't notice it so you won't > return, and the app might hang. i know how host signals work, but my limited understanding of how qemu works is that its guest signal handling would make this a non-issue. if that isnt the case, then i can trivially convert it to ppoll. > Looks like this same issue came up with a proposed pselect > patch last year: > http://www.mail-archive.com/qemu-devel@nongnu.org/msg28339.html > (together with some remarks about it being better not to > trust buggy libcs and go straight for the host syscall.) i also need pselect, and i have a local patch to support it (the same way i've implemented ppoll). returning ENOSYS is unacceptable as it assumes the port in question can fall back to another select variant. but if the arch *only* supports pselect6, then there is nothing to fall back to if select/newselect are not supported by the arch. -mike
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 6116ab5..5382662 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -6230,18 +6230,42 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, ret = do_select(arg1, arg2, arg3, arg4, arg5); break; #endif -#ifdef TARGET_NR_poll +#if defined(TARGET_NR_poll) || defined(TARGET_NR_ppoll) +# ifdef TARGET_NR_poll case TARGET_NR_poll: +# endif +# ifdef TARGET_NR_ppoll + case TARGET_NR_ppoll: +# endif { struct target_pollfd *target_pfd; unsigned int nfds = arg2; int timeout = arg3; struct pollfd *pfd; unsigned int i; + sigset_t origmask; target_pfd = lock_user(VERIFY_WRITE, arg1, sizeof(struct target_pollfd) * nfds, 1); if (!target_pfd) goto efault; + + if (num == TARGET_NR_ppoll) { + sigset_t set; + abi_ulong mask; + + if (arg3) { + struct timespec timeout_ts; + target_to_host_timespec(&timeout_ts, arg3); + timeout = timeout_ts.tv_sec * 1000 + + timeout_ts.tv_nsec / 1000000; + } else + timeout = -1; + + mask = arg4; + target_to_host_old_sigset(&set, &mask); + sigprocmask(SIG_SETMASK, &set, &origmask); + } + pfd = alloca(sizeof(struct pollfd) * nfds); for(i = 0; i < nfds; i++) { pfd[i].fd = tswap32(target_pfd[i].fd); @@ -6256,6 +6280,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, - sizeof(struct pollfd)); } unlock_user(target_pfd, arg1, ret); + + if (num == TARGET_NR_ppoll) + sigprocmask(SIG_SETMASK, &origmask, NULL); } break; #endif
Some architectures (like Blackfin) only implement ppoll (and skip poll). So add support for it using existing poll code. Signed-off-by: Mike Frysinger <vapier@gentoo.org> --- linux-user/syscall.c | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-)