From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLyWd-0006xi-Cu for qemu-devel@nongnu.org; Wed, 10 Oct 2012 11:48:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TLyWX-0001oF-2Q for qemu-devel@nongnu.org; Wed, 10 Oct 2012 11:48:47 -0400 Received: from mail-ie0-f173.google.com ([209.85.223.173]:64794) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLyWW-0001o2-Sq for qemu-devel@nongnu.org; Wed, 10 Oct 2012 11:48:40 -0400 Received: by mail-ie0-f173.google.com with SMTP id 17so1118608iea.4 for ; Wed, 10 Oct 2012 08:48:40 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1348935086-11336-2-git-send-email-abarcelo@ac.upc.edu> References: <1348935086-11336-1-git-send-email-abarcelo@ac.upc.edu> <1348935086-11336-2-git-send-email-abarcelo@ac.upc.edu> Date: Wed, 10 Oct 2012 16:48:40 +0100 Message-ID: From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 1/2] signal: added a wrapper for sigprocmask function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Barcelo Cc: Riku Voipio , qemu-devel@nongnu.org On 29 September 2012 17:11, Alex Barcelo wrote: Hi; thanks for this patch. > A transparent wrapper for sigprocmask function. As I mentioned in my reply to the cover letter, this needs a Signed-off-by: line. The commit message could also be a bit more verbose, for instance something like: Create a wrapper for signal mask changes initiated by the guest; this will give us a place to put code which prevents the guest from changing the handling of signals used by QEMU itself internally. (To some extent this is a personal style thing, but I tend to favour fairly verbose commentary and commit messages rather than terseness.) > > --- > linux-user/qemu.h | 1 + > linux-user/signal.c | 8 ++++++++ > linux-user/syscall.c | 20 ++++++++++---------- > 3 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/linux-user/qemu.h b/linux-user/qemu.h > index fc4cc00..e2dd6a6 100644 > --- a/linux-user/qemu.h > +++ b/linux-user/qemu.h > @@ -237,6 +237,7 @@ int host_to_target_signal(int sig); > long do_sigreturn(CPUArchState *env); > long do_rt_sigreturn(CPUArchState *env); > abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp); > +int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset); > > #ifdef TARGET_I386 > /* vm86.c */ > diff --git a/linux-user/signal.c b/linux-user/signal.c > index 7869147..b8b8268 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -5463,6 +5463,14 @@ long do_rt_sigreturn(CPUArchState *env) > > #endif > > +/* Wrapper for sigprocmask function > + * Emulates a sigprocmask in a safe way for the guest You might like to add: * Note that set and oldset are host signal sets, not guest ones. > + */ > +int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset) > +{ > + return sigprocmask(how, set, oldset); > +} > + > void process_pending_signals(CPUArchState *cpu_env) > { > int sig; > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 471d060..1117bbe 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -4260,7 +4260,7 @@ static void *clone_func(void *arg) > if (info->parent_tidptr) > put_user_u32(info->tid, info->parent_tidptr); > /* Enable signals. */ > - sigprocmask(SIG_SETMASK, &info->sigmask, NULL); > + do_sigprocmask(SIG_SETMASK, &info->sigmask, NULL); > /* Signal to the parent that we're ready. */ > pthread_mutex_lock(&info->mutex); > pthread_cond_broadcast(&info->cond); > @@ -4351,12 +4351,12 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp, > /* It is not safe to deliver signals until the child has finished > initializing, so temporarily block all signals. */ > sigfillset(&sigmask); > - sigprocmask(SIG_BLOCK, &sigmask, &info.sigmask); > + do_sigprocmask(SIG_BLOCK, &sigmask, &info.sigmask); > > ret = pthread_create(&info.thread, &attr, clone_func, &info); > /* TODO: Free new CPU state if thread creation failed. */ > > - sigprocmask(SIG_SETMASK, &info.sigmask, NULL); > + do_sigprocmask(SIG_SETMASK, &info.sigmask, NULL); > pthread_attr_destroy(&attr); > if (ret == 0) { > /* Wait for the child to initialize. */ The three sigprocmask() calls above are all QEMU itself manipulating its own signal mask (to block signals temporarily across a thread creation). They're not the guest asking for a signal mask change. So these three can all just use sigprocmask() and shouldn't go via do_sigprocmask() I think. > @@ -5875,7 +5875,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > { > sigset_t cur_set; > abi_ulong target_set; > - sigprocmask(0, NULL, &cur_set); > + do_sigprocmask(0, NULL, &cur_set); > host_to_target_old_sigset(&target_set, &cur_set); > ret = target_set; > } > @@ -5886,10 +5886,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > { > sigset_t set, oset, cur_set; > abi_ulong target_set = arg1; > - sigprocmask(0, NULL, &cur_set); > + do_sigprocmask(0, NULL, &cur_set); > target_to_host_old_sigset(&set, &target_set); > sigorset(&set, &set, &cur_set); > - sigprocmask(SIG_SETMASK, &set, &oset); > + do_sigprocmask(SIG_SETMASK, &set, &oset); > host_to_target_old_sigset(&target_set, &oset); > ret = target_set; > } > @@ -5920,7 +5920,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > mask = arg2; > target_to_host_old_sigset(&set, &mask); > > - ret = get_errno(sigprocmask(how, &set, &oldset)); > + ret = get_errno(do_sigprocmask(how, &set, &oldset)); > if (!is_error(ret)) { > host_to_target_old_sigset(&mask, &oldset); > ret = mask; > @@ -5954,7 +5954,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > how = 0; > set_ptr = NULL; > } > - ret = get_errno(sigprocmask(how, set_ptr, &oldset)); > + ret = get_errno(do_sigprocmask(how, set_ptr, &oldset)); > if (!is_error(ret) && arg3) { > if (!(p = lock_user(VERIFY_WRITE, arg3, sizeof(target_sigset_t), 0))) > goto efault; > @@ -5994,7 +5994,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > how = 0; > set_ptr = NULL; > } > - ret = get_errno(sigprocmask(how, set_ptr, &oldset)); > + ret = get_errno(do_sigprocmask(how, set_ptr, &oldset)); > if (!is_error(ret) && arg3) { > if (!(p = lock_user(VERIFY_WRITE, arg3, sizeof(target_sigset_t), 0))) > goto efault; > @@ -7870,7 +7870,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > } > mask = arg2; > target_to_host_old_sigset(&set, &mask); > - sigprocmask(how, &set, &oldset); > + do_sigprocmask(how, &set, &oldset); > host_to_target_old_sigset(&mask, &oldset); > ret = mask; > } I think all the uses of sigprocmask() in linux-user/signal.c also need to be do_sigprocmask(), as they are the guest trying to control its signal mask (via the mask it specifies for running signal handlers, or the mask it passes back when restoring context on return from a signal handler). -- PMM