From 6bd84d001f23f4c720da35c9a064190b59514ce3 Mon Sep 17 00:00:00 2001
From: Kyle Sabo <robo210@gmail.com>
Date: Fri, 23 Aug 2024 00:44:16 -0700
Subject: [PATCH] pr feedback

---
 src/error.rs                               |  6 ++++--
 src/layer.rs                               | 25 ++++++++++++++++------
 src/native/common_schema/user_events_cs.rs |  2 +-
 src/native/mod.rs                          |  2 +-
 src/native/user_events.rs                  |  2 +-
 5 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/error.rs b/src/error.rs
index 611bfb0..fb46e1e 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -2,10 +2,12 @@ use thiserror::Error;
 
 #[derive(Error, Debug)]
 pub enum EtwError {
-    #[error("Provider group GUID must not be zeroes")]
+    #[error("Provider group GUID must not be zeros")]
     EmptyProviderGroupGuid,
     #[error("Provider group names must be lower case ASCII or numeric digits: {0:?}")]
     InvalidProviderGroupCharacters(String),
     #[error("Linux provider names must be ASCII alphanumeric: {0:?}")]
-    InvalidProviderNameCharacters(String)
+    InvalidProviderNameCharacters(String),
+    #[error("Linux provider name and provider group must less than 234 characters combined. Current length: {0:?}")]
+    TooManyCharacters(usize)
 }
diff --git a/src/layer.rs b/src/layer.rs
index cd7ce87..ea2ac72 100644
--- a/src/layer.rs
+++ b/src/layer.rs
@@ -128,13 +128,24 @@ where
 
     fn validate_config(&self) -> Result<(), EtwError> {
         #[cfg(target_os = "linux")]
-        if self
-            .provider_name
-            .contains(|f: char| !f.is_ascii_alphanumeric())
         {
-            // The perf command is very particular about the provider names it accepts.
-            // The Linux kernel itself cares less, and other event consumers should also presumably not need this check.
-            return Err(EtwError::InvalidProviderNameCharacters(self.provider_name.clone()));
+            if self
+                .provider_name
+                .contains(|f: char| !f.is_ascii_alphanumeric() && f != '_')
+            {
+                // The perf command is very particular about the provider names it accepts.
+                // The Linux kernel itself cares less, and other event consumers should also presumably not need this check.
+                return Err(EtwError::InvalidProviderNameCharacters(self.provider_name.clone()));
+            }
+
+            let group_name_len = match &self.provider_group {
+                None => 0,
+                Some(ref name) => Mode::get_provider_group(&name).as_ref().len()
+            };
+
+            if self.provider_name.len() + group_name_len >= 234 {
+                return Err(EtwError::TooManyCharacters(self.provider_name.len() + group_name_len));
+            }
         }
 
         match &self.provider_group {
@@ -150,7 +161,7 @@ where
         match self.provider_group {
             None => {}
             Some(ref name) => {
-                targets = targets.with_target(Mode::get_provider_group(name), LevelFilter::TRACE);
+                targets = targets.with_target(Mode::get_provider_group(name).as_ref(), LevelFilter::TRACE);
             }
         }
 
diff --git a/src/native/common_schema/user_events_cs.rs b/src/native/common_schema/user_events_cs.rs
index 27c31cf..b6f9e87 100644
--- a/src/native/common_schema/user_events_cs.rs
+++ b/src/native/common_schema/user_events_cs.rs
@@ -68,7 +68,7 @@ impl crate::native::ProviderTypes for CommonSchemaProvider {
         }
     }
 
-    fn get_provider_group(value: &Self::ProviderGroupType) -> impl Into<String> {
+    fn get_provider_group(value: &Self::ProviderGroupType) -> impl AsRef<str> {
         value.clone()
     }
 }
diff --git a/src/native/mod.rs b/src/native/mod.rs
index 1d9cf21..6b81ece 100644
--- a/src/native/mod.rs
+++ b/src/native/mod.rs
@@ -100,7 +100,7 @@ pub trait ProviderTypes {
     // that it trivially matches a constraint unless we lower the constraint
     // checking into the impl, done here through constraint on the return type.
     #[cfg(target_os = "linux")]
-    fn get_provider_group(value: &Self::ProviderGroupType) -> impl Into<String>;
+    fn get_provider_group(value: &Self::ProviderGroupType) -> impl AsRef<str>;
 }
 
 #[doc(hidden)]
diff --git a/src/native/user_events.rs b/src/native/user_events.rs
index 6f70f50..bc98b70 100644
--- a/src/native/user_events.rs
+++ b/src/native/user_events.rs
@@ -75,7 +75,7 @@ impl crate::native::ProviderTypes for Provider {
         }
     }
 
-    fn get_provider_group(value: &Self::ProviderGroupType) -> impl Into<String> {
+    fn get_provider_group(value: &Self::ProviderGroupType) -> impl AsRef<str> {
         value.clone()
     }
 }