patch to add error checking in term

[Marcus Brinkmann]

Hi,

this patch changes the interfaces of the bottom handler to return error
values.  To keep it simple, for now every old function is changed to return 0.
The actual error checking in devio.c for example can then be added later.
This is also desirable because I am not sure if term can really cope with
these functions returning errors in all places, so I want to keep it
incremental (the hurdio bottom handler will return errors, and then we can
test it there first, before we go back and add error checking to the device
bottom handler).

However, some places in term do not check yet the errors coming from the bottom
handler (even after applying the patch).  Here is a list, please comment if
you can.

In general, some problems stem from the fact that term does some global
diddling, then calls the bottom handler and expects it to cope.  I have
introduced some local old_termflags variables so the state can be restored
on error, but am not sure if this is safe in all places.  It should be. 
(Below I suggest something similar for set_state).  [Thomas, admit it:  You
not only left error checking as an exercise for the reader, but also put
a few stones in the way to make it more interesting!]

user.c:
po_destroy_hook: Clearly error checking is useless here.  We can only hope
                 for the best.

trivfs_S_io_write: I have added error checking, but there is one additional
start_output at the end of it, which is called unconditionally.  We should
probably conditionalize this (and ignore its error), any suggestions? 

S_tiocltl_tiocflush, set_state:
We clear the queue before notifying the bottom handler of the flush (or
abandoning all output).  Does it make sense to ask the bottom handler to
flush, and if that fails, not to clear our queue?

set_state:  I don't know if we can recover from a failed set_bits, as the
global termstate is edited.  Can someone check this out?  Maybe saving and
restoring the old state.

munge.c:
I did not make any changes here.  The only change that we could make is
to check the return value of suspend_physical_output and only diddle the bit
if that worked.  For start_output I don't think we can do anything useful on
failure, and for abandon_physical_output see above (clearing the queue). 
drop_output is a void function anyway.

Thanks,
Marcus


Index: ChangeLog
===================================================================
RCS file: /cvsroot/hurd/hurd/term/ChangeLog,v
retrieving revision 1.67
diff -u -r1.67 ChangeLog
--- ChangeLog   4 Jan 2002 00:11:21 -0000       1.67
+++ ChangeLog   1 Feb 2002 02:19:51 -0000
@@ -1,3 +1,44 @@
+2002-02-01  Marcus Brinkmann  <marcus@gnu.org>
+
+       * users.c (open_hook): Save error value of set_bits.  Save old
+       termflags and restore them if if set_bits failed.
+       (S_tioctl_tiocmods): Set err to result of mdmctl.
+       (S_tioctl_tiocmset): Likewise.
+       (S_tioctl_tiocmbic): Likewise.
+       (S_tioctl_tiocmbis): Likewise.
+       (S_tioctl_tioccdtr): Likewise.
+       (S_tioctl_tiocsdtr): Likewise.
+       (S_tioctl_tioccbrk): Likewise for clear_break.
+       (S_tioctl_tiocsbrk): Likewise for set_break.
+       (S_tioctl_tiocstart): Likewise for start_output.  Save old
+       termflags and restore them if if start_output failed.
+       (S_tioctl_tiocstop): Likewise for stop_output.
+       (S_trivfs_io_write): Abort the operation if start_output fails.
+
+       * term.h (struct bottomhalf): Change the return types of the
+       following members from void to error_t: abandon_physical_output,
+       suspend_physical_output, notice_input_flushed, desert_dtr,
+       set_break, clear_break, start_output, set_bits, mdmctl.
+       * devio.c (devio_abandon_physical_output): Change return value to
+       error_t, and return 0.
+       (devio_suspend_physical_output): Likewise.
+       (devio_notice_input_flushed): Likewise.
+       (devio_desert_dtr): Likewise.
+       (devio_set_break): Likewise.
+       (devio_clear_break): Likewise.
+       (devio_start_output): Likewise.
+       (devio_set_bits): Likewise.
+       (devio_mdmctl): Likewise.
+       * ptyio.c (ptyio_suspend_physical_output): Likewise.
+       (ptyio_notice_input_flushed): Likewise.
+       (ptyio_desert_dtr): Likewise.
+       (ptyio_set_bits): Likewise.
+       (ptyio_set_break): Likewise.
+       (ptyio_clear_break): Likewise.
+       (ptyio_mdmctl): Likewise.
+       (ptyio_start_output): Likewise.
+       (ptyio_abandon_physical_output): Likewise.
+
 2002-01-04  Marcus Brinkmann  <marcus@gnu.org>
 
        * devio.c (bogus_speed_to_real_speed): Handle B57600, B115200 if
