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 > > > > FreeBSD src repository > > > > Modified files: > > sys/dev/atkbdc atkbd.c > > sys/dev/digi digi.c > > sys/dev/kbdmux kbdmux.c > > sys/dev/syscons scvidctl.c syscons.c > > sys/dev/uart uart_kbd_sun.c > > sys/dev/usb ukbd.c > > sys/dev/vkbd vkbd.c > > sys/fs/procfs procfs_ioctl.c > > sys/kern sys_generic.c tty_pts.c tty_pty.c > > sys/modules/digi/digi Makefile > > sys/modules/if_tap Makefile > > sys/modules/kbdmux Makefile > > sys/modules/procfs Makefile > > sys/modules/ukbd Makefile > > sys/modules/vkbd Makefile > > sys/net if_tap.c if_tap.h > > sys/pc98/cbus pckbd.c > > sys/sys consio.h digiio.h ioccom.h kbio.h > > pioctl.h ttycom.h > > 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. > > > > Full ABI compatibility is provided, with the bonus of fixing the > > handling of old ioctls on sparc64. > > Eh? You just changed ioctl values breaking ABI all over the place, e.g. > sys/pioctl.h. > 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
> ioctl values and thus ABI breakage.
>
No. Old ioctls are still supported, just not in headers.
> Plus, what if you have:
>
> struct foo {
> int bar;
> };
>
> #define FOOIO _IOW('y', 0, struct foo)
>
> that's going to have the same issue isn't it?
>
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
> big hack and it doesn't help with ABI compat at all. :(
>
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_VOID
> ioctls declared using _IO() data is a (caddr_t *), not an (int *) (the uap
> struct for ioctl clearly defines data as a caddr_t). Fix whatever crap you
> have to in the kernel to deal with it, but don't change the userland ABI. :(
>
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,
--
Ruslan Ermilov
[EMAIL PROTECTED]
FreeBSD committer
pgpeKYGcD7roh.pgp
Description: PGP signature
