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>

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index bc4f8272f3615b8054245ab1dfcceae4af0af9a1..176089ebd0f2a45975d623909c8c1b739da56d71 100644 (file)
--- 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 54677532806073894a26a351a904beb473ab280e..f24492d63ddbce72c2d33a4ce72022aabc50f7f1 100644 (file)
--- 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.*/