From 972f241bf5f9e10b6dd1b9f5652cfdfe3b2158f0 Mon Sep 17 00:00:00 2001 From: Hyun Jae Moon Date: Fri, 23 Feb 2024 20:40:19 +0000 Subject: Replace HttpResponse struct with http::Response Deleted http_response.rs and the necessary methods are defined in server_response.rs Test: scripts/cmake_setup.py && ninja -C objs Bug: 293512425 Change-Id: Idc2d007bdd180ae081fafa1ab19bc8fe58f13083 --- rust/daemon/src/http_server/http_handlers.rs | 2 +- rust/daemon/src/http_server/http_response.rs | 98 ------------------- rust/daemon/src/http_server/mod.rs | 1 - rust/daemon/src/http_server/server_response.rs | 124 +++++++++++++++++++++---- 4 files changed, 106 insertions(+), 119 deletions(-) delete mode 100644 rust/daemon/src/http_server/http_response.rs diff --git a/rust/daemon/src/http_server/http_handlers.rs b/rust/daemon/src/http_server/http_handlers.rs index ec8991a..fa0aebe 100644 --- a/rust/daemon/src/http_server/http_handlers.rs +++ b/rust/daemon/src/http_server/http_handlers.rs @@ -179,7 +179,7 @@ pub fn handle_connection(mut stream: TcpStream, valid_files: Arc router.handle_request(&request, &mut response_writer); if let Some(response) = response_writer.get_response() { // Status code of 101 represents switching of protocols from HTTP to Websocket - if response.status_code == 101 { + if response.status().as_u16() == 101 { match collect_query(request.uri().query().unwrap_or("")) { Ok(queries) => run_websocket_transport(stream, queries), Err(err) => warn!("{err}"), diff --git a/rust/daemon/src/http_server/http_response.rs b/rust/daemon/src/http_server/http_response.rs deleted file mode 100644 index b71bad3..0000000 --- a/rust/daemon/src/http_server/http_response.rs +++ /dev/null @@ -1,98 +0,0 @@ -// Copyright 2023 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Response module for micro HTTP server. -//! -//! This library implements the basic parts of Response Message from -//! (RFC 5322)[ https://www.rfc-editor.org/rfc/rfc5322.html] "HTTP -//! Message Format." -//! -//! This library is only used for serving the netsim client and is not -//! meant to implement all aspects of RFC 5322. - -use std::str::FromStr; - -use http::{HeaderMap, HeaderName, HeaderValue}; - -use super::server_response::StrHeaders; - -pub struct HttpResponse { - pub status_code: u16, - pub headers: HeaderMap, - pub body: Vec, -} - -impl HttpResponse { - pub fn new_ok_with_length(content_type: &str, length: usize) -> HttpResponse { - let body = Vec::new(); - let mut headers = HeaderMap::new(); - headers.insert("Content-Type", HeaderValue::from_str(content_type).unwrap()); - headers - .insert("Content-Length", HeaderValue::from_str(length.to_string().as_str()).unwrap()); - HttpResponse { status_code: 200, headers, body } - } - - pub fn new_ok(content_type: &str, body: Vec) -> HttpResponse { - let mut headers = HeaderMap::new(); - headers.insert("Content-Type", HeaderValue::from_str(content_type).unwrap()); - headers.insert( - "Content-Length", - HeaderValue::from_str(body.len().to_string().as_str()).unwrap(), - ); - HttpResponse { status_code: 200, headers, body } - } - - pub fn new_ok_switch_protocol(connection: &str) -> HttpResponse { - let mut headers = HeaderMap::new(); - headers.insert("Upgrade", HeaderValue::from_str(connection).unwrap()); - headers.insert("Connection", HeaderValue::from_static("Upgrade")); - HttpResponse { status_code: 101, headers, body: Vec::new() } - } - - pub fn new_error(status_code: u16, body: Vec) -> HttpResponse { - let mut headers = HeaderMap::new(); - headers.insert("Content-Type", HeaderValue::from_static("text/plain")); - headers.insert( - "Content-Length", - HeaderValue::from_str(body.len().to_string().as_str()).unwrap(), - ); - HttpResponse { status_code, headers, body } - } - - pub fn add_headers(&mut self, headers: StrHeaders) { - for (key, value) in headers { - self.headers.insert( - HeaderName::from_str(key.as_str()).unwrap(), - HeaderValue::from_str(value.as_str()).unwrap(), - ); - } - } -} - -#[cfg(test)] -mod tests { - use crate::http_server::server_response::{ServerResponseWritable, ServerResponseWriter}; - use std::io::Cursor; - - #[test] - fn test_write_to() { - let mut stream = Cursor::new(Vec::new()); - let mut writer = ServerResponseWriter::new(&mut stream); - writer.put_ok_with_vec("text/plain", b"Hello World".to_vec(), vec![]); - let written_bytes = stream.get_ref(); - let expected_bytes = - b"HTTP/1.1 200 OK\r\ncontent-type: text/plain\r\ncontent-length: 11\r\n\r\nHello World"; - assert_eq!(written_bytes, expected_bytes); - } -} diff --git a/rust/daemon/src/http_server/mod.rs b/rust/daemon/src/http_server/mod.rs index 1f91a7e..4913f7c 100644 --- a/rust/daemon/src/http_server/mod.rs +++ b/rust/daemon/src/http_server/mod.rs @@ -14,7 +14,6 @@ mod http_handlers; pub(crate) mod http_request; -mod http_response; mod http_router; pub(crate) mod server; pub(crate) mod server_response; diff --git a/rust/daemon/src/http_server/server_response.rs b/rust/daemon/src/http_server/server_response.rs index c9f849e..020f265 100644 --- a/rust/daemon/src/http_server/server_response.rs +++ b/rust/daemon/src/http_server/server_response.rs @@ -23,11 +23,11 @@ //! This library is intended solely for serving netsim clients. use std::io::Write; +use std::str::FromStr; +use http::{HeaderName, HeaderValue, Response}; use log::error; -use crate::http_server::http_response::HttpResponse; - pub type ResponseWritable<'a> = &'a mut dyn ServerResponseWritable; pub type StrHeaders = Vec<(String, String)>; @@ -46,33 +46,34 @@ pub trait ServerResponseWritable { // A response writer that can contain a TCP stream or other writable. pub struct ServerResponseWriter<'a> { writer: &'a mut dyn Write, - response: Option, + response: Option>>, } impl<'a> ServerResponseWriter<'a> { pub fn new(writer: &mut W) -> ServerResponseWriter { ServerResponseWriter { writer, response: None } } - pub fn put_response(&mut self, response: HttpResponse) { - let reason = match response.status_code { + pub fn put_response(&mut self, response: Response>) { + let reason = match response.status().as_u16() { 101 => "Switching Protocols", 200 => "OK", 404 => "Not Found", _ => "Unknown Reason", }; - let mut buffer = format!("HTTP/1.1 {} {}\r\n", response.status_code, reason).into_bytes(); - for (name, value) in response.headers.iter() { + let mut buffer = + format!("HTTP/1.1 {} {}\r\n", response.status().as_str(), reason).into_bytes(); + for (name, value) in response.headers() { buffer .extend_from_slice(format!("{}: {}\r\n", name, value.to_str().unwrap()).as_bytes()); } buffer.extend_from_slice(b"\r\n"); - buffer.extend_from_slice(&response.body); + buffer.extend_from_slice(response.body()); if let Err(e) = self.writer.write_all(&buffer) { error!("handle_connection error {e}"); }; self.response = Some(response); } - pub fn get_response(self) -> Option { + pub fn get_response(self) -> Option>> { self.response } } @@ -82,8 +83,18 @@ impl<'a> ServerResponseWriter<'a> { // by the handler methods. impl ServerResponseWritable for ServerResponseWriter<'_> { fn put_error(&mut self, error_code: u16, error_message: &str) { - let response = HttpResponse::new_error(error_code, error_message.into()); - self.put_response(response); + let body = error_message.as_bytes().to_vec(); + self.put_response( + Response::builder() + .status(error_code) + .header("Content-Type", HeaderValue::from_static("text/plain")) + .header( + "Content-Length", + HeaderValue::from_str(body.len().to_string().as_str()).unwrap(), + ) + .body(body) + .unwrap(), + ); } fn put_chunk(&mut self, chunk: &[u8]) { if let Err(e) = self.writer.write_all(chunk) { @@ -92,32 +103,62 @@ impl ServerResponseWritable for ServerResponseWriter<'_> { self.writer.flush().unwrap(); } fn put_ok_with_length(&mut self, mime_type: &str, length: usize, headers: StrHeaders) { - let mut response = HttpResponse::new_ok_with_length(mime_type, length); - response.add_headers(headers); + let mut response = Response::builder() + .status(200) + .header("Content-Type", HeaderValue::from_str(mime_type).unwrap()) + .header("Content-Length", HeaderValue::from_str(length.to_string().as_str()).unwrap()) + .body(Vec::new()) + .unwrap(); + add_headers(&mut response, headers); self.put_response(response); } fn put_ok(&mut self, mime_type: &str, body: &str, headers: StrHeaders) { - let mut response = HttpResponse::new_ok(mime_type, body.into()); - response.add_headers(headers); + let mut response = new_ok(mime_type, body.into()); + add_headers(&mut response, headers); self.put_response(response); } fn put_ok_with_vec(&mut self, mime_type: &str, body: Vec, headers: StrHeaders) { - let mut response = HttpResponse::new_ok(mime_type, body); - response.add_headers(headers); + let mut response = new_ok(mime_type, body); + add_headers(&mut response, headers); self.put_response(response); } fn put_ok_switch_protocol(&mut self, connection: &str, headers: StrHeaders) { - let mut response = HttpResponse::new_ok_switch_protocol(connection); - response.add_headers(headers); + let mut response = Response::builder() + .status(101) + .header("Upgrade", HeaderValue::from_str(connection).unwrap()) + .header("Connection", HeaderValue::from_static("Upgrade")) + .body(Vec::new()) + .unwrap(); + add_headers(&mut response, headers); self.put_response(response); } } +fn new_ok(content_type: &str, body: Vec) -> Response> { + Response::builder() + .status(200) + .header("Content-Type", HeaderValue::from_str(content_type).unwrap()) + .header("Content-Length", HeaderValue::from_str(body.len().to_string().as_str()).unwrap()) + .body(body) + .unwrap() +} + +fn add_headers(response: &mut Response>, headers: StrHeaders) { + for (key, value) in headers { + response.headers_mut().insert( + HeaderName::from_str(key.as_str()).unwrap(), + HeaderValue::from_str(value.as_str()).unwrap(), + ); + } +} + #[cfg(test)] mod tests { use super::*; use std::io::Cursor; + const SAMPLE_CHUNK: &[u8] = &[0, 1, 2, 3, 4]; + #[test] fn test_put_error() { let mut stream = Cursor::new(Vec::new()); @@ -139,4 +180,49 @@ mod tests { b"HTTP/1.1 200 OK\r\ncontent-type: text/plain\r\ncontent-length: 11\r\n\r\nHello World"; assert_eq!(written_bytes, expected_bytes); } + + #[test] + fn test_put_ok_with_length() { + let mut stream = Cursor::new(Vec::new()); + let mut writer = ServerResponseWriter::new(&mut stream); + writer.put_ok_with_length("text/plain", 100, vec![]); + let written_bytes = stream.get_ref(); + let expected_bytes = + b"HTTP/1.1 200 OK\r\ncontent-type: text/plain\r\ncontent-length: 100\r\n\r\n"; + assert_eq!(written_bytes, expected_bytes); + } + + #[test] + fn test_put_ok_with_vec() { + let mut stream = Cursor::new(Vec::new()); + let mut writer = ServerResponseWriter::new(&mut stream); + writer.put_ok_with_vec("text/plain", SAMPLE_CHUNK.to_vec(), vec![]); + let written_bytes = stream.get_ref(); + let expected_bytes = &[ + b"HTTP/1.1 200 OK\r\ncontent-type: text/plain\r\ncontent-length: 5\r\n\r\n".to_vec(), + SAMPLE_CHUNK.to_vec(), + ] + .concat(); + assert_eq!(written_bytes, expected_bytes); + } + + #[test] + fn test_put_ok_switch_protocol() { + let mut stream = Cursor::new(Vec::new()); + let mut writer = ServerResponseWriter::new(&mut stream); + writer.put_ok_switch_protocol("Websocket", vec![]); + let written_bytes = stream.get_ref(); + let expected_bytes = b"HTTP/1.1 101 Switching Protocols\r\nupgrade: Websocket\r\nconnection: Upgrade\r\n\r\n"; + assert_eq!(written_bytes, expected_bytes); + } + + #[test] + fn test_put_chunk() { + let mut stream = Cursor::new(Vec::new()); + let mut writer = ServerResponseWriter::new(&mut stream); + writer.put_chunk(SAMPLE_CHUNK); + let written_bytes = stream.get_ref(); + let expected_bytes = SAMPLE_CHUNK; + assert_eq!(written_bytes, expected_bytes); + } } -- cgit v1.2.3 From a5e3e65af963bdf042f1f7f907276ab80eb45025 Mon Sep 17 00:00:00 2001 From: James Farrell Date: Wed, 28 Feb 2024 21:32:17 +0000 Subject: Fix style warning for rustc 1.76.0 Test: Built with test_compiler.py Change-Id: I958993ac500c8834293d109326c025b22f39e403 Bug: 327204642 --- rust/cli/src/args.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/cli/src/args.rs b/rust/cli/src/args.rs index 577372a..b253df3 100644 --- a/rust/cli/src/args.rs +++ b/rust/cli/src/args.rs @@ -281,7 +281,7 @@ impl Command { fn get_filtered_captures( client: &cxx::UniquePtr, - patterns: &Vec, + patterns: &[String], ) -> Vec { // Get list of captures let result = client.send_grpc(&GrpcMethod::ListCapture, &Vec::new()); -- cgit v1.2.3 From 5c24785f5925662715eb13670fc36ba0865a10f9 Mon Sep 17 00:00:00 2001 From: Hyun Jae Moon Date: Wed, 21 Feb 2024 22:47:11 +0000 Subject: Register ReportInvalidPacket handler into rootcanal Bifurcated the Vec from SharedEmualtedChip to avoid deadlocks. Test: scripts/cmake_setup.py && ninja -C objs Bug: 323226412 Change-Id: I2bc57f758295aa0d0b117410a0e56632c2f15db4 --- rust/daemon/src/echip/bluetooth.rs | 66 ++++++++++++++++------------------ rust/daemon/src/echip/emulated_chip.rs | 13 +------ src/hci/bluetooth_facade.cc | 11 ++++-- 3 files changed, 40 insertions(+), 50 deletions(-) diff --git a/rust/daemon/src/echip/bluetooth.rs b/rust/daemon/src/echip/bluetooth.rs index f9228f5..218ba15 100644 --- a/rust/daemon/src/echip/bluetooth.rs +++ b/rust/daemon/src/echip/bluetooth.rs @@ -37,12 +37,12 @@ static ECHIP_BT_MUTEX: Mutex<()> = Mutex::new(()); pub type RootcanalIdentifier = u32; -// BLUETOOTH_ECHIPS is a singleton that contains a hash map from -// RootcanalIdentifier to SharedEmulatedChip +// BLUETOOTH_INVALID_PACKETS is a singleton that contains a hash map from +// RootcanalIdentifier to Vec // This singleton is only used when Rootcanal reports invalid // packets by rootcanal_id and we add those to Bluetooth struct. lazy_static! { - static ref BLUETOOTH_ECHIPS: Arc>> = + static ref BLUETOOTH_INVALID_PACKETS: Arc>>> = Arc::new(Mutex::new(BTreeMap::new())); } @@ -59,7 +59,6 @@ pub struct Bluetooth { rootcanal_id: RootcanalIdentifier, low_energy_enabled: ProtoState, classic_enabled: ProtoState, - invalid_packets: Vec, } fn patch_state( @@ -135,14 +134,21 @@ impl EmulatedChip for Bluetooth { // Lock to protect id_to_chip_info_ table in C++ let _unused = ECHIP_BT_MUTEX.lock().expect("Failed to acquire lock on ECHIP_BT_MUTEX"); ffi_bluetooth::bluetooth_remove(self.rootcanal_id); + BLUETOOTH_INVALID_PACKETS.lock().expect("invalid packets").remove(&self.rootcanal_id); } fn get_stats(&self, duration_secs: u64) -> Vec { // Construct NetsimRadioStats for BLE and Classic. let mut ble_stats_proto = ProtoRadioStats::new(); ble_stats_proto.set_duration_secs(duration_secs); - for invalid_packet in &self.invalid_packets { - ble_stats_proto.invalid_packets.push(invalid_packet.clone()); + if let Some(v) = BLUETOOTH_INVALID_PACKETS + .lock() + .expect("Failed to acquire lock on BLUETOOTH_INVALID_PACKETS") + .get(&self.rootcanal_id) + { + for invalid_packet in v { + ble_stats_proto.invalid_packets.push(invalid_packet.clone()); + } } let mut classic_stats_proto = ble_stats_proto.clone(); @@ -160,24 +166,6 @@ impl EmulatedChip for Bluetooth { } vec![ble_stats_proto, classic_stats_proto] } - - fn report_invalid_packet( - &mut self, - reason: InvalidPacketReason, - description: String, - packet: Vec, - ) { - // Remove the earliest reported packet if length greater than 5 - if self.invalid_packets.len() >= 5 { - self.invalid_packets.remove(0); - } - // append error packet - let mut invalid_packet = InvalidPacket::new(); - invalid_packet.set_reason(reason); - invalid_packet.set_description(description); - invalid_packet.set_packet(packet); - self.invalid_packets.push(invalid_packet); - } } /// Create a new Emulated Bluetooth Chip @@ -197,11 +185,9 @@ pub fn new(create_params: &CreateParams, chip_id: ChipIdentifier) -> SharedEmula rootcanal_id, low_energy_enabled: ProtoState::ON, classic_enabled: ProtoState::ON, - invalid_packets: Vec::new(), }; - let shared_echip = SharedEmulatedChip(Arc::new(Mutex::new(Box::new(echip)))); - BLUETOOTH_ECHIPS.lock().unwrap().insert(rootcanal_id, shared_echip.clone()); - shared_echip + BLUETOOTH_INVALID_PACKETS.lock().expect("invalid packets").insert(rootcanal_id, Vec::new()); + SharedEmulatedChip(Arc::new(Mutex::new(Box::new(echip)))) } /// Starts the Bluetooth service. @@ -222,13 +208,23 @@ pub fn report_invalid_packet_cxx( description: &CxxString, packet: &CxxVector, ) { - let reason_enum = InvalidPacketReason::from_i32(reason).unwrap_or(InvalidPacketReason::UNKNOWN); - match BLUETOOTH_ECHIPS.lock().unwrap().get(&rootcanal_id) { - Some(echip) => echip.lock().report_invalid_packet( - reason_enum, - description.to_string(), - packet.as_slice().to_vec(), - ), + match BLUETOOTH_INVALID_PACKETS.lock().unwrap().get_mut(&rootcanal_id) { + Some(v) => { + // Remove the earliest reported packet if length greater than 5 + if v.len() >= 5 { + v.remove(0); + } + // append error packet + let mut invalid_packet = InvalidPacket::new(); + invalid_packet.set_reason( + InvalidPacketReason::from_i32(reason).unwrap_or(InvalidPacketReason::UNKNOWN), + ); + invalid_packet.set_description(description.to_string()); + invalid_packet.set_packet(packet.as_slice().to_vec()); + v.push(invalid_packet); + // Log the report + info!("Reported Invalid Packet for Bluetooth EmulatedChip with rootcanal_id: {rootcanal_id}, reason:{reason}, description: {description:?}, packet: {packet:?}"); + } None => error!("Bluetooth EmulatedChip not created for rootcanal_id: {rootcanal_id}"), } } diff --git a/rust/daemon/src/echip/emulated_chip.rs b/rust/daemon/src/echip/emulated_chip.rs index 9548715..2a0d39c 100644 --- a/rust/daemon/src/echip/emulated_chip.rs +++ b/rust/daemon/src/echip/emulated_chip.rs @@ -19,10 +19,8 @@ use std::{ use lazy_static::lazy_static; +use netsim_proto::model::Chip as ProtoChip; use netsim_proto::stats::NetsimRadioStats as ProtoRadioStats; -use netsim_proto::{ - model::Chip as ProtoChip, stats::invalid_packet::Reason as InvalidPacketReason, -}; use crate::{ devices::chip::ChipIdentifier, @@ -96,15 +94,6 @@ pub trait EmulatedChip { /// Return the NetsimRadioStats protobuf from the emulated chip. This is /// part of NetsimStats protobuf. fn get_stats(&self, duration_secs: u64) -> Vec; - - /// Optional method: Add error_packet bytes into NetsimRadioStats - fn report_invalid_packet( - &mut self, - _reason: InvalidPacketReason, - _description: String, - _packet: Vec, - ) { - } } /// Lookup for SharedEmulatedChip with chip_id diff --git a/src/hci/bluetooth_facade.cc b/src/hci/bluetooth_facade.cc index 2838afe..c9fdc7a 100644 --- a/src/hci/bluetooth_facade.cc +++ b/src/hci/bluetooth_facade.cc @@ -200,9 +200,6 @@ void Start(const rust::Slice<::std::uint8_t const> proto_bytes, // output is to a file, so no color wanted rootcanal::log::SetLogColorEnable(false); - // TODO(b/323226412): Pass netsim::hci::facade::ReportInvalidPacket signature - // into rootcanal - config::Bluetooth config; config.ParseFromArray(proto_bytes.data(), proto_bytes.size()); controller_proto_ = std::make_shared( @@ -376,6 +373,14 @@ uint32_t Add(uint32_t chip_id, const std::string &address_string, auto hci_device = std::make_shared(transport, *controller_properties); + // Pass netsim::hci::facade::ReportInvalidPacket signature into hci_device + hci_device->RegisterInvalidPacketHandler( + [](uint32_t rootcanal_id, rootcanal::InvalidPacketReason reason, + std::string description, const std::vector &packet) { + netsim::hci::facade::ReportInvalidPacket( + rootcanal_id, static_cast(reason), description, packet); + }); + // Use the `AsyncManager` to ensure that the `AddHciConnection` method is // invoked atomically, preventing data races. std::promise rootcanal_id_promise; -- cgit v1.2.3