Index: devio.c
===================================================================
RCS file: /cvsroot/hurd/hurd/term/devio.c,v
retrieving revision 1.28
diff -u -r1.28 devio.c
--- devio.c     4 Jan 2002 00:11:21 -0000       1.28
+++ devio.c     1 Feb 2002 02:19:52 -0000
@@ -237,7 +237,7 @@
 
 /* If there are characters on the output queue and no
    pending output requests, then send them. */
-static void
+static error_t
 devio_start_output ()
 {
   char *cp;
@@ -247,7 +247,7 @@
   size = qsize (outputq);
 
   if (!size || output_pending || (termflags & USER_OUTPUT_SUSP))
-    return;
+    return 0;
 
   if (output_stopped)
     {
@@ -278,6 +278,7 @@
     devio_desert_dtr ();
   else if (!err)
     output_pending = 1;
+  return 0;
 }
 
 error_t
@@ -368,19 +369,21 @@
   return 0;
 }
 
-static void
+static error_t
 devio_set_break ()
 {
   device_set_status (phys_device, TTY_SET_BREAK, 0, 0);
+  return 0;
 }
 
-static void
+static error_t
 devio_clear_break ()
 {
   device_set_status (phys_device, TTY_CLEAR_BREAK, 0, 0);
+  return 0;
 }
 
-static void
+static error_t
 devio_abandon_physical_output ()
 {
   int val = D_WRITE;
@@ -388,7 +391,7 @@
   /* If this variable is clear, then carrier is gone, so we
      have nothing to do. */
   if (!phys_reply_writes_pi)
-    return;
+    return 0;
 
   mach_port_deallocate (mach_task_self (), phys_reply_writes);
   ports_reallocate_port (phys_reply_writes_pi);
@@ -397,9 +400,10 @@
   device_set_status (phys_device, TTY_FLUSH, &val, TTY_FLUSH_COUNT);
   npending_output = 0;
   output_pending = 0;
+  return 0;
 }
 
-static void
+static error_t
 devio_suspend_physical_output ()
 {
   if (!output_stopped)
@@ -407,11 +411,13 @@
       device_set_status (phys_device, TTY_STOP, 0, 0);
       output_stopped = 1;
     }
+  return 0;
 }
 
-static void
+static error_t
 devio_notice_input_flushed ()
 {
+  return 0;
 }
 
 static int
@@ -460,7 +466,7 @@
   return err;
 }
 
-static void
+static error_t
 devio_desert_dtr ()
 {
   int bits;
@@ -471,6 +477,7 @@
                     (dev_status_t) &bits, TTY_MODEM_COUNT);
 
   report_carrier_off ();
+  return 0;
 }
 
 static error_t
@@ -580,7 +587,7 @@
 /* Adjust physical state on the basis of the terminal state.
    Where it isn't possible, mutate terminal state to match
    reality. */
-static void
+static error_t
 devio_set_bits ()
 {
   if (!(termstate.c_cflag & CIGNORE) && phys_device != MACH_PORT_NULL)
@@ -636,9 +643,10 @@
       if (termstate.c_cflag & PARENB)
        char_size_mask_xxx |= 0x80;
     }
+  return 0;
 }
 
-static void
+static error_t
 devio_mdmctl (int how, int bits)
 {
   int oldbits, newbits;
@@ -661,6 +669,8 @@
 
   device_set_status (phys_device, TTY_MODEM,
                     (dev_status_t) &newbits, TTY_MODEM_COUNT);
+
+  return 0;
 }
 
 static int
Index: ptyio.c
===================================================================
RCS file: /cvsroot/hurd/hurd/term/ptyio.c,v
retrieving revision 1.31
diff -u -r1.31 ptyio.c
--- ptyio.c     11 Jul 1999 05:32:23 -0000      1.31
+++ ptyio.c     1 Feb 2002 02:19:54 -0000
@@ -138,7 +138,7 @@
 
 /* Lower half for tty node */
 
-static void 
+static error_t
 ptyio_start_output ()
 {
   if (packet_mode && output_stopped && (!(termflags & USER_OUTPUT_SUSP)))
@@ -148,9 +148,10 @@
       output_stopped = 0;
     }
   wake_reader ();
