Clean up code in message_bridge based on advice from Sarah
I accidentally edited instead of duplicated a test as well.
Change-Id: Ic30646c10ff6aa1baf37f1a7e1b26086eafef105
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>
diff --git a/aos/network/message_bridge_client_lib.cc b/aos/network/message_bridge_client_lib.cc
index 6db772a..983d38a 100644
--- a/aos/network/message_bridge_client_lib.cc
+++ b/aos/network/message_bridge_client_lib.cc
@@ -170,44 +170,48 @@
return;
}
- if (message->message_type == Message::kNotification) {
- const union sctp_notification *snp =
- (const union sctp_notification *)message->data();
+ switch (message->message_type) {
+ case Message::kNotification: {
+ const union sctp_notification *snp =
+ (const union sctp_notification *)message->data();
- switch (snp->sn_header.sn_type) {
- case SCTP_ASSOC_CHANGE: {
- const struct sctp_assoc_change *sac = &snp->sn_assoc_change;
- switch (sac->sac_state) {
- case SCTP_RESTART:
- NodeDisconnected();
- [[fallthrough]];
- case SCTP_COMM_UP:
- NodeConnected(sac->sac_assoc_id);
+ switch (snp->sn_header.sn_type) {
+ case SCTP_ASSOC_CHANGE: {
+ const struct sctp_assoc_change *sac = &snp->sn_assoc_change;
+ switch (sac->sac_state) {
+ case SCTP_RESTART:
+ NodeDisconnected();
+ [[fallthrough]];
+ case SCTP_COMM_UP:
+ NodeConnected(sac->sac_assoc_id);
- VLOG(1) << "Received up from " << message->PeerAddress() << " on "
- << sac->sac_assoc_id << " state " << sac->sac_state;
- break;
- case SCTP_COMM_LOST:
- case SCTP_SHUTDOWN_COMP:
- case SCTP_CANT_STR_ASSOC: {
- NodeDisconnected();
- VLOG(1) << "Disconnect from " << message->PeerAddress() << " on "
- << sac->sac_assoc_id << " state " << sac->sac_state;
- } break;
- default:
- LOG(FATAL) << "Never seen state " << sac->sac_state << " before.";
- break;
- }
- } break;
- }
+ VLOG(1) << "Received up from " << message->PeerAddress() << " on "
+ << sac->sac_assoc_id << " state " << sac->sac_state;
+ break;
+ case SCTP_COMM_LOST:
+ case SCTP_SHUTDOWN_COMP:
+ case SCTP_CANT_STR_ASSOC: {
+ NodeDisconnected();
+ VLOG(1) << "Disconnect from " << message->PeerAddress() << " on "
+ << sac->sac_assoc_id << " state " << sac->sac_state;
+ } break;
+ default:
+ LOG(FATAL) << "Never seen state " << sac->sac_state << " before.";
+ break;
+ }
+ } break;
+ }
- if (VLOG_IS_ON(1)) {
- PrintNotification(message.get());
- }
- } else if (message->message_type == Message::kMessage) {
- HandleData(message.get());
- } else if (message->message_type == Message::kOverflow) {
- NodeDisconnected();
+ if (VLOG_IS_ON(1)) {
+ PrintNotification(message.get());
+ }
+ } break;
+ case Message::kMessage:
+ HandleData(message.get());
+ break;
+ case Message::kOverflow:
+ NodeDisconnected();
+ break;
}
client_.FreeMessage(std::move(message));
}
diff --git a/aos/network/message_bridge_server_lib.cc b/aos/network/message_bridge_server_lib.cc
index 07ea969..6b81856 100644
--- a/aos/network/message_bridge_server_lib.cc
+++ b/aos/network/message_bridge_server_lib.cc
@@ -460,44 +460,48 @@
return;
}
- if (message->message_type == Message::kNotification) {
- const union sctp_notification *snp =
- (const union sctp_notification *)message->data();
+ switch (message->message_type) {
+ case Message::kNotification: {
+ const union sctp_notification *snp =
+ (const union sctp_notification *)message->data();
- if (VLOG_IS_ON(2)) {
- PrintNotification(message.get());
- }
+ if (VLOG_IS_ON(2)) {
+ PrintNotification(message.get());
+ }
- switch (snp->sn_header.sn_type) {
- case SCTP_ASSOC_CHANGE: {
- const struct sctp_assoc_change *sac = &snp->sn_assoc_change;
- switch (sac->sac_state) {
- case SCTP_RESTART:
- NodeDisconnected(sac->sac_assoc_id);
- [[fallthrough]];
- case SCTP_COMM_UP:
- NodeConnected(sac->sac_assoc_id);
- VLOG(1) << "Received up from " << message->PeerAddress() << " on "
- << sac->sac_assoc_id << " state " << sac->sac_state;
- break;
- case SCTP_COMM_LOST:
- case SCTP_SHUTDOWN_COMP:
- case SCTP_CANT_STR_ASSOC:
- NodeDisconnected(sac->sac_assoc_id);
- VLOG(1) << "Disconnect from " << message->PeerAddress() << " on "
- << sac->sac_assoc_id << " state " << sac->sac_state;
- break;
- default:
- LOG(FATAL) << "Never seen state " << sac->sac_state << " before.";
- break;
- }
- } break;
- }
- } else if (message->message_type == Message::kMessage) {
- HandleData(message.get());
- } else if (message->message_type == Message::kOverflow) {
- MaybeIncrementInvalidConnectionCount(nullptr);
- NodeDisconnected(message->header.rcvinfo.rcv_assoc_id);
+ switch (snp->sn_header.sn_type) {
+ case SCTP_ASSOC_CHANGE: {
+ const struct sctp_assoc_change *sac = &snp->sn_assoc_change;
+ switch (sac->sac_state) {
+ case SCTP_RESTART:
+ NodeDisconnected(sac->sac_assoc_id);
+ [[fallthrough]];
+ case SCTP_COMM_UP:
+ NodeConnected(sac->sac_assoc_id);
+ VLOG(1) << "Received up from " << message->PeerAddress() << " on "
+ << sac->sac_assoc_id << " state " << sac->sac_state;
+ break;
+ case SCTP_COMM_LOST:
+ case SCTP_SHUTDOWN_COMP:
+ case SCTP_CANT_STR_ASSOC:
+ NodeDisconnected(sac->sac_assoc_id);
+ VLOG(1) << "Disconnect from " << message->PeerAddress() << " on "
+ << sac->sac_assoc_id << " state " << sac->sac_state;
+ break;
+ default:
+ LOG(FATAL) << "Never seen state " << sac->sac_state << " before.";
+ break;
+ }
+ } break;
+ }
+ } break;
+ case Message::kMessage:
+ HandleData(message.get());
+ break;
+ case Message::kOverflow:
+ MaybeIncrementInvalidConnectionCount(nullptr);
+ NodeDisconnected(message->header.rcvinfo.rcv_assoc_id);
+ break;
}
server_.FreeMessage(std::move(message));
}
diff --git a/aos/network/message_bridge_test.cc b/aos/network/message_bridge_test.cc
index 9106a86..66826c3 100644
--- a/aos/network/message_bridge_test.cc
+++ b/aos/network/message_bridge_test.cc
@@ -1346,6 +1346,109 @@
pi1_remote_timestamp_thread.reset();
}
+// Test that differing config sha256's result in no connection.
+TEST_P(MessageBridgeParameterizedTest, MismatchedSha256) {
+ // This is rather annoying to set up. We need to start up a client and
+ // server, on the same node, but get them to think that they are on different
+ // nodes.
+ //
+ // We need the client to not post directly to "/test" like it would in a
+ // real system, otherwise we will re-send the ping message... So, use an
+ // application specific map to have the client post somewhere else.
+ //
+ // To top this all off, each of these needs to be done with a ShmEventLoop,
+ // which needs to run in a separate thread... And it is really hard to get
+ // everything started up reliably. So just be super generous on timeouts and
+ // hope for the best. We can be more generous in the future if we need to.
+ //
+ // We are faking the application names by passing in --application_name=foo
+ OnPi1();
+
+ MakePi1Server(
+ "dummy sha256 ");
+ MakePi1Client();
+
+ // And build the app for testing.
+ MakePi1Test();
+ aos::Fetcher<ServerStatistics> pi1_server_statistics_fetcher =
+ pi1_test_event_loop->MakeFetcher<ServerStatistics>("/pi1/aos");
+ aos::Fetcher<ClientStatistics> pi1_client_statistics_fetcher =
+ pi1_test_event_loop->MakeFetcher<ClientStatistics>("/pi1/aos");
+
+ // Now do it for "raspberrypi2", the client.
+ OnPi2();
+ MakePi2Server();
+
+ // And build the app for testing.
+ MakePi2Test();
+ aos::Fetcher<ServerStatistics> pi2_server_statistics_fetcher =
+ pi2_test_event_loop->MakeFetcher<ServerStatistics>("/pi2/aos");
+ aos::Fetcher<ClientStatistics> pi2_client_statistics_fetcher =
+ pi2_test_event_loop->MakeFetcher<ClientStatistics>("/pi2/aos");
+
+ // Wait until we are connected, then send.
+
+ StartPi1Test();
+ StartPi2Test();
+ StartPi1Server();
+ StartPi1Client();
+ StartPi2Server();
+
+ {
+ MakePi2Client();
+
+ RunPi2Client(chrono::milliseconds(3050));
+
+ // Now confirm we are synchronized.
+ EXPECT_TRUE(pi1_server_statistics_fetcher.Fetch());
+ EXPECT_TRUE(pi1_client_statistics_fetcher.Fetch());
+ EXPECT_TRUE(pi2_server_statistics_fetcher.Fetch());
+ EXPECT_TRUE(pi2_client_statistics_fetcher.Fetch());
+
+ const ServerConnection *const pi1_connection =
+ pi1_server_statistics_fetcher->connections()->Get(0);
+ const ClientConnection *const pi1_client_connection =
+ pi1_client_statistics_fetcher->connections()->Get(0);
+ const ServerConnection *const pi2_connection =
+ pi2_server_statistics_fetcher->connections()->Get(0);
+ const ClientConnection *const pi2_client_connection =
+ pi2_client_statistics_fetcher->connections()->Get(0);
+
+ // Make sure one direction is disconnected with a bunch of connection
+ // attempts and failures.
+ EXPECT_EQ(pi1_connection->state(), State::DISCONNECTED);
+ EXPECT_EQ(pi1_connection->connection_count(), 0u);
+ EXPECT_GT(pi1_connection->invalid_connection_count(), 10u);
+
+ EXPECT_EQ(pi2_client_connection->state(), State::DISCONNECTED);
+ EXPECT_GT(pi2_client_connection->connection_count(), 10u);
+
+ // And the other direction is happy.
+ EXPECT_EQ(pi2_connection->state(), State::CONNECTED);
+ EXPECT_EQ(pi2_connection->connection_count(), 1u);
+ EXPECT_TRUE(pi2_connection->has_connected_since_time());
+ EXPECT_FALSE(pi2_connection->has_monotonic_offset());
+ EXPECT_TRUE(pi2_connection->has_boot_uuid());
+
+ EXPECT_EQ(pi1_client_connection->state(), State::CONNECTED);
+ EXPECT_EQ(pi1_client_connection->connection_count(), 1u);
+
+ VLOG(1) << aos::FlatbufferToJson(pi2_server_statistics_fetcher.get());
+ VLOG(1) << aos::FlatbufferToJson(pi1_server_statistics_fetcher.get());
+ VLOG(1) << aos::FlatbufferToJson(pi2_client_statistics_fetcher.get());
+ VLOG(1) << aos::FlatbufferToJson(pi1_client_statistics_fetcher.get());
+
+ StopPi2Client();
+ }
+
+ // Shut everyone else down
+ StopPi1Server();
+ StopPi1Client();
+ StopPi2Server();
+ StopPi1Test();
+ StopPi2Test();
+}
+
// Test that a client which connects with too big a message gets disconnected
// without crashing.
TEST_P(MessageBridgeParameterizedTest, TooBigConnect) {
@@ -1454,8 +1557,8 @@
const ServerConnection *const pi2_connection =
pi2_server_statistics_fetcher->connections()->Get(0);
- // Make sure one direction is disconnected with a bunch of connection
- // attempts and failures.
+ // Make sure the server we just sent a bunch of junk to is grumpy and
+ // disconnected the bad client.
EXPECT_EQ(pi1_connection->state(), State::DISCONNECTED);
EXPECT_EQ(pi1_connection->connection_count(), 0u);
EXPECT_GE(pi1_server_statistics_fetcher->invalid_connection_count(), 1u);
diff --git a/aos/network/sctp_lib.cc b/aos/network/sctp_lib.cc
index d0924d5..2658271 100644
--- a/aos/network/sctp_lib.cc
+++ b/aos/network/sctp_lib.cc
@@ -445,11 +445,6 @@
return result;
}
- CHECK_LE(size, static_cast<ssize_t>(max_read_size_))
- << ": Message overflowed buffer on stream "
- << result->header.rcvinfo.rcv_sid << "."
- << " Check for config mismatch or rogue device.";
-
if (result->message_type == Message::kNotification) {
// Notifications are never fragmented, just return it now.
CHECK(inmessage.msg_flags & MSG_EOR)