2009-06-15 15:39:44

by Vegard Nossum

Subject: [PATCH] tty_ldisc: propagate errors from tty_ldisc_release

>From ea6ee429b1f173e6905c3e6251649a6caf93d533 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <[email protected]>
Date: Mon, 15 Jun 2009 17:26:35 +0200
Subject: [PATCH] tty_ldisc: propagate errors from tty_ldisc_release

This is probably better than just ignoring the errors.

Signed-off-by: Vegard Nossum <[email protected]>
---
drivers/char/tty_ldisc.c | 26 ++++++++++++++++++++------
include/linux/tty.h | 2 +-
2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index 39c8f86..1d39e2b 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -840,17 +840,24 @@ int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty)
* Called during the final close of a tty/pty pair in order to shut down
* the line discpline layer. On exit the ldisc assigned is N_TTY and the
* ldisc has not been opened.
+ *
+ * Returns 0 on success, negative error code otherwise.
*/

-void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
+int tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
{
+ int err;
+
/*
* Prevent flush_to_ldisc() from rescheduling the work for later. Then
* kill any delayed work. As this is the final close it does not
* race with the set_ldisc code path.
*/

- tty_ldisc_halt(tty);
+ err = tty_ldisc_halt(tty);
+ if (err < 0)
+ return err;
+
flush_scheduled_work();

/*
@@ -859,7 +866,16 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
* side is zero.
*/

- tty_ldisc_wait_idle(tty);
+ err = tty_ldisc_wait_idle(tty);
+ if (err < 0)
+ return err;
+
+ /* This will need doing differently if we need to lock */
+ if (o_tty) {
+ err = tty_ldisc_release(o_tty, NULL);
+ if (err < 0)
+ return err;
+ }

/*
* Shutdown the current line discipline, and reset it to N_TTY.
@@ -868,9 +884,7 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
*/

tty_ldisc_reinit(tty);
- /* This will need doing differently if we need to lock */
- if (o_tty)
- tty_ldisc_release(o_tty, NULL);
+ return 0;
}

/**
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1488d8c..474abd6 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -461,7 +461,7 @@ extern int tty_register_ldisc(int disc, struct tty_ldisc_ops *new_ldisc);
extern int tty_unregister_ldisc(int disc);
extern int tty_set_ldisc(struct tty_struct *tty, int ldisc);
extern int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty);
-extern void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty);
+extern int tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty);
extern void tty_ldisc_init(struct tty_struct *tty);
extern void tty_ldisc_begin(void);
/* This last one is just for the tty layer internals and shouldn't be used elsewhere */
--
1.6.0.6

2009-06-15 15:50:21

by Alan

Subject: Re: [PATCH] tty_ldisc: propagate errors from tty_ldisc_release

> Subject: [PATCH] tty_ldisc: propagate errors from tty_ldisc_release
>
> This is probably better than just ignoring the errors.

tty_ldisc_halt doesn't return an error as such - it just reports whether
the work could be cancelled. Nor can we exit on error paths in those
situations.

Basically one or two cases need to know if we cancelled the work queue or
not (so they can restart it).

So NAK the patch