Skip to content

Commit

Permalink
Fix bugs in vty error handling
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Chris Hall committed Jun 8, 2012
1 parent 12d01b8 commit 58b4a34
Show file tree
Hide file tree
Showing 15 changed files with 1,608 additions and 665 deletions.
37 changes: 27 additions & 10 deletions bgpd/bgp_fsm.c
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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) ;
} ;
} ;

Expand All @@ -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) ;
Expand Down
2 changes: 1 addition & 1 deletion configure.ac
View file
Expand Up @@ -7,7 +7,7 @@
##
AC_PREREQ(2.53)

AC_INIT(Quagga, 0.99.20ex24b, [http://bugzilla.quagga.net])
AC_INIT(Quagga, 0.99.20ex25b, [http://bugzilla.quagga.net])
AC_CONFIG_SRCDIR(lib/zebra.h)
AC_CONFIG_MACRO_DIR([m4])

Expand Down
140 changes: 125 additions & 15 deletions lib/list_util.h
View file
Expand Up @@ -512,14 +512,18 @@ Private bool ssl_del_func(void** p_prev, void* item, size_t link_offset)
*
* ddl_in_after(after, base, item, list) -- insert after
*
* Treat as void function. The after & item may *not* be NULL.
* Treat as void function. The item may *not* be NULL.
*
* If after == NULL, insert item at the end of the current list.
*
* Undefined if item is already on any list (including this one), or if
* after is not on the list.
*
* ddl_in_before(before, base, item, list) -- insert before
*
* Treat as void function. The before & item may *not* be NULL.
* Treat as void function. The item may *not* be NULL.
*
* If before == NULL, insert item at the start of the current list.
*
* Undefined if item is already on any list (including this one), or if
* before is not on the list.
Expand Down Expand Up @@ -570,14 +574,32 @@ Private bool ssl_del_func(void** p_prev, void* item, size_t link_offset)
*
* Treat as function returning void*. Returns NULL if the item is NULL.
*
* ddl_slice(base, sub, list) -- remove sublist from given list
*
* Treat as void function. Does nothing if the sublist is empty.
*
* ddl_splice_after(after, base, sub, list)
* -- insert sublist after given item
*
* Treat as void function. Does nothing if the sublist is empty.
*
* If after == NULL, insert sublist at the end of the current list.
*
* ddl_splice_before(before, base, sub, list)
* -- insert sublist before given item
*
* Treat as void function. Does nothing if the sublist is empty.
*
* If before == NULL, insert sublist at the start of the current list.
*
* Note that ddl_del() and ddl_pop() do NOT affect the item->list.next
* or item->list.prev pointers.
*
* Where:
*
* "base" to be an r-value of type: struct base_pair(struct item*)*
*
* That is... a variable or field which is a pointer to
* That is... a variable or field which is a pointer pair.
*
* "item" to be an l-value of type struct item*
*
Expand All @@ -586,7 +608,9 @@ Private bool ssl_del_func(void** p_prev, void* item, size_t link_offset)
* "list" to be the name of a field in struct item
* of type: struct list_pair(struct item*)
*
* "sub" to be an r-value of type: struct base_pair(struct item*)*
*
* That is... a variable or field which is a pointer pointer pair.
*
*------------------------------------------------------------------------------
* For example:
Expand Down Expand Up @@ -672,23 +696,33 @@ Private bool ssl_del_func(void** p_prev, void* item, size_t link_offset)
} while (0)

#define ddl_in_after(after, base, item, list) \
do { (item)->list.next = (after)->list.next ; \
(item)->list.prev = (after) ; \
if ((after)->list.next != NULL) \
(after)->list.next->list.prev = (item) ; \
do { if (after != NULL) \
{ \
(item)->list.next = (after)->list.next ; \
(item)->list.prev = (after) ; \
if ((after)->list.next != NULL) \
(after)->list.next->list.prev = (item) ; \
else \
(base).tail = (item) ; \
(after)->list.next = (item) ; \
} \
else \
(base).tail = (item) ; \
(after)->list.next = (item) ; \
ddl_append(base, item, list) ; \
} while (0)

#define ddl_in_before(before, base, item, list) \
do { (item)->list.next = (before) ; \
(item)->list.prev = (before)->list.prev ; \
if ((before)->list.prev != NULL) \
(before)->list.prev->list.next = (item) ; \
do { if (before != NULL) \
{ \
(item)->list.next = (before) ; \
(item)->list.prev = (before)->list.prev ; \
if ((before)->list.prev != NULL) \
(before)->list.prev->list.next = (item) ; \
else \
(base).head = (item) ; \
(before)->list.prev = (item) ; \
} \
else \
(base).head = (item) ; \
(before)->list.prev = (item) ; \
ddl_push(base, item, list) ; \
} while (0)

#define ddl_del(base, item, list) \
Expand Down Expand Up @@ -751,6 +785,82 @@ Private bool ssl_del_func(void** p_prev, void* item, size_t link_offset)
#define ddl_prev(item, list) \
((item) != NULL ? (item)->list.prev : NULL)

#define ddl_slice(base, sub, list) \
do { if ((sub).head != NULL) \
{ \
if ((sub).head->list.prev != NULL) \
(sub).head->list.prev->list.next = (sub).tail->list.next ; \
else \
{ \
qassert((sub).head == (base).head) ; \
(base).head = (sub).tail->list.next ; \
} ; \
\
if ((sub).tail->list.next != NULL) \
(sub).tail->list.next->list.prev = (sub).head->list.prev ; \
else \
{ \
qassert((sub).tail == (base).tail) ; \
(base).tail = (sub).head->list.prev ; \
} ; \
\
(sub).head->list.prev = NULL ; \
(sub).tail->list.next = NULL ; \
} \
} while (0)

#define ddl_splice_after(after, base, sub, list) \
do { if ((sub).head != NULL) \
{ \
if (after != NULL) \
{ \
(sub).head->list.prev = (after) ; \
(sub).tail->list.next = (after)->list.next ; \
if ((after)->list.next != NULL) \
(after)->list.next->list.prev = (sub).tail ; \
else \
(base).tail = (sub).tail ; \
(after)->list.next = (sub).head ; \
} \
else \
{ \
(sub).head->list.prev = (base).tail ; \
(sub).tail->list.next = NULL ; \
if ((base).tail != NULL) \
(base).tail->list.next = (sub).head ; \
else \
(base).head = (sub).head ; \
(base).tail = (sub).tail ; \
} ; \
} \
} while (0)

#define ddl_splice_before(before, base, sub, list) \
do { if ((sub).head != NULL) \
{ \
if (before != NULL) \
{ \
(sub).tail->list.next = (before) ; \
(sub).head->list.prev = (before)->list.prev ; \
if ((before)->list.prev != NULL) \
(before)->list.prev->list.next = (sub).head ; \
else \
(base).head = (sub).head ; \
(before)->list.prev = (sub).tail ; \
} \
else \
{ \
(sub).tail->list.next = (base).head ; \
(sub).head->list.prev = NULL ; \
if ((base).head != NULL) \
(base).head->list.prev = (sub).tail ; \
else \
(base).tail = (sub).tail ; \
(base).head = (sub).head ; \
} ; \
} \
} while (0)

/*==============================================================================
* Double Base, Single Link
*
Expand Down
9 changes: 4 additions & 5 deletions lib/qrand.c
View file
Expand Up @@ -32,17 +32,16 @@
*
* If range == 1, returns 0 every time !
*/
extern int
qrand(qrand_seq seq, int range)
extern uint
qrand(qrand_seq seq, uint range)
{
uint64_t r ;

r = seq->last ^ 3141592653 ;
r = ((r * 2650845021) + 5) & 0xFFFFFFFF ; /* see Knuth */
r = ((seq->last * 2650845021) + 5) & 0xFFFFFFFF ; /* see Knuth */
seq->last = r ;

if (range == 0)
return r >> 1 ;
else
return (r % range) ;
return (r * range) >> 32 ;
} ;
4 changes: 2 additions & 2 deletions lib/qrand.h
View file
Expand Up @@ -35,7 +35,7 @@

struct qrand_seq
{
uint last ;
uint32_t last ;
} ;

typedef struct qrand_seq* qrand_seq ;
Expand All @@ -47,6 +47,6 @@ typedef struct qrand_seq qrand_seq_t[1] ;
* Functions
*/

extern int qrand(qrand_seq seq, int range) ;
extern uint qrand(qrand_seq seq, uint range) ;

#endif /* _ZEBRA_QRAND_H */
2 changes: 1 addition & 1 deletion lib/vector.c
View file
Expand Up @@ -903,7 +903,7 @@ vector_sak(int to_copy, vector to,
if (n_dst_nulls > 0)
memset(&dst->p_items[dst->end], 0, P_ITEMS_SIZE(n_dst_nulls)) ;
if (n_dst_move > 0)
memmove(&dst->p_items[i_dst + n_dst], &dst->p_items[i_dst + n_src],
memmove(&dst->p_items[i_dst + n_src], &dst->p_items[i_dst + n_dst],
P_ITEMS_SIZE(n_dst_move)) ;
if (n_src_real > 0)
memcpy(&dst->p_items[i_dst], &src->p_items[i_src],
Expand Down

0 comments on commit 58b4a34

Please sign in to comment.