From 07a165268236fc64380d3abff46ae950e1799e65 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Fri, 24 Mar 2017 19:05:56 +0000 Subject: [PATCH] bgpd: move bgp_connect_check() to bgp_fsm.c Prior to this change, after initiating a nonblocking connection to the remote peer bgpd would call both BGP_READ_ON and BGP_WRITE_ON on the peer's socket. This resulted in a call to select(), so that when some event (either a connection success or failure) occurred on the socket, one of bgp_read() or bgp_write() would run. At the beginning of each of those functions was a hook into bgp_connect_check(), which checked the socket status and issued the correct connection event onto the BGP FSM. This code is better suited for bgp_fsm.c. Placing it there avoids scheduling packet reads or writes when we don't know if the socket has established a connection yet, and the specific functionality is a better fit for the responsibility scope of this unit. This change also helps isolate the responsibilities of the packet-writing kernel thread. Signed-off-by: Quentin Young --- bgpd/bgp_fsm.c | 48 +++++++++++++++++++++++++++++++++++-- bgpd/bgp_packet.c | 60 +++++------------------------------------------ 2 files changed, 52 insertions(+), 56 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index bc4f8272f3..176089ebd0 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -1176,6 +1176,48 @@ static int bgp_stop_with_notify(struct peer *peer, u_char code, u_char sub_code) return (bgp_stop(peer)); } +/** + * Determines whether a TCP session has successfully established for a peer and + * events as appropriate. + * + * This function is called when setting up a new session. After connect() is + * called on the peer's socket (in bgp_start()), the fd is passed to select() + * to wait for connection success or failure. When select() returns, this + * function is called to evaluate the result. + */ +static int bgp_connect_check(struct thread *thread) +{ + int status; + socklen_t slen; + int ret; + struct peer *peer; + + peer = THREAD_ARG(thread); + + /* Check file descriptor. */ + slen = sizeof(status); + ret = getsockopt(peer->fd, SOL_SOCKET, SO_ERROR, (void *)&status, + &slen); + + /* If getsockopt is fail, this is fatal error. */ + if (ret < 0) { + zlog_info("can't get sockopt for nonblocking connect"); + BGP_EVENT_ADD(peer, TCP_fatal_error); + return -1; + } + + /* When status is 0 then TCP connection is established. */ + if (status == 0) { + BGP_EVENT_ADD(peer, TCP_connection_open); + return 1; + } else { + if (bgp_debug_neighbor_events(peer)) + zlog_debug("%s [Event] Connect failed (%s)", peer->host, + safe_strerror(errno)); + BGP_EVENT_ADD(peer, TCP_connection_open_failed); + return 0; + } +} /* TCP connection open. Next we send open message to remote peer. And add read thread for reading open message. */ @@ -1329,8 +1371,10 @@ int bgp_start(struct peer *peer) peer->fd); return -1; } - BGP_READ_ON(peer->t_read, bgp_read, peer->fd); - peer_writes_on(peer); + // when the socket becomes ready (or fails to connect), + // bgp_connect_check + // will be called. + thread_add_read(bm->master, bgp_connect_check, peer, peer->fd); break; } return 0; diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 5467753280..f24492d63d 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -129,43 +129,6 @@ static void bgp_packet_delete_unsafe(struct peer *peer) stream_free(stream_fifo_pop(peer->obuf)); } - -/* Check file descriptor whether connect is established. */ -static int bgp_connect_check(struct peer *peer, int change_state) -{ - int status; - socklen_t slen; - int ret; - - /* Anyway I have to reset read and write thread. */ - BGP_READ_OFF(peer->t_read); - - /* Check file descriptor. */ - slen = sizeof(status); - ret = getsockopt(peer->fd, SOL_SOCKET, SO_ERROR, (void *)&status, - &slen); - - /* If getsockopt is fail, this is fatal error. */ - if (ret < 0) { - zlog_info("can't get sockopt for nonblocking connect"); - BGP_EVENT_ADD(peer, TCP_fatal_error); - return -1; - } - - /* When status is 0 then TCP connection is established. */ - if (status == 0) { - BGP_EVENT_ADD(peer, TCP_connection_open); - return 1; - } else { - if (bgp_debug_neighbor_events(peer)) - zlog_debug("%s [Event] Connect failed (%s)", peer->host, - safe_strerror(errno)); - if (change_state) - BGP_EVENT_ADD(peer, TCP_connection_open_failed); - return 0; - } -} - static struct stream *bgp_update_packet_eor(struct peer *peer, afi_t afi, safi_t safi) { @@ -2055,19 +2018,14 @@ int bgp_read(struct thread *thread) */ notify_out = peer->notify_out; - /* For non-blocking IO check. */ - if (peer->status == Connect) { - bgp_connect_check(peer, 1); - goto done; - } else { - if (peer->fd < 0) { - zlog_err("bgp_read peer's fd is negative value %d", - peer->fd); - return -1; - } - BGP_READ_ON(peer->t_read, bgp_read, peer->fd); + if (peer->fd < 0) { + zlog_err("bgp_read(): peer's fd is negative value %d", + peer->fd); + return -1; } + BGP_READ_ON(peer->t_read, bgp_read, peer->fd); + /* Read packet header to determine type of the packet */ if (peer->packet_size == 0) peer->packet_size = BGP_HEADER_SIZE; @@ -2233,12 +2191,6 @@ static int bgp_write(struct peer *peer) unsigned int count = 0; unsigned int oc = 0; - /* For non-blocking IO check. */ - if (peer->status == Connect) { - bgp_connect_check(peer, 1); - return 0; - } - /* Write packets. The number of packets written is the value of * bgp->wpkt_quanta or the size of the output buffer, whichever is * smaller.*/ -- 2.39.5