+  return 0;
 }
 
-static void
+static error_t
 ptyio_abandon_physical_output ()
 {
   if (packet_mode)
@@ -158,9 +159,10 @@
       control_byte |= TIOCPKT_FLUSHWRITE;
       wake_reader ();
     }
+  return 0;
 }
 
-static void
+static error_t
 ptyio_suspend_physical_output ()
 {
   if (packet_mode)
@@ -170,6 +172,7 @@
       output_stopped = 1;
       wake_reader ();
     }
+  return 0;
 }
 
 static int 
@@ -179,7 +182,7 @@
   return 0;
 }
 
-static void
+static error_t
 ptyio_notice_input_flushed ()
 {
   if (packet_mode)
@@ -187,6 +190,7 @@
       control_byte |= TIOCPKT_FLUSHREAD;
       wake_reader ();
     }
+  return 0;
 }
 
 static error_t 
@@ -196,14 +200,15 @@
   return 0;
 }
 
-static void 
+static error_t 
 ptyio_desert_dtr ()
 {
   dtr_on = 0;
   wake_reader ();
+  return 0;
 }
 
-static void 
+static error_t 
 ptyio_set_bits ()
 {
   if (packet_mode)
@@ -237,23 +242,27 @@
       if (wakeup)
        wake_reader ();
     }
+  return 0;
 }
 
 /* These do nothing.  In BSD the associated ioctls get errors, but
    I'd rather just ignore them. */
-static void 
+static error_t 
 ptyio_set_break ()
 {
+  return 0;
 }
 
-static void 
+static error_t
 ptyio_clear_break ()
 {
+  return 0;
 }
 
-static void
+static error_t
 ptyio_mdmctl (int a, int b)
 {
+  return 0;
 }
 
 static int
Index: term.h
===================================================================
RCS file: /cvsroot/hurd/hurd/term/term.h,v
retrieving revision 1.44
diff -u -r1.44 term.h
--- term.h      4 Oct 1999 14:39:04 -0000       1.44
+++ term.h      1 Feb 2002 02:19:54 -0000
@@ -140,17 +140,17 @@
 /* Functions a bottom half defines */
 struct bottomhalf
 {
-  void (*start_output) (void);
-  void (*set_break) (void);
-  void (*clear_break) (void);
-  void (*abandon_physical_output) (void);
-  void (*suspend_physical_output) (void);
+  error_t (*start_output) (void);
+  error_t (*set_break) (void);
+  error_t (*clear_break) (void);
+  error_t (*abandon_physical_output) (void);
+  error_t (*suspend_physical_output) (void);
   int (*pending_output_size) (void);
-  void (*notice_input_flushed) (void);
+  error_t (*notice_input_flushed) (void);
   error_t (*assert_dtr) (void);
-  void (*desert_dtr) (void);
-  void (*set_bits) (void);
-  void (*mdmctl) (int, int);
+  error_t (*desert_dtr) (void);
+  error_t (*set_bits) (void);
+  error_t (*mdmctl) (int, int);
   int (*mdmstate) (void);
 };
 
Index: users.c
===================================================================
RCS file: /cvsroot/hurd/hurd/term/users.c,v
retrieving revision 1.92
diff -u -r1.92 users.c
--- users.c     16 Jun 2001 20:24:02 -0000      1.92
+++ users.c     1 Feb 2002 02:19:57 -0000
@@ -212,8 +212,11 @@
 
   if (!err)
     {
+      int old_termflags = termflags;
       termflags |= TTY_OPEN;
-      (*bottom->set_bits) ();
+      err = (*bottom->set_bits) ();
+      if (err)
+       termflags = old_termflags;
     }
 
   mutex_unlock (&global_lock);
