Fix bugs in vty error handlingex25b

Advance version to 0.99.20ex25b.

Bug fixes:

  * when closing a vty, if close() returned an error, the vtys would
    fall into a loop trying to recover.

    This was found on FreeBSD, which will return ECONNRESET on close(),
    which is not standard POSIX behaviour.

  * when closing a vty in response to EPIPE or ECONNRESET, managed to
    leave the vty mutex locked.

    This stopped the Routing Engine *dead* on the next CLI command
    that required the Routing Engine.  SIGTERM/SIGINT would not
    stop bgpd -- a SIGKILL would be required.

Other changes:

  * toned down the error reporting of EPIPE in the vty -- so no
    longer looks like an error...

      ...closing a vty by "exit" command is logged as "closed"

      ...closing a vty in response to the client closing their end of
         the connection is logged as "terminated: terminal closed",
         and no longer logs an EPIPE error.

  * changed error reporting on close() and shutdown() for vty, so
    that nothing is logged if an i/o error has already logged...

    ...so that redundant error messages are suppressed.

  * applied slightly finer jittering to bgpd timers.

  * work in progress on new vtysh.

1 files changed, 27 insertions, 10 deletions

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index 9b63e9d2..03266369 100644
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -36,6 +36,7 @@
 
 #include "lib/qtimers.h"
 #include "lib/sockunion.h"
+#include "lib/qrand.h"
 
 #include "bgpd/bgp_debug.h"
 #include "bgpd/bgp_network.h"
@@ -1539,7 +1540,7 @@ bgp_fsm_event(bgp_connection connection, bgp_fsm_event_t event)
  */
 
 static void
-bgp_hold_timer_set(bgp_connection connection, unsigned secs) ;
+bgp_hold_timer_set(bgp_connection connection, uint secs) ;
 
 /*------------------------------------------------------------------------------
  * Null action -- do nothing at all.
@@ -2282,27 +2283,43 @@ static qtimer_action bgp_keepalive_timer_action ;
  */
 enum
 {
-  no_jitter   = 0,
-  with_jitter = 1,
+  no_jitter   = false,
+  with_jitter = true,
 } ;
 
+static qrand_seq_t jseq = QRAND_SEQ_INIT(314159265) ;
+
 /*------------------------------------------------------------------------------
  * Start or reset given qtimer with given interval, in seconds.
  *
  * If the interval is zero, unset the timer.
  */
 static void
-bgp_timer_set(bgp_connection connection, qtimer timer, unsigned secs,
-                                              int jitter, qtimer_action* action)
+bgp_timer_set(bgp_connection connection, qtimer timer, uint secs,
+                                             bool jitter, qtimer_action* action)
 {
   if (secs == 0)
     qtimer_unset(timer) ;
   else
     {
-      secs *= 40 ;      /* a bit of resolution for jitter       */
-      if (jitter != no_jitter)
-        secs -= ((rand() % ((int)secs + 1)) / 4) ;
-      qtimer_set_interval(timer, QTIME(secs) / 40, action) ;
+      qtime_t interval ;
+
+      if (jitter)
+        {
+          /* Jitter as recommended in RFC 4271, Section 10
+           *
+           * We expect secs to be relatively small, so we can multiply it by
+           * some number 750..1000 without overflow !
+           */
+          qassert(secs <= (UINT_MAX / 1000)) ;
+
+          secs *= 750 + qrand(jseq, 250 + 1) ;
+          interval = QTIME(secs) / 1000 ;
+        }
+      else
+        interval = QTIME(secs) ;
+
+      qtimer_set_interval(timer, interval, action) ;
     } ;
 } ;
 
@@ -2313,7 +2330,7 @@ bgp_timer_set(bgp_connection connection, qtimer timer, unsigned secs,
  * Setting 0 will unset the HoldTimer.
  */
 static void
-bgp_hold_timer_set(bgp_connection connection, unsigned secs)
+bgp_hold_timer_set(bgp_connection connection, uint secs)
 {
   bgp_timer_set(connection, connection->hold_timer, secs, no_jitter,
                                                         bgp_hold_timer_action) ;