* [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
@ 2009-09-28 13:53 David Howells
2009-09-28 14:16 ` Alan Cox
2009-09-28 14:55 ` David Howells
0 siblings, 2 replies; 12+ messages in thread
From: David Howells @ 2009-09-28 13:53 UTC (permalink / raw
To: torvalds, akpm, gregkh; +Cc: alan, linux-kernel, David Howells
Remove pty_ops_bsd as nothing appears to use it anymore. It results in the
following warning:
drivers/char/pty.c:344: warning: unused variable `pty_ops_bsd'
if CONFIG_LEGACY_PTYS=y.
Also remove pty_bsd_ioctl() as that's only referred to by pty_ops_bsd.
Possibly legacy_pty_init() should be passing this to tty_set_operations()
rather than pty_ops.
Signed-off-by: David Howells <dhowells@redhat.com>
---
drivers/char/pty.c | 23 -----------------------
1 files changed, 0 insertions(+), 23 deletions(-)
diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index 53761ce..175f805 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -328,32 +328,9 @@ static const struct tty_operations pty_ops = {
#ifdef CONFIG_LEGACY_PTYS
static struct tty_driver *pty_driver, *pty_slave_driver;
-static int pty_bsd_ioctl(struct tty_struct *tty, struct file *file,
- unsigned int cmd, unsigned long arg)
-{
- switch (cmd) {
- case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */
- return pty_set_lock(tty, (int __user *) arg);
- }
- return -ENOIOCTLCMD;
-}
-
static int legacy_count = CONFIG_LEGACY_PTY_COUNT;
module_param(legacy_count, int, 0);
-static const struct tty_operations pty_ops_bsd = {
- .open = pty_open,
- .close = pty_close,
- .write = pty_write,
- .write_room = pty_write_room,
- .flush_buffer = pty_flush_buffer,
- .chars_in_buffer = pty_chars_in_buffer,
- .unthrottle = pty_unthrottle,
- .set_termios = pty_set_termios,
- .ioctl = pty_bsd_ioctl,
- .resize = pty_resize
-};
-
static void __init legacy_pty_init(void)
{
if (legacy_count <= 0)
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-28 13:53 [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used David Howells
@ 2009-09-28 14:16 ` Alan Cox
2009-09-28 14:55 ` Linus Torvalds
2009-09-29 15:55 ` David Howells
2009-09-28 14:55 ` David Howells
1 sibling, 2 replies; 12+ messages in thread
From: Alan Cox @ 2009-09-28 14:16 UTC (permalink / raw
To: David Howells; +Cc: torvalds, akpm, gregkh, linux-kernel, David Howells
On Mon, 28 Sep 2009 14:53:32 +0100
David Howells <dhowells@redhat.com> wrote:
> Remove pty_ops_bsd as nothing appears to use it anymore. It results in the
> following warning:
>
> drivers/char/pty.c:344: warning: unused variable `pty_ops_bsd'
>
> if CONFIG_LEGACY_PTYS=y.
>
> Also remove pty_bsd_ioctl() as that's only referred to by pty_ops_bsd.
>
> Possibly legacy_pty_init() should be passing this to tty_set_operations()
> rather than pty_ops.
It should indeed, otherwise the BSD pty locking ioctl fails.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-28 13:53 [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used David Howells
2009-09-28 14:16 ` Alan Cox
@ 2009-09-28 14:55 ` David Howells
2009-09-28 15:25 ` Alan Cox
1 sibling, 1 reply; 12+ messages in thread
From: David Howells @ 2009-09-28 14:55 UTC (permalink / raw
To: Alan Cox; +Cc: dhowells, torvalds, akpm, gregkh, linux-kernel
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > Possibly legacy_pty_init() should be passing this to tty_set_operations()
> > rather than pty_ops.
>
> It should indeed, otherwise the BSD pty locking ioctl fails.
In that case, what should be using pty_ops? If CONFIG_LEGACY_PTYS=n, then I
see:
drivers/char/pty.c:314: warning: unused variable `pty_ops'
David
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-28 14:16 ` Alan Cox
@ 2009-09-28 14:55 ` Linus Torvalds
2009-09-29 15:55 ` David Howells
1 sibling, 0 replies; 12+ messages in thread
From: Linus Torvalds @ 2009-09-28 14:55 UTC (permalink / raw
To: Alan Cox; +Cc: David Howells, akpm, gregkh, linux-kernel
On Mon, 28 Sep 2009, Alan Cox wrote:
> On Mon, 28 Sep 2009 14:53:32 +0100
> David Howells <dhowells@redhat.com> wrote:
> >
> > Possibly legacy_pty_init() should be passing this to tty_set_operations()
> > rather than pty_ops.
>
> It should indeed, otherwise the BSD pty locking ioctl fails.
Hmm. The pty_ops_bsd thing is missing the .install handler. And looking at
it, I'm not immediately seeing the difference between the pty_install
handler and the pty_unix98_install one. There's some differences in
termios initialization, and there is that
driver->other->ttys[idx] = o_tty;
driver->ttys[idx] = tty
thing, but it's not entirely clear to me why the legacy pty's don't use
the 'lookup()' logic instead (like the unix98 ones). Oh well.
Does anybody want to test this patch?
And is there any situation where this can be actually noticed? You
obviously need to have an old distro that actually uses the old static pty
setup.
Linus
---
drivers/char/pty.c | 24 ++++++------------------
1 files changed, 6 insertions(+), 18 deletions(-)
diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index 53761ce..7f93ca2 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -261,6 +261,9 @@ done:
return 0;
}
+/* Traditional BSD devices */
+#ifdef CONFIG_LEGACY_PTYS
+
static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
{
struct tty_struct *o_tty;
@@ -310,22 +313,6 @@ free_mem_out:
return -ENOMEM;
}
-
-static const struct tty_operations pty_ops = {
- .install = pty_install,
- .open = pty_open,
- .close = pty_close,
- .write = pty_write,
- .write_room = pty_write_room,
- .flush_buffer = pty_flush_buffer,
- .chars_in_buffer = pty_chars_in_buffer,
- .unthrottle = pty_unthrottle,
- .set_termios = pty_set_termios,
- .resize = pty_resize
-};
-
-/* Traditional BSD devices */
-#ifdef CONFIG_LEGACY_PTYS
static struct tty_driver *pty_driver, *pty_slave_driver;
static int pty_bsd_ioctl(struct tty_struct *tty, struct file *file,
@@ -342,6 +329,7 @@ static int legacy_count = CONFIG_LEGACY_PTY_COUNT;
module_param(legacy_count, int, 0);
static const struct tty_operations pty_ops_bsd = {
+ .install = pty_install,
.open = pty_open,
.close = pty_close,
.write = pty_write,
@@ -383,7 +371,7 @@ static void __init legacy_pty_init(void)
pty_driver->init_termios.c_ospeed = 38400;
pty_driver->flags = TTY_DRIVER_RESET_TERMIOS | TTY_DRIVER_REAL_RAW;
pty_driver->other = pty_slave_driver;
- tty_set_operations(pty_driver, &pty_ops);
+ tty_set_operations(pty_driver, &pty_ops_bsd);
pty_slave_driver->owner = THIS_MODULE;
pty_slave_driver->driver_name = "pty_slave";
@@ -399,7 +387,7 @@ static void __init legacy_pty_init(void)
pty_slave_driver->flags = TTY_DRIVER_RESET_TERMIOS |
TTY_DRIVER_REAL_RAW;
pty_slave_driver->other = pty_driver;
- tty_set_operations(pty_slave_driver, &pty_ops);
+ tty_set_operations(pty_slave_driver, &pty_ops_bsd);
if (tty_register_driver(pty_driver))
panic("Couldn't register pty driver");
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-28 14:55 ` David Howells
@ 2009-09-28 15:25 ` Alan Cox
0 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2009-09-28 15:25 UTC (permalink / raw
To: David Howells; +Cc: dhowells, torvalds, akpm, gregkh, linux-kernel
On Mon, 28 Sep 2009 15:55:44 +0100
David Howells <dhowells@redhat.com> wrote:
> Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> > > Possibly legacy_pty_init() should be passing this to tty_set_operations()
> > > rather than pty_ops.
> >
> > It should indeed, otherwise the BSD pty locking ioctl fails.
>
> In that case, what should be using pty_ops? If CONFIG_LEGACY_PTYS=n, then I
> see:
>
> drivers/char/pty.c:314: warning: unused variable `pty_ops'
You'd have to go and check the old code but I believe one side of the BSD
pair uses pty_ops the other pty_bsd ops, and unix98 uses different ops
again.
All it does is make sure the right ioctls appear on the right devices.
Quite why the can't all use one and a simple if in the ioctl handler I'm
not sure
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-28 14:16 ` Alan Cox
2009-09-28 14:55 ` Linus Torvalds
@ 2009-09-29 15:55 ` David Howells
2009-09-29 16:23 ` Linus Torvalds
2009-09-30 10:20 ` David Howells
1 sibling, 2 replies; 12+ messages in thread
From: David Howells @ 2009-09-29 15:55 UTC (permalink / raw
To: Linus Torvalds; +Cc: dhowells, Alan Cox, akpm, gregkh, linux-kernel
Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> pty_slave_driver->owner = THIS_MODULE;
> pty_slave_driver->driver_name = "pty_slave";
> @@ -399,7 +387,7 @@ static void __init legacy_pty_init(void)
> pty_slave_driver->flags = TTY_DRIVER_RESET_TERMIOS |
> TTY_DRIVER_REAL_RAW;
> pty_slave_driver->other = pty_driver;
> - tty_set_operations(pty_slave_driver, &pty_ops);
> + tty_set_operations(pty_slave_driver, &pty_ops_bsd);
>
> if (tty_register_driver(pty_driver))
> panic("Couldn't register pty driver");
>
Is it right to use pty_ops_bsd in _both_ places? Looking at the code in
linux-2.6.0, the BSD ioctl only applies to the master and doesn't apply to the
slave.
David
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-29 15:55 ` David Howells
@ 2009-09-29 16:23 ` Linus Torvalds
2009-09-29 16:38 ` Linus Torvalds
2009-09-29 18:40 ` David Howells
2009-09-30 10:20 ` David Howells
1 sibling, 2 replies; 12+ messages in thread
From: Linus Torvalds @ 2009-09-29 16:23 UTC (permalink / raw
To: David Howells; +Cc: Alan Cox, akpm, gregkh, linux-kernel
On Tue, 29 Sep 2009, David Howells wrote:
>
> Is it right to use pty_ops_bsd in _both_ places? Looking at the code in
> linux-2.6.0, the BSD ioctl only applies to the master and doesn't apply to the
> slave.
Right you are, good catch.
That said, I have to say that this whole pty lock thing seems to have been
broken since 2.6.26, and even now, nobody actually _complained_. You found
the problem due to a compiler warning rather than due to somebody noticing
that pty_bsd_ioctl() is no longer hooked up.
Because as far as I can tell, the bug was introduced by commit
3e8e88ca053150efdbecb45d8f481cf560ec808d ("pty: prepare for tty->ops
changes") back in April of 2008. That added the whole 'pty_ops_bsd'
structure, but it has never actually been used.
So I do wonder whether the right thing to do would not be to simple remove
the whole pty_ops_bsd code entirely. Or maybe mode the trivial TIOCSPTLCK
into the generic tty ioctl handling, and just make it test for
"driver->subtype == PTY_TYPE_MASTER" - and at least get rid of this subtle
thing that was broken for over a year without anybody noticing..
Linus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-29 16:23 ` Linus Torvalds
@ 2009-09-29 16:38 ` Linus Torvalds
2009-09-29 18:40 ` David Howells
1 sibling, 0 replies; 12+ messages in thread
From: Linus Torvalds @ 2009-09-29 16:38 UTC (permalink / raw
To: David Howells; +Cc: Alan Cox, akpm, gregkh, linux-kernel
On Tue, 29 Sep 2009, Linus Torvalds wrote:
>
>
> On Tue, 29 Sep 2009, David Howells wrote:
> >
> > Is it right to use pty_ops_bsd in _both_ places? Looking at the code in
> > linux-2.6.0, the BSD ioctl only applies to the master and doesn't apply to the
> > slave.
>
> Right you are, good catch.
Anyway, here's an updated patch to just fix this part. Whether we need to
have that crazy locking at all, considering that it was broken for 18
months, is still an interesting question. But this patch just tries to fix
the long-time bug.
Comments?
Linus
---
drivers/char/pty.c | 43 ++++++++++++++++++++++++-------------------
1 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index 53761ce..dcbbf57 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -261,6 +261,9 @@ done:
return 0;
}
+/* Traditional BSD devices */
+#ifdef CONFIG_LEGACY_PTYS
+
static int pty_install(struct tty_driver *driver, struct tty_struct *tty)
{
struct tty_struct *o_tty;
@@ -310,22 +313,6 @@ free_mem_out:
return -ENOMEM;
}
-
-static const struct tty_operations pty_ops = {
- .install = pty_install,
- .open = pty_open,
- .close = pty_close,
- .write = pty_write,
- .write_room = pty_write_room,
- .flush_buffer = pty_flush_buffer,
- .chars_in_buffer = pty_chars_in_buffer,
- .unthrottle = pty_unthrottle,
- .set_termios = pty_set_termios,
- .resize = pty_resize
-};
-
-/* Traditional BSD devices */
-#ifdef CONFIG_LEGACY_PTYS
static struct tty_driver *pty_driver, *pty_slave_driver;
static int pty_bsd_ioctl(struct tty_struct *tty, struct file *file,
@@ -341,7 +328,12 @@ static int pty_bsd_ioctl(struct tty_struct *tty, struct file *file,
static int legacy_count = CONFIG_LEGACY_PTY_COUNT;
module_param(legacy_count, int, 0);
-static const struct tty_operations pty_ops_bsd = {
+/*
+ * The master side of a pty can do TIOCSPTLCK and thus
+ * has pty_bsd_ioctl.
+ */
+static const struct tty_operations master_pty_ops_bsd = {
+ .install = pty_install,
.open = pty_open,
.close = pty_close,
.write = pty_write,
@@ -354,6 +346,19 @@ static const struct tty_operations pty_ops_bsd = {
.resize = pty_resize
};
+static const struct tty_operations slave_pty_ops_bsd = {
+ .install = pty_install,
+ .open = pty_open,
+ .close = pty_close,
+ .write = pty_write,
+ .write_room = pty_write_room,
+ .flush_buffer = pty_flush_buffer,
+ .chars_in_buffer = pty_chars_in_buffer,
+ .unthrottle = pty_unthrottle,
+ .set_termios = pty_set_termios,
+ .resize = pty_resize
+};
+
static void __init legacy_pty_init(void)
{
if (legacy_count <= 0)
@@ -383,7 +388,7 @@ static void __init legacy_pty_init(void)
pty_driver->init_termios.c_ospeed = 38400;
pty_driver->flags = TTY_DRIVER_RESET_TERMIOS | TTY_DRIVER_REAL_RAW;
pty_driver->other = pty_slave_driver;
- tty_set_operations(pty_driver, &pty_ops);
+ tty_set_operations(pty_driver, &master_pty_ops_bsd);
pty_slave_driver->owner = THIS_MODULE;
pty_slave_driver->driver_name = "pty_slave";
@@ -399,7 +404,7 @@ static void __init legacy_pty_init(void)
pty_slave_driver->flags = TTY_DRIVER_RESET_TERMIOS |
TTY_DRIVER_REAL_RAW;
pty_slave_driver->other = pty_driver;
- tty_set_operations(pty_slave_driver, &pty_ops);
+ tty_set_operations(pty_slave_driver, &slave_pty_ops_bsd);
if (tty_register_driver(pty_driver))
panic("Couldn't register pty driver");
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-29 16:23 ` Linus Torvalds
2009-09-29 16:38 ` Linus Torvalds
@ 2009-09-29 18:40 ` David Howells
2009-09-29 22:39 ` Alan Cox
1 sibling, 1 reply; 12+ messages in thread
From: David Howells @ 2009-09-29 18:40 UTC (permalink / raw
To: Linus Torvalds; +Cc: dhowells, Alan Cox, akpm, gregkh, linux-kernel
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> Comments?
Perhaps add legacy pty support to the list of features to be removed and see
who howls.
> static struct tty_driver *pty_driver, *pty_slave_driver;
These shouldn't be global variables. They're only used within
legacy_pty_init(). That's a side issue, however.
Apart from that, it looks reasonable.
Acked-by: David Howells <dhowells@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-29 18:40 ` David Howells
@ 2009-09-29 22:39 ` Alan Cox
0 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2009-09-29 22:39 UTC (permalink / raw
To: David Howells; +Cc: Linus Torvalds, dhowells, akpm, gregkh, linux-kernel
On Tue, 29 Sep 2009 19:40:10 +0100
David Howells <dhowells@redhat.com> wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > Comments?
>
> Perhaps add legacy pty support to the list of features to be removed and see
> who howls.
You do break stuff still and they are part of the API, ABI and namespace
so you break compatibility.
> > static struct tty_driver *pty_driver, *pty_slave_driver;
>
> These shouldn't be global variables. They're only used within
> legacy_pty_init(). That's a side issue, however.
Agreed
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-29 15:55 ` David Howells
2009-09-29 16:23 ` Linus Torvalds
@ 2009-09-30 10:20 ` David Howells
2009-09-30 11:28 ` Alan Cox
1 sibling, 1 reply; 12+ messages in thread
From: David Howells @ 2009-09-30 10:20 UTC (permalink / raw
To: Linus Torvalds, Alan Cox; +Cc: dhowells, akpm, gregkh, linux-kernel
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> So I do wonder whether the right thing to do would not be to simple remove
> the whole pty_ops_bsd code entirely. Or maybe mode the trivial TIOCSPTLCK
> into the generic tty ioctl handling, and just make it test for
> "driver->subtype == PTY_TYPE_MASTER" - and at least get rid of this subtle
> thing that was broken for over a year without anybody noticing..
Also, does it matter if someone issues TIOCGPTN on a legacy pty master? There
is a number in tty->index that can be returned. If it can, then
pty_bsd_ioctl() can be merged with pty_unix98_ioctl(), and possibly both
ioctls can be given to the generic tty ioctl handling with the condition that
driver->subtype == PTY_TYPE_MASTER.
David
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used
2009-09-30 10:20 ` David Howells
@ 2009-09-30 11:28 ` Alan Cox
0 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2009-09-30 11:28 UTC (permalink / raw
To: David Howells; +Cc: Linus Torvalds, dhowells, akpm, gregkh, linux-kernel
O> Also, does it matter if someone issues TIOCGPTN on a legacy pty master? There
> is a number in tty->index that can be returned. If it can, then
Not a lot - providing someone isn't using it as a way to tell pty types
apart (which I have seen done in example code)
> pty_bsd_ioctl() can be merged with pty_unix98_ioctl(), and possibly both
> ioctls can be given to the generic tty ioctl handling
I would keep
if (type == FOO)
as far away from the generic tty ioctl code as possible. It's already
convoluted and complex with zillions of ioctl calls. Separating out lots
of the console and other ioctls properly cleaned it up no end and going
backwards would be a bad mistake.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2009-09-30 11:27 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-28 13:53 [PATCH] Remove pty_ops_bsd and pty_bsd_ioctl() as they're not used David Howells
2009-09-28 14:16 ` Alan Cox
2009-09-28 14:55 ` Linus Torvalds
2009-09-29 15:55 ` David Howells
2009-09-29 16:23 ` Linus Torvalds
2009-09-29 16:38 ` Linus Torvalds
2009-09-29 18:40 ` David Howells
2009-09-29 22:39 ` Alan Cox
2009-09-30 10:20 ` David Howells
2009-09-30 11:28 ` Alan Cox
2009-09-28 14:55 ` David Howells
2009-09-28 15:25 ` Alan Cox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).