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 <qlyoung@cumulusnetworks.com>

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);
+		// when the socket becomes ready (or fails to connect),
-		peer_writes_on(peer);
+		// bgp_connect_check
+		// will be called.
+		thread_add_read(bm->master, bgp_connect_check, peer, peer->fd);
 		break;
 	}
 	return 0;

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,18 +2018,13 @@ 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",
+		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)
@@ -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.*/