From fe1dabb28e79f5dfdda9165a0746e49339e45116 Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Wed, 16 Nov 2022 12:55:21 +0100 Subject: [PATCH 1/7] core: improve logging of libp2p_core::upgrade::apply --- core/src/upgrade.rs | 21 +++++++++++++++++++++ core/src/upgrade/apply.rs | 39 ++++++++++++++++++++++++++++++--------- 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index de9ef765e16..f8c7a54f218 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -86,6 +86,7 @@ pub use self::{ }; pub use crate::Negotiated; pub use multistream_select::{NegotiatedComplete, NegotiationError, ProtocolError, Version}; +use std::fmt::{Display, Write}; /// Types serving as protocol names. /// @@ -134,6 +135,26 @@ impl> ProtocolName for T { } } +pub struct DisplayProtocolName(N); +impl Display for DisplayProtocolName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for byte in self.0.protocol_name() { + if (b' '..=b'~').contains(byte) { + f.write_char(char::from(*byte))?; + } else { + write!(f, "<{:02X}>", byte)?; + } + } + Ok(()) + } +} + +#[test] +fn protocol_name_display() { + assert_eq!(DisplayProtocolName("hello").to_string(), "hello"); + assert_eq!(DisplayProtocolName("hellö/").to_string(), "hell/"); +} + /// Common trait for upgrades that can be applied on inbound substreams, outbound substreams, /// or both. pub trait UpgradeInfo { diff --git a/core/src/upgrade/apply.rs b/core/src/upgrade/apply.rs index ce506cfeb74..e2c176afcc3 100644 --- a/core/src/upgrade/apply.rs +++ b/core/src/upgrade/apply.rs @@ -18,7 +18,9 @@ // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -use crate::upgrade::{InboundUpgrade, OutboundUpgrade, ProtocolName, UpgradeError}; +use crate::upgrade::{ + DisplayProtocolName, InboundUpgrade, OutboundUpgrade, ProtocolName, UpgradeError, +}; use crate::{connection::ConnectedPoint, Negotiated}; use futures::{future::Either, prelude::*}; use log::debug; @@ -26,6 +28,7 @@ use multistream_select::{self, DialerSelectFuture, ListenerSelectFuture}; use std::{iter, mem, pin::Pin, task::Context, task::Poll}; pub use multistream_select::Version; +use smallvec::SmallVec; // TODO: Still needed? /// Applies an upgrade to the inbound and outbound direction of a connection or substream. @@ -105,6 +108,7 @@ where }, Upgrade { future: Pin>, + name: SmallVec<[u8; 32]>, }, Undefined, } @@ -137,22 +141,30 @@ where return Poll::Pending; } }; + let name = SmallVec::from_slice(info.protocol_name()); self.inner = InboundUpgradeApplyState::Upgrade { future: Box::pin(upgrade.upgrade_inbound(io, info.0)), + name, }; } - InboundUpgradeApplyState::Upgrade { mut future } => { + InboundUpgradeApplyState::Upgrade { mut future, name } => { match Future::poll(Pin::new(&mut future), cx) { Poll::Pending => { - self.inner = InboundUpgradeApplyState::Upgrade { future }; + self.inner = InboundUpgradeApplyState::Upgrade { future, name }; return Poll::Pending; } Poll::Ready(Ok(x)) => { - debug!("Successfully applied negotiated protocol"); + log::trace!( + "Successfully applied negotiated inbound protocol {}", + DisplayProtocolName(name) + ); return Poll::Ready(Ok(x)); } Poll::Ready(Err(e)) => { - debug!("Failed to apply negotiated protocol"); + debug!( + "Failed to apply negotiated inbound protocol {}", + DisplayProtocolName(name) + ); return Poll::Ready(Err(UpgradeError::Apply(e))); } } @@ -185,6 +197,7 @@ where }, Upgrade { future: Pin>, + name: SmallVec<[u8; 32]>, }, Undefined, } @@ -217,22 +230,30 @@ where return Poll::Pending; } }; + let name = SmallVec::from_slice(info.protocol_name()); self.inner = OutboundUpgradeApplyState::Upgrade { future: Box::pin(upgrade.upgrade_outbound(connection, info.0)), + name, }; } - OutboundUpgradeApplyState::Upgrade { mut future } => { + OutboundUpgradeApplyState::Upgrade { mut future, name } => { match Future::poll(Pin::new(&mut future), cx) { Poll::Pending => { - self.inner = OutboundUpgradeApplyState::Upgrade { future }; + self.inner = OutboundUpgradeApplyState::Upgrade { future, name }; return Poll::Pending; } Poll::Ready(Ok(x)) => { - debug!("Successfully applied negotiated protocol"); + log::trace!( + "Successfully applied negotiated outbound protocol {}", + DisplayProtocolName(name) + ); return Poll::Ready(Ok(x)); } Poll::Ready(Err(e)) => { - debug!("Failed to apply negotiated protocol"); + debug!( + "Failed to apply negotiated outbound protocol {}", + DisplayProtocolName(name) + ); return Poll::Ready(Err(UpgradeError::Apply(e))); } } From 18002ff1ae7baaae89e1903cfddab8e0006baab8 Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Thu, 17 Nov 2022 12:32:49 +0100 Subject: [PATCH 2/7] Apply suggestions from code review Co-authored-by: Thomas Eizinger --- core/src/upgrade/apply.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/upgrade/apply.rs b/core/src/upgrade/apply.rs index e2c176afcc3..558c2e8eb29 100644 --- a/core/src/upgrade/apply.rs +++ b/core/src/upgrade/apply.rs @@ -155,14 +155,14 @@ where } Poll::Ready(Ok(x)) => { log::trace!( - "Successfully applied negotiated inbound protocol {}", + "Upgraded inbound stream to {}", DisplayProtocolName(name) ); return Poll::Ready(Ok(x)); } Poll::Ready(Err(e)) => { debug!( - "Failed to apply negotiated inbound protocol {}", + "Failed to upgrade inbound stream to {}", DisplayProtocolName(name) ); return Poll::Ready(Err(UpgradeError::Apply(e))); From 4ccd17c9630d6d5167fa70139ed93a3f0724ab76 Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Thu, 17 Nov 2022 12:36:59 +0100 Subject: [PATCH 3/7] more PR review --- core/src/upgrade.rs | 7 ++++--- core/src/upgrade/apply.rs | 9 +++------ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index f8c7a54f218..ed150277b34 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -86,7 +86,7 @@ pub use self::{ }; pub use crate::Negotiated; pub use multistream_select::{NegotiatedComplete, NegotiationError, ProtocolError, Version}; -use std::fmt::{Display, Write}; +use std::fmt; /// Types serving as protocol names. /// @@ -136,8 +136,9 @@ impl> ProtocolName for T { } pub struct DisplayProtocolName(N); -impl Display for DisplayProtocolName { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl fmt::Display for DisplayProtocolName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use fmt::Write; for byte in self.0.protocol_name() { if (b' '..=b'~').contains(byte) { f.write_char(char::from(*byte))?; diff --git a/core/src/upgrade/apply.rs b/core/src/upgrade/apply.rs index 558c2e8eb29..aff5bb44955 100644 --- a/core/src/upgrade/apply.rs +++ b/core/src/upgrade/apply.rs @@ -154,10 +154,7 @@ where return Poll::Pending; } Poll::Ready(Ok(x)) => { - log::trace!( - "Upgraded inbound stream to {}", - DisplayProtocolName(name) - ); + log::trace!("Upgraded inbound stream to {}", DisplayProtocolName(name)); return Poll::Ready(Ok(x)); } Poll::Ready(Err(e)) => { @@ -244,14 +241,14 @@ where } Poll::Ready(Ok(x)) => { log::trace!( - "Successfully applied negotiated outbound protocol {}", + "Upgraded outbound stream to {}", DisplayProtocolName(name) ); return Poll::Ready(Ok(x)); } Poll::Ready(Err(e)) => { debug!( - "Failed to apply negotiated outbound protocol {}", + "Failed to upgrade outbound stream to {}", DisplayProtocolName(name) ); return Poll::Ready(Err(UpgradeError::Apply(e))); From 1fb088aaeac82374a8182598a5228305fef53c67 Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Thu, 24 Nov 2022 09:36:18 +0100 Subject: [PATCH 4/7] replace test with doctest --- core/src/upgrade.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index ed150277b34..cf09c271a73 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -135,7 +135,14 @@ impl> ProtocolName for T { } } -pub struct DisplayProtocolName(N); +/// Wrapper for printing a [`ProtocolName`] that is expected to be mostly ASCII +/// +/// ```rust +/// # use libp2p_core::upgrade::DisplayProtocolName; +/// assert_eq!(DisplayProtocolName("hello").to_string(), "hello"); +/// assert_eq!(DisplayProtocolName("hellö/").to_string(), "hell/"); +/// ``` +pub struct DisplayProtocolName(pub N); impl fmt::Display for DisplayProtocolName { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use fmt::Write; @@ -150,12 +157,6 @@ impl fmt::Display for DisplayProtocolName { } } -#[test] -fn protocol_name_display() { - assert_eq!(DisplayProtocolName("hello").to_string(), "hello"); - assert_eq!(DisplayProtocolName("hellö/").to_string(), "hell/"); -} - /// Common trait for upgrades that can be applied on inbound substreams, outbound substreams, /// or both. pub trait UpgradeInfo { From e38213cdc18b3d6d58e33c212b20e9e7a16b6819 Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Thu, 24 Nov 2022 12:54:25 +0100 Subject: [PATCH 5/7] Update core/src/upgrade.rs Co-authored-by: Max Inden --- core/src/upgrade.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index cf09c271a73..b433cf84b6a 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -143,6 +143,7 @@ impl> ProtocolName for T { /// assert_eq!(DisplayProtocolName("hellö/").to_string(), "hell/"); /// ``` pub struct DisplayProtocolName(pub N); + impl fmt::Display for DisplayProtocolName { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use fmt::Write; From 579d86ef6574cc501a5be7254484f774c740ff4e Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Mon, 5 Dec 2022 09:36:23 +0100 Subject: [PATCH 6/7] test all the bytes --- core/src/upgrade.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index b433cf84b6a..759c90528e0 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -139,8 +139,15 @@ impl> ProtocolName for T { /// /// ```rust /// # use libp2p_core::upgrade::DisplayProtocolName; -/// assert_eq!(DisplayProtocolName("hello").to_string(), "hello"); -/// assert_eq!(DisplayProtocolName("hellö/").to_string(), "hell/"); +/// assert_eq!(DisplayProtocolName(b"/hello/1.0").to_string(), "/hello/1.0"); +/// assert_eq!(DisplayProtocolName("/hellö/").to_string(), "/hell/"); +/// # assert_eq!( +/// # DisplayProtocolName((0u8..=255).collect::>()).to_string(), +/// # (0..32).map(|c| format!("<{:02X}>", c)) +/// # .chain((32..127).map(|c| format!("{}", char::from_u32(c).unwrap()))) +/// # .chain((127..256).map(|c| format!("<{:02X}>", c))) +/// # .collect::() +/// # ); /// ``` pub struct DisplayProtocolName(pub N); From 90ab1edd9cfe12a3b5aabdcb7e8cfd9bd798cf26 Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Fri, 9 Dec 2022 10:26:18 +0100 Subject: [PATCH 7/7] move DisplayProtocolName to apply.rs and make pub(crate) --- core/src/upgrade.rs | 31 ----------------------------- core/src/upgrade/apply.rs | 41 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/core/src/upgrade.rs b/core/src/upgrade.rs index 759c90528e0..de9ef765e16 100644 --- a/core/src/upgrade.rs +++ b/core/src/upgrade.rs @@ -86,7 +86,6 @@ pub use self::{ }; pub use crate::Negotiated; pub use multistream_select::{NegotiatedComplete, NegotiationError, ProtocolError, Version}; -use std::fmt; /// Types serving as protocol names. /// @@ -135,36 +134,6 @@ impl> ProtocolName for T { } } -/// Wrapper for printing a [`ProtocolName`] that is expected to be mostly ASCII -/// -/// ```rust -/// # use libp2p_core::upgrade::DisplayProtocolName; -/// assert_eq!(DisplayProtocolName(b"/hello/1.0").to_string(), "/hello/1.0"); -/// assert_eq!(DisplayProtocolName("/hellö/").to_string(), "/hell/"); -/// # assert_eq!( -/// # DisplayProtocolName((0u8..=255).collect::>()).to_string(), -/// # (0..32).map(|c| format!("<{:02X}>", c)) -/// # .chain((32..127).map(|c| format!("{}", char::from_u32(c).unwrap()))) -/// # .chain((127..256).map(|c| format!("<{:02X}>", c))) -/// # .collect::() -/// # ); -/// ``` -pub struct DisplayProtocolName(pub N); - -impl fmt::Display for DisplayProtocolName { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - use fmt::Write; - for byte in self.0.protocol_name() { - if (b' '..=b'~').contains(byte) { - f.write_char(char::from(*byte))?; - } else { - write!(f, "<{:02X}>", byte)?; - } - } - Ok(()) - } -} - /// Common trait for upgrades that can be applied on inbound substreams, outbound substreams, /// or both. pub trait UpgradeInfo { diff --git a/core/src/upgrade/apply.rs b/core/src/upgrade/apply.rs index aff5bb44955..8f0bcdabc55 100644 --- a/core/src/upgrade/apply.rs +++ b/core/src/upgrade/apply.rs @@ -18,9 +18,7 @@ // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -use crate::upgrade::{ - DisplayProtocolName, InboundUpgrade, OutboundUpgrade, ProtocolName, UpgradeError, -}; +use crate::upgrade::{InboundUpgrade, OutboundUpgrade, ProtocolName, UpgradeError}; use crate::{connection::ConnectedPoint, Negotiated}; use futures::{future::Either, prelude::*}; use log::debug; @@ -29,6 +27,7 @@ use std::{iter, mem, pin::Pin, task::Context, task::Poll}; pub use multistream_select::Version; use smallvec::SmallVec; +use std::fmt; // TODO: Still needed? /// Applies an upgrade to the inbound and outbound direction of a connection or substream. @@ -274,3 +273,39 @@ impl AsRef<[u8]> for NameWrap { self.0.protocol_name() } } + +/// Wrapper for printing a [`ProtocolName`] that is expected to be mostly ASCII +pub(crate) struct DisplayProtocolName(pub N); + +impl fmt::Display for DisplayProtocolName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use fmt::Write; + for byte in self.0.protocol_name() { + if (b' '..=b'~').contains(byte) { + f.write_char(char::from(*byte))?; + } else { + write!(f, "<{:02X}>", byte)?; + } + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn display_protocol_name() { + assert_eq!(DisplayProtocolName(b"/hello/1.0").to_string(), "/hello/1.0"); + assert_eq!(DisplayProtocolName("/hellö/").to_string(), "/hell/"); + assert_eq!( + DisplayProtocolName((0u8..=255).collect::>()).to_string(), + (0..32) + .map(|c| format!("<{:02X}>", c)) + .chain((32..127).map(|c| format!("{}", char::from_u32(c).unwrap()))) + .chain((127..256).map(|c| format!("<{:02X}>", c))) + .collect::() + ); + } +}