From fe85901021df14596f22fdeaa5c57c38e8123c37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Frosteg=C3=A5rd?= Date: Fri, 10 Apr 2020 15:14:50 +0200 Subject: [PATCH] bittorrent_udp: add RequestParseError; remove InvalidRequest; other fixes --- TODO.md | 1 - aquatic/src/lib/network.rs | 27 ++-- aquatic_bench/src/bin/bench_handlers/main.rs | 14 +- bittorrent_udp/src/converters/requests.rs | 135 ++++++++++++------- bittorrent_udp/src/types/request.rs | 7 - 5 files changed, 114 insertions(+), 70 deletions(-) diff --git a/TODO.md b/TODO.md index f8c46cb..ce4c49f 100644 --- a/TODO.md +++ b/TODO.md @@ -11,7 +11,6 @@ * quickcheck request byte conversion * other test cases * Check if announce response to bytes code changed caused slowdown - * thiserror errors instead of InvalidRequest ## Not important diff --git a/aquatic/src/lib/network.rs b/aquatic/src/lib/network.rs index 468372f..264fd76 100644 --- a/aquatic/src/lib/network.rs +++ b/aquatic/src/lib/network.rs @@ -151,16 +151,27 @@ fn handle_readable_socket( Ok(Request::Scrape(r)) => { scrape_requests.push((r, src)); }, - Ok(Request::Invalid(r)) => { - let response = Response::Error(ErrorResponse { - transaction_id: r.transaction_id, - message: "Invalid request".to_string(), - }); - - responses.push((response, src)); - }, Err(err) => { eprintln!("request_from_bytes error: {:?}", err); + + if let Some(transaction_id) = err.transaction_id { + let opt_message = if err.error.is_some() { + Some("Parse error".to_string()) + } else if let Some(message) = err.message { + Some(message) + } else { + None + }; + + if let Some(message) = opt_message { + let response = ErrorResponse { + transaction_id, + message, + }; + + responses.push((response.into(), src)); + } + } }, } }, diff --git a/aquatic_bench/src/bin/bench_handlers/main.rs b/aquatic_bench/src/bin/bench_handlers/main.rs index 302fea7..684229e 100644 --- a/aquatic_bench/src/bin/bench_handlers/main.rs +++ b/aquatic_bench/src/bin/bench_handlers/main.rs @@ -2,11 +2,11 @@ //! //! Example summary output: //! ``` -//! ## Average results over 50 rounds +//! ## Average results over 20 rounds //! -//! Connect handler: 2 530 072 requests/second, 395.38 ns/request -//! Announce handler: 309 719 requests/second, 3229.87 ns/request -//! Scrape handler: 595 259 requests/second, 1680.01 ns/request +//! Connect handler: 2 473 860 requests/second, 404.94 ns/request +//! Announce handler: 302 665 requests/second, 3306.17 ns/request +//! Scrape handler: 745 598 requests/second, 1341.30 ns/request //! ``` use std::time::{Duration, Instant}; @@ -77,7 +77,7 @@ fn main(){ let mut buffer = [0u8; MAX_REQUEST_BYTES]; let mut cursor = Cursor::new(buffer.as_mut()); - request_to_bytes(&mut cursor, Request::Connect(request)); + request_to_bytes(&mut cursor, Request::Connect(request)).unwrap(); (buffer, src) }) @@ -132,7 +132,7 @@ fn main(){ let mut buffer = [0u8; MAX_REQUEST_BYTES]; let mut cursor = Cursor::new(buffer.as_mut()); - request_to_bytes(&mut cursor, Request::Announce(request)); + request_to_bytes(&mut cursor, Request::Announce(request)).unwrap(); (buffer, src) }) @@ -186,7 +186,7 @@ fn main(){ let mut buffer = [0u8; MAX_REQUEST_BYTES]; let mut cursor = Cursor::new(buffer.as_mut()); - request_to_bytes(&mut cursor, Request::Scrape(request)); + request_to_bytes(&mut cursor, Request::Scrape(request)).unwrap(); (buffer, src) }) diff --git a/bittorrent_udp/src/converters/requests.rs b/bittorrent_udp/src/converters/requests.rs index 0dea780..a6f40bd 100644 --- a/bittorrent_udp/src/converters/requests.rs +++ b/bittorrent_udp/src/converters/requests.rs @@ -12,54 +12,87 @@ use super::common::*; const PROTOCOL_IDENTIFIER: i64 = 4_497_486_125_440; +#[derive(Debug)] +pub struct RequestParseError { + pub transaction_id: Option, + pub message: Option, + pub error: Option, +} + + +impl RequestParseError { + pub fn new(err: io::Error, transaction_id: i32) -> Self { + Self { + transaction_id: Some(TransactionId(transaction_id)), + message: None, + error: Some(err) + } + } + pub fn io(err: io::Error) -> Self { + Self { + transaction_id: None, + message: None, + error: Some(err) + } + } + pub fn text(transaction_id: i32, message: &str) -> Self { + Self { + transaction_id: Some(TransactionId(transaction_id)), + message: Some(message.to_string()), + error: None, + } + } +} + + #[inline] pub fn request_to_bytes( bytes: &mut impl Write, request: Request -){ +) -> Result<(), io::Error> { match request { Request::Connect(r) => { - bytes.write_i64::(PROTOCOL_IDENTIFIER).unwrap(); - bytes.write_i32::(0).unwrap(); - bytes.write_i32::(r.transaction_id.0).unwrap(); + bytes.write_i64::(PROTOCOL_IDENTIFIER)?; + bytes.write_i32::(0)?; + bytes.write_i32::(r.transaction_id.0)?; }, Request::Announce(r) => { - bytes.write_i64::(r.connection_id.0).unwrap(); - bytes.write_i32::(1).unwrap(); - bytes.write_i32::(r.transaction_id.0).unwrap(); + bytes.write_i64::(r.connection_id.0)?; + bytes.write_i32::(1)?; + bytes.write_i32::(r.transaction_id.0)?; - bytes.write_all(&r.info_hash.0).unwrap(); - bytes.write_all(&r.peer_id.0).unwrap(); + bytes.write_all(&r.info_hash.0)?; + bytes.write_all(&r.peer_id.0)?; - bytes.write_i64::(r.bytes_downloaded.0).unwrap(); - bytes.write_i64::(r.bytes_left.0).unwrap(); - bytes.write_i64::(r.bytes_uploaded.0).unwrap(); + bytes.write_i64::(r.bytes_downloaded.0)?; + bytes.write_i64::(r.bytes_left.0)?; + bytes.write_i64::(r.bytes_uploaded.0)?; - bytes.write_i32::(event_to_i32(r.event)).unwrap(); + bytes.write_i32::(event_to_i32(r.event))?; bytes.write_all(&r.ip_address.map_or( [0; 4], |ip| ip.octets() - )).unwrap(); + ))?; - bytes.write_u32::(r.key.0).unwrap(); - bytes.write_i32::(r.peers_wanted.0).unwrap(); - bytes.write_u16::(r.port.0).unwrap(); + bytes.write_u32::(r.key.0)?; + bytes.write_i32::(r.peers_wanted.0)?; + bytes.write_u16::(r.port.0)?; }, Request::Scrape(r) => { - bytes.write_i64::(r.connection_id.0).unwrap(); - bytes.write_i32::(2).unwrap(); - bytes.write_i32::(r.transaction_id.0).unwrap(); + bytes.write_i64::(r.connection_id.0)?; + bytes.write_i32::(2)?; + bytes.write_i32::(r.transaction_id.0)?; for info_hash in r.info_hashes { - bytes.write_all(&info_hash.0).unwrap(); + bytes.write_all(&info_hash.0)?; } } - - _ => () // Invalid requests should never happen } + + Ok(()) } @@ -67,12 +100,15 @@ pub fn request_to_bytes( pub fn request_from_bytes( bytes: &[u8], max_scrape_torrents: u8, -) -> Result { +) -> Result { let mut cursor = Cursor::new(bytes); - let connection_id = cursor.read_i64::()?; - let action = cursor.read_i32::()?; - let transaction_id = cursor.read_i32::()?; + let connection_id = cursor.read_i64::() + .map_err(RequestParseError::io)?; + let action = cursor.read_i32::() + .map_err(RequestParseError::io)?; + let transaction_id = cursor.read_i32::() + .map_err(RequestParseError::io)?; match action { // Connect @@ -82,12 +118,10 @@ pub fn request_from_bytes( transaction_id: TransactionId(transaction_id) }).into()) } else { - Ok(Request::Invalid(InvalidRequest { - transaction_id: TransactionId(transaction_id), - message: - "Please send protocol identifier in connect request" - .to_string() - })) + Err(RequestParseError::text( + transaction_id, + "Protocol identifier missing" + )) } }, @@ -97,19 +131,29 @@ pub fn request_from_bytes( let mut peer_id = [0; 20]; let mut ip = [0; 4]; - cursor.read_exact(&mut info_hash)?; - cursor.read_exact(&mut peer_id)?; + cursor.read_exact(&mut info_hash) + .map_err(|err| RequestParseError::new(err, transaction_id))?; + cursor.read_exact(&mut peer_id) + .map_err(|err| RequestParseError::new(err, transaction_id))?; - let bytes_downloaded = cursor.read_i64::()?; - let bytes_left = cursor.read_i64::()?; - let bytes_uploaded = cursor.read_i64::()?; - let event = cursor.read_i32::()?; + let bytes_downloaded = cursor.read_i64::() + .map_err(|err| RequestParseError::new(err, transaction_id))?; + let bytes_left = cursor.read_i64::() + .map_err(|err| RequestParseError::new(err, transaction_id))?; + let bytes_uploaded = cursor.read_i64::() + .map_err(|err| RequestParseError::new(err, transaction_id))?; + let event = cursor.read_i32::() + .map_err(|err| RequestParseError::new(err, transaction_id))?; - cursor.read_exact(&mut ip)?; + cursor.read_exact(&mut ip) + .map_err(|err| RequestParseError::new(err, transaction_id))?; - let key = cursor.read_u32::()?; - let peers_wanted = cursor.read_i32::()?; - let port = cursor.read_u16::()?; + let key = cursor.read_u32::() + .map_err(|err| RequestParseError::new(err, transaction_id))?; + let peers_wanted = cursor.read_i32::() + .map_err(|err| RequestParseError::new(err, transaction_id))?; + let port = cursor.read_u16::() + .map_err(|err| RequestParseError::new(err, transaction_id))?; let opt_ip = if ip == [0; 4] { None @@ -150,9 +194,6 @@ pub fn request_from_bytes( }).into()) } - _ => Ok(Request::Invalid(InvalidRequest { - transaction_id: TransactionId(transaction_id), - message: "Invalid action".to_string() - })) + _ => Err(RequestParseError::text(transaction_id, "Invalid action")) } } diff --git a/bittorrent_udp/src/types/request.rs b/bittorrent_udp/src/types/request.rs index 397f282..4297a9c 100644 --- a/bittorrent_udp/src/types/request.rs +++ b/bittorrent_udp/src/types/request.rs @@ -39,19 +39,12 @@ pub struct ScrapeRequest { pub info_hashes: Vec } -/// This is used for returning specific errors from the parser -#[derive(PartialEq, Eq, Clone, Debug)] -pub struct InvalidRequest { - pub transaction_id: TransactionId, - pub message: String -} #[derive(PartialEq, Eq, Clone, Debug)] pub enum Request { Connect(ConnectRequest), Announce(AnnounceRequest), Scrape(ScrapeRequest), - Invalid(InvalidRequest), }