From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=39295 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PhAPK-0007c0-JG
	for qemu-devel@nongnu.org; Sun, 23 Jan 2011 19:35:47 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1PhAPJ-0001nT-AG
	for qemu-devel@nongnu.org; Sun, 23 Jan 2011 19:35:46 -0500
Received: from mail-fx0-f45.google.com ([209.85.161.45]:63860)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1PhAPJ-0001nO-1y
	for qemu-devel@nongnu.org; Sun, 23 Jan 2011 19:35:45 -0500
Received: by fxm12 with SMTP id 12so3699299fxm.4
	for <qemu-devel@nongnu.org>; Sun, 23 Jan 2011 16:35:44 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <1295825542-12550-1-git-send-email-vapier@gentoo.org>
References: <1295812563-7163-1-git-send-email-vapier@gentoo.org>
	<1295825542-12550-1-git-send-email-vapier@gentoo.org>
Date: Mon, 24 Jan 2011 00:35:43 +0000
Message-ID: <AANLkTimSyeM+Qn8yzG-MqwADAX7=59SceyPgyOAG8NkF@mail.gmail.com>
Subject: Re: [Qemu-devel] [PATCH v2] linux-user: add ppoll syscall support
From: Peter Maydell <peter.maydell@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Mike Frysinger <vapier@gentoo.org>
Cc: Riku Voipio <riku.voipio@iki.fi>, qemu-devel@nongnu.org

On 23 January 2011 23:32, Mike Frysinger <vapier@gentoo.org> wrote:
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (arg3)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ta=
rget_to_host_timespec(timeout_ts, arg3);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ti=
meout_ts =3D NULL;

Coding style mandates braces here. Also, target_to_host_timespec()
can return non-zero if the user handed us a bad pointer, which
you need to handle. (No, none of the other users of the
function do this; yes, I think they're all broken :-))

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0target_to_host_o=
ld_sigset(&sigmask, &mask);

Are you sure this is right?
http://lxr.linux.no/#linux+v2.6.37/fs/select.c#L950
suggests the syscall takes a new sigset, not an old one.

You also need to lock_user() the memory for the sigset.
(target_to_host_timespec() does lock_user/unlock_user for
you but the target_to_host_*sigset() don't).

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D get_errn=
o(ppoll(pfd, nfds, timeout_ts, &sigmask));
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else
> +# endif
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D get_errn=
o(poll(pfd, nfds, timeout));
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!is_error(ret)) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for(i =3D 0; i < =
nfds; i++) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tar=
get_pfd[i].revents =3D tswap16(pfd[i].revents);

The ppoll() manpage says
"The Linux ppoll() system call modifies its timeout argument."

Your implementation doesn't do this. Also, this means you
really do need to call the host ppoll syscall directly, because
glibc deliberately hides this behaviour and would prevent
us from implementing it.

thanks
-- PMM