Date: Thu, 28 Sep 2006 01:27:48 +0400 From: Ruslan Ermilov <ru@freebsd.org> To: John Baldwin <jhb@freebsd.org> 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 ... Message-ID: <20060927212748.GA83490@rambler-co.ru> In-Reply-To: <200609271703.43643.jhb@freebsd.org> References: <200609271957.k8RJv25Z028902@repoman.freebsd.org> <200609271703.43643.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060927212748.GA83490>