More found connection conversion issues #17385

donaldsharp · 2024-11-07T17:02:17Z

I have started another round of converting the struct peer doppelganger away and found some more functions that should properly be connection oriented. Move them over. I also made some code changes to make the coding pattern in bgp to be a bit better.

bgpd/bgpd.c

riw777

looks good

github-actions · 2024-11-12T16:56:14Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

ton31337 · 2024-11-20T17:20:29Z

bgpd/bgpd.c

+void peer_notify_unconfig(struct peer_connection *connection)
+{
+	if (BGP_IS_VALID_STATE_FOR_NOTIF(connection->status))
+		bgp_notify_send(connection, BGP_NOTIFY_CEASE, BGP_NOTIFY_CEASE_CONFIG_CHANGE);


This should be BGP_NOTIFY_CEASE_PEER_UNCONFIG, right?

yes I got it backwards, fixed

ton31337 · 2024-11-20T17:20:42Z

bgpd/bgpd.c

-		bgp_notify_send(peer->connection, BGP_NOTIFY_CEASE,
-				BGP_NOTIFY_CEASE_PEER_UNCONFIG);
+	if (BGP_IS_VALID_STATE_FOR_NOTIF(connection->status)) {
+		bgp_notify_send(connection, BGP_NOTIFY_CEASE, BGP_NOTIFY_CEASE_PEER_UNCONFIG);


Here it should be BGP_NOTIFY_CEASE_CONFIG_CHANGE, right?

github-actions · 2024-11-21T20:36:14Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

ton31337

I see still lots of places where BGP_IS_VALID_STATE_FOR_NOTIF() is called, but it's actually called also in peer_notify_config_change(), correct?

ton31337 · 2024-11-26T13:56:09Z

bgpd/bgpd.c

@@ -7057,8 +7006,7 @@ int peer_password_unset(struct peer *peer)

 		/* Send notification or reset peer depending on state. */
 		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))


peer_notify_config_change() already do this check, right?

fixed them all

ton31337 · 2024-11-26T13:56:12Z

bgpd/bgpd.c

@@ -7030,8 +6980,7 @@ int peer_password_unset(struct peer *peer)
 	if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
 		/* Send notification or reset peer depending on state. */
 		if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->connection->status))


peer_notify_config_change() already do this check, right?

ton31337 · 2024-11-26T13:56:16Z

bgpd/bgpd.c

@@ -6984,8 +6935,7 @@ int peer_password_set(struct peer *peer, const char *password)
 		member->last_reset = PEER_DOWN_PASSWORD_CHANGE;
 		/* Send notification or reset peer depending on state. */
 		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))


peer_notify_config_change() already do this check, right?

ton31337 · 2024-11-26T13:56:20Z

bgpd/bgpd.c

@@ -6946,8 +6898,7 @@ int peer_password_set(struct peer *peer, const char *password)
 		peer->last_reset = PEER_DOWN_PASSWORD_CHANGE;
 		/* Send notification or reset peer depending on state. */
 		if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->connection->status))


peer_notify_config_change() already do this check, right?

ton31337 · 2024-11-26T13:56:29Z

bgpd/bgpd.c

@@ -6914,8 +6867,7 @@ int peer_local_as_unset(struct peer *peer)

 		/* Send notification or stop peer depending on state. */
 		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))


ton31337 · 2024-11-26T13:56:36Z

bgpd/bgpd.c

@@ -5852,8 +5807,7 @@ void peer_update_source_unset(struct peer *peer)

 		/* Send notification or reset peer depending on state. */
 		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))


ton31337 · 2024-11-26T13:56:40Z

bgpd/bgpd.c

@@ -5816,8 +5772,7 @@ void peer_update_source_unset(struct peer *peer)
 		peer->last_reset = PEER_DOWN_UPDATE_SOURCE_CHANGE;
 		/* Send notification or reset peer depending on state. */
 		if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->connection->status))


ton31337 · 2024-11-26T13:56:44Z

bgpd/bgpd.c

@@ -5765,8 +5722,7 @@ void peer_update_source_addr_set(struct peer *peer, const union sockunion *su)

 		/* Send notification or reset peer depending on state. */
 		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))


ton31337 · 2024-11-26T13:57:00Z

bgpd/bgpd.c

@@ -5427,9 +5392,7 @@ int peer_ebgp_multihop_set(struct peer *peer, int ttl)
 		if (peer->sort != BGP_PEER_IBGP) {
 			if (BGP_IS_VALID_STATE_FOR_NOTIF(


ton31337 · 2024-11-26T14:29:51Z

bgpd/bgpd.c

-			bgp_notify_send(member->connection, BGP_NOTIFY_CEASE,
-					BGP_NOTIFY_CEASE_CONFIG_CHANGE);
-		else
+		if (peer_notify_config_change(peer->connection))


!peer_notify_config_change() :)

ton31337

LGTM

github-actions · 2024-11-26T16:07:22Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Convert this function to being connection based. Signed-off-by: Donald Sharp <[email protected]>

We have about a bajillion tests of if we can notify the peer and then we send a config change notification. Let's just make a function that does this. Signed-off-by: Donald Sharp <[email protected]>

if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->connection->status)) peer_notify_config_change(peer->connection); else bgp_session_reset_safe(peer, &nnode); Let's add a bool return to peer_notify_config_change of whether or not it should call the peer session reset. This simplifies the code a bunch. Signed-off-by: Donald Sharp <[email protected]>

