From owner-cvs-all@FreeBSD.ORG Wed Sep 27 21:30:14 2006 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id EE91716A4A7; Wed, 27 Sep 2006 21:30:13 +0000 (UTC) (envelope-from ru@rambler-co.ru) Received: from relay0.rambler.ru (relay0.rambler.ru [81.19.66.187]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5287143DCB; Wed, 27 Sep 2006 21:28:52 +0000 (GMT) (envelope-from ru@rambler-co.ru) Received: from relay0.rambler.ru (localhost [127.0.0.1]) by relay0.rambler.ru (Postfix) with ESMTP id B829B618D; Thu, 28 Sep 2006 01:27:46 +0400 (MSD) Received: from edoofus.park.rambler.ru (unknown [81.19.65.108]) by relay0.rambler.ru (Postfix) with ESMTP id 7DC166131; Thu, 28 Sep 2006 01:27:46 +0400 (MSD) Received: (from ru@localhost) by edoofus.park.rambler.ru (8.13.8/8.13.8) id k8RLRmYx099959; Thu, 28 Sep 2006 01:27:48 +0400 (MSD) (envelope-from ru) Date: Thu, 28 Sep 2006 01:27:48 +0400 From: Ruslan Ermilov To: John Baldwin Message-ID: <20060927212748.GA83490@rambler-co.ru> References: <200609271957.k8RJv25Z028902@repoman.freebsd.org> <200609271703.43643.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="EeQfGwPcQSOJBaQU" Content-Disposition: inline In-Reply-To: <200609271703.43643.jhb@freebsd.org> User-Agent: Mutt/1.5.13 (2006-08-11) X-Virus-Scanned: No virus found Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/dev/atkbdc atkbd.c src/sys/dev/digi digi.c src/sys/dev/kbdmux kbdmux.c src/sys/dev/syscons scvidctl.c syscons.c src/sys/dev/uart uart_kbd_sun.c src/sys/dev/usb ukbd.c src/sys/dev/vkbd vkbd.c src/sys/fs/procfs procfs_ioctl.c ... X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Sep 2006 21:30:14 -0000 --EeQfGwPcQSOJBaQU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi John, On Wed, Sep 27, 2006 at 05:03:42PM -0400, John Baldwin wrote: > On Wednesday 27 September 2006 15:57, Ruslan Ermilov wrote: > > ru 2006-09-27 19:57:02 UTC > >=20 > > FreeBSD src repository > >=20 > > Modified files: > > sys/dev/atkbdc atkbd.c=20 > > sys/dev/digi digi.c=20 > > sys/dev/kbdmux kbdmux.c=20 > > sys/dev/syscons scvidctl.c syscons.c=20 > > sys/dev/uart uart_kbd_sun.c=20 > > sys/dev/usb ukbd.c=20 > > sys/dev/vkbd vkbd.c=20 > > sys/fs/procfs procfs_ioctl.c=20 > > sys/kern sys_generic.c tty_pts.c tty_pty.c=20 > > sys/modules/digi/digi Makefile=20 > > sys/modules/if_tap Makefile=20 > > sys/modules/kbdmux Makefile=20 > > sys/modules/procfs Makefile=20 > > sys/modules/ukbd Makefile=20 > > sys/modules/vkbd Makefile=20 > > sys/net if_tap.c if_tap.h=20 > > sys/pc98/cbus pckbd.c=20 > > sys/sys consio.h digiio.h ioccom.h kbio.h=20 > > pioctl.h ttycom.h=20 > > Log: > > Fix our ioctl(2) implementation when the argument is "int". New > > ioctls passing integer arguments should use the _IOWINT() macro. > > This fixes a lot of ioctl's not working on sparc64, most notable > > being keyboard/syscons ioctls. > > =20 > > Full ABI compatibility is provided, with the bonus of fixing the > > handling of old ioctls on sparc64. >=20 > Eh? You just changed ioctl values breaking ABI all over the place, e.g.= =20 > sys/pioctl.h. >=20 No, see also the sys/fs/procfs/procfs_ioctl.c part of a change. > The size field changed from 0 to sizeof(int) meaning different=20 > ioctl values and thus ABI breakage. >=20 No. Old ioctls are still supported, just not in headers. > Plus, what if you have: >=20 > struct foo { > int bar; > }; >=20 > #define FOOIO _IOW('y', 0, struct foo) >=20 > that's going to have the same issue isn't it? >=20 No. In this case, you're passing a pointer. Here's what happened when you called ioctl(fd, IOCTL, (int)1) on sparc64. "1" is passed in a register (64-bit). Due to the ioctl() prototype in the kernel, it sees 1 as "caddr_t data". This means that you have "0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1" copied on the stack location pointed to by uap->data. Later on, a conversion from "int" to "int *" is made, and a pointer to that location of memory is passed to a device's ioctl() handler. It then accesses it as *(int *)arg and gets 0. > This really just looks like a=20 > big hack and it doesn't help with ABI compat at all. :( >=20 It does. It has been tested on i386/amd64/sparc64, with both old and new userland. > I think instead the various ioctl handlers have to realize that for IOC_V= OID=20 > ioctls declared using _IO() data is a (caddr_t *), not an (int *) (the ua= p=20 > struct for ioctl clearly defines data as a caddr_t). Fix whatever crap y= ou=20 > have to in the kernel to deal with it, but don't change the userland ABI.= :( >=20 POSIX and our manpage both say that an argument should either be an "int" or a pointer. This means we need to support "int" arguments but do it properly. My first version of the patch fixed the kernel only, replacing "int" with intptr_t where a kernel initiated an ioctl(). This is very inconvenient for a driver writer, when a userland passes an "int" argument, and a kernel has to pass an "intptr_t" argument, due to the bogus implementation. I think you were too quick in replying, and didn't look at the whole patch in detail. :-) Cheers, --=20 Ruslan Ermilov ru@FreeBSD.org FreeBSD committer --EeQfGwPcQSOJBaQU Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.5 (FreeBSD) iD8DBQFFGuzUqRfpzJluFF4RAsGRAKCD/T3vbZQ5k+dH/6Yt6UVh/vyJ4wCdExzc /Iq9FCi6fAIhVaNKUqk8Pm8= =fSbo -----END PGP SIGNATURE----- --EeQfGwPcQSOJBaQU--