Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: Send "Peer De-configured" notification before closing the socket #5273

Closed
wants to merge 1 commit into from
Closed

bgpd: Send "Peer De-configured" notification before closing the socket #5273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions bgpd/bgp_fsm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,6 +1181,19 @@ int bgp_stop(struct peer *peer)
}
}

/* If a BGP speaker decides to de-configure a peer, then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bgp_stop() would be called when "BGP_Stop" even is raised. And "BGP_Stop" event can be raised at different states of bgp and for various reasons, not only for "Peer De-configured".
Eg clear bgp cmd

so we would end up

  1. sending "Peer De-configured" notification wrongly even when the peer is not delete.
  2. We might end up sending more than one notification message (pls see the below comment for the reason)

* the speaker SHOULD send a NOTIFICATION message with the
* Error Code Cease and the Error Subcode "Peer De-configured".
*
* Make sure this is sent only after all the timers are stopped
* and before the peer's socket is closed to avoid race condition
* when you send a notification, FSM Connect fires before
* closing the socket and you still see `Peer closed the session`.
*/
if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->status))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending notification inside bgp_stop() would lead to generating a "BGP_Stop" event inside the function bgp_write_notify(), there by calling bgp_stop() again ?

BGP_XYZ_State --> Idle ----> Idle

This can be solved by moving the bgp stop event generation from bgp_write_notify() (--> BGP_EVENT_ADD(peer, BGP_Stop) )to the respective calling function.

bgp_notify_send(peer, BGP_NOTIFY_CEASE,
BGP_NOTIFY_CEASE_PEER_UNCONFIG);

/* Close of file descriptor. */
if (peer->fd >= 0) {
close(peer->fd);
Expand Down
10 changes: 7 additions & 3 deletions bgpd/bgp_packet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,8 +685,11 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code,
/* Set BGP packet length. */
bgp_packet_set_size(s);

/* wipe output buffer */
stream_fifo_clean(peer->obuf);
/* Output buffer could be already cleaned by bgp_stop()
* thus check if it's not NULL.
*/
if (peer->obuf)
stream_fifo_clean(peer->obuf);

/*
* If possible, store last packet for debugging purposes. This check is
Expand Down Expand Up @@ -753,7 +756,8 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code,
peer->last_reset = PEER_DOWN_NOTIFY_SEND;

/* Add packet to peer's output queue */
stream_fifo_push(peer->obuf, s);
if (peer->obuf)
stream_fifo_push(peer->obuf, s);

bgp_write_notify(peer);
}
Expand Down