Signed-off-by: Donald Sharp <[email protected]>

Let's use the connection associated with the peer instead. Signed-off-by: Donald Sharp <[email protected]>

Signed-off-by: Donald Sharp <[email protected]>

frrbot bot added the bgp label Nov 7, 2024

github-actions bot added master size/L labels Nov 7, 2024

ton31337 reviewed Nov 8, 2024

View reviewed changes

bgpd/bgpd.c Outdated Show resolved Hide resolved

riw777 approved these changes Nov 12, 2024

View reviewed changes

github-actions bot added the conflicts label Nov 12, 2024

donaldsharp force-pushed the more_found_connection_conversion_issues branch from f99d54e to e3f24e5 Compare November 19, 2024 14:37

github-actions bot removed the conflicts label Nov 19, 2024

ton31337 reviewed Nov 20, 2024

View reviewed changes

github-actions bot added the conflicts label Nov 21, 2024

donaldsharp force-pushed the more_found_connection_conversion_issues branch from e3f24e5 to d1772d6 Compare November 25, 2024 18:10

github-actions bot removed the conflicts label Nov 25, 2024

ton31337 reviewed Nov 26, 2024

View reviewed changes

donaldsharp force-pushed the more_found_connection_conversion_issues branch from d1772d6 to 1bdce30 Compare November 26, 2024 14:24

ton31337 reviewed Nov 26, 2024

View reviewed changes

donaldsharp force-pushed the more_found_connection_conversion_issues branch from 1bdce30 to 90f78f6 Compare November 26, 2024 14:40

ton31337 approved these changes Nov 26, 2024

View reviewed changes

github-actions bot added the conflicts label Nov 26, 2024

donaldsharp added 7 commits November 26, 2024 11:49

bgpd: peer_notify_unconfig should be connection based

2e5b4e3

Convert this function to being connection based. Signed-off-by: Donald Sharp <[email protected]>

bgpd: Add peer_notify_config_change() function

ba0edb9

We have about a bajillion tests of if we can notify the peer and then we send a config change notification. Let's just make a function that does this. Signed-off-by: Donald Sharp <[email protected]>

bgpd: Modify bgp_udpatesockname to pass in a connection

2771431

Signed-off-by: Donald Sharp <[email protected]>

bgpd: Modify bgp_connect_in_progress_update_connection to use connection

72f716e

Signed-off-by: Donald Sharp <[email protected]>

bgpd: bgp_getsockname should use connection

1baeb81

Let's use the connection associated with the peer instead. Signed-off-by: Donald Sharp <[email protected]>

bgpd: peer_active is connection oriented, make it so

7bf3f53

Signed-off-by: Donald Sharp <[email protected]>

donaldsharp force-pushed the more_found_connection_conversion_issues branch from 90f78f6 to 7bf3f53 Compare November 26, 2024 16:59

github-actions bot removed the conflicts label Nov 26, 2024

ton31337 merged commit 4c2f5c7 into FRRouting:master Nov 26, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More found connection conversion issues #17385

More found connection conversion issues #17385

donaldsharp commented Nov 7, 2024

riw777 left a comment

github-actions bot commented Nov 12, 2024

ton31337 Nov 20, 2024

donaldsharp Nov 25, 2024

ton31337 Nov 20, 2024

donaldsharp Nov 25, 2024

github-actions bot commented Nov 21, 2024

ton31337 left a comment

ton31337 Nov 26, 2024

donaldsharp Nov 26, 2024

ton31337 Nov 26, 2024

ton31337 Nov 26, 2024

ton31337 Nov 26, 2024

ton31337 Nov 26, 2024

ton31337 Nov 26, 2024

ton31337 Nov 26, 2024

ton31337 Nov 26, 2024

ton31337 Nov 26, 2024

ton31337 Nov 26, 2024

donaldsharp Nov 26, 2024

ton31337 left a comment

github-actions bot commented Nov 26, 2024

		@@ -7057,8 +7006,7 @@ int peer_password_unset(struct peer *peer)

		/* Send notification or reset peer depending on state. */
		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))

		@@ -6914,8 +6867,7 @@ int peer_local_as_unset(struct peer *peer)

		/* Send notification or stop peer depending on state. */
		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))

		@@ -5852,8 +5807,7 @@ void peer_update_source_unset(struct peer *peer)

		/* Send notification or reset peer depending on state. */
		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))

		@@ -5765,8 +5722,7 @@ void peer_update_source_addr_set(struct peer peer, const union sockunion su)

		/* Send notification or reset peer depending on state. */
		if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))

		@@ -5427,9 +5392,7 @@ int peer_ebgp_multihop_set(struct peer *peer, int ttl)
		if (peer->sort != BGP_PEER_IBGP) {
		if (BGP_IS_VALID_STATE_FOR_NOTIF(

More found connection conversion issues #17385

More found connection conversion issues #17385

Conversation

donaldsharp commented Nov 7, 2024

riw777 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2024

ton31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ton31337 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2024