@@ -560,6 +563,7 @@
 {
   int i;
   int cancel;
+  error_t err = 0;
 
   if (!cred)
     return EOPNOTSUPP;
@@ -595,9 +599,14 @@
     {
       while (!qavail (outputq) && !cancel)
        {
-         (*bottom->start_output) ();
-         if (!qavail (outputq))
-           cancel = hurd_condition_wait (outputq->wait, &global_lock);
+         err = (*bottom->start_output) ();
+         if (err)
+           cancel = 1;
+         else
+           {
+             if (!qavail (outputq))
+               cancel = hurd_condition_wait (outputq->wait, &global_lock);
+           }
        }
       if (cancel)
        break;
@@ -615,7 +624,7 @@
 
   mutex_unlock (&global_lock);
 
-  return ((cancel && datalen && !*amt) ? EINTR : 0);
+  return ((cancel && datalen && !*amt) ? (err ?: EINTR) : 0);
 }
 
 /* Called for user reads from the terminal. */
@@ -922,10 +931,7 @@
   if (!(cred->po->openmodes & (O_READ|O_WRITE)))
     err = EBADF;
   else
-    {
-      (*bottom->mdmctl) (MDMCTL_SET, state);
-      err = 0;
-    }
+    err = (*bottom->mdmctl) (MDMCTL_SET, state);
 
   mutex_unlock (&global_lock);
 
@@ -1395,10 +1401,7 @@
   if (!(cred->po->openmodes & (O_READ|O_WRITE)))
     err = EBADF;
   else
-    {
-      (*bottom->mdmctl) (MDMCTL_SET, bits);
-      err = 0;
-    }
+    err = (*bottom->mdmctl) (MDMCTL_SET, bits);
 
   mutex_unlock (&global_lock);
   ports_port_deref (cred);
@@ -1427,10 +1430,7 @@
   if (!(cred->po->openmodes & (O_READ|O_WRITE)))
     err = EBADF;
   else
-    {
-      (*bottom->mdmctl) (MDMCTL_BIC, bits);
-      err = 0;
-    }
+    err = (*bottom->mdmctl) (MDMCTL_BIC, bits);
   mutex_unlock (&global_lock);
 
   ports_port_deref (cred);
@@ -1460,10 +1460,7 @@
   if (!(cred->po->openmodes & (O_READ|O_WRITE)))
     err = EBADF;
   else
-    {
-      (*bottom->mdmctl) (MDMCTL_BIS, bits);
-      err = 0;
-    }
+    err = (*bottom->mdmctl) (MDMCTL_BIS, bits);
   mutex_unlock (&global_lock);
   ports_port_deref (cred);
   return err;
@@ -1492,9 +1489,12 @@
     err = EBADF;
   else
     {
+      int old_termflags = termflags;
+
       termflags &= ~USER_OUTPUT_SUSP;
-      (*bottom->start_output) ();
-      err = 0;
+      err = (*bottom->start_output) ();
+      if (err)
+       termflags = old_termflags;
     }
   mutex_unlock (&global_lock);
 
@@ -1524,9 +1524,11 @@
     err = EBADF;
   else
     {
+      int old_termflags = termflags;
       termflags |= USER_OUTPUT_SUSP;
-      (*bottom->suspend_physical_output) ();
-      err = 0;
+      err = (*bottom->suspend_physical_output) ();
+      if (err)
+       termflags = old_termflags;
     }
   mutex_unlock (&global_lock);
 
@@ -1690,10 +1692,7 @@
   if (!(cred->po->openmodes & (O_READ|O_WRITE)))
     err = EBADF;
   else
-    {
-      (*bottom->mdmctl) (MDMCTL_BIC, TIOCM_DTR);
-      err = 0;
-    }
+    err = (*bottom->mdmctl) (MDMCTL_BIC, TIOCM_DTR);
   mutex_unlock (&global_lock);
 
   ports_port_deref (cred);
@@ -1721,10 +1720,7 @@
   if (!(cred->po->openmodes & (O_READ|O_WRITE)))
     err = EBADF;
   else
-    {
-      (*bottom->mdmctl) (MDMCTL_BIS, TIOCM_DTR);
-      err = 0;
-    }
+    err = (*bottom->mdmctl) (MDMCTL_BIS, TIOCM_DTR);
   mutex_unlock (&global_lock);
 
   ports_port_deref (cred);
@@ -1752,10 +1748,7 @@
   if (!(cred->po->openmodes & (O_READ|O_WRITE)))
     err = EBADF;
   else
-    {
-      (*bottom->clear_break) ();
-      err = 0;
-    }
+    err = (*bottom->clear_break) ();
   mutex_unlock (&global_lock);
 
   ports_port_deref (cred);
@@ -1783,10 +1776,7 @@
   if (!(cred->po->openmodes & (O_READ|O_WRITE)))
     err = EBADF;
   else
-    {
-      (*bottom->set_break) ();
-      err = 0;
-    }
+    err = (*bottom->set_break) ();
   mutex_unlock (&global_lock);
 
   ports_port_deref (cred);


-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de