Address Feedback

Fix eventpipe_collect_tracing_command_free_tracepoint_sets logic
Clarify len variable names
Consistently use -1 as invalid file descriptor
Update event_pipe to use ep_rt_utf16_to_utf8_string/free

Author: mdh1418

src/mono/mono/component/event_pipe.c

@@ -158,16 +158,16 @@ event_pipe_enable (
 
 	if (config_providers) {
 		for (guint32 i = 0; i < providers_len; ++i) {
-			char *provider_name = providers[i].provider_name ? mono_utf16_to_utf8 (providers[i].provider_name, g_utf16_len (providers[i].provider_name), error) : NULL;
-			char *filter_data = providers[i].filter_data ? mono_utf16_to_utf8 (providers[i].filter_data, g_utf16_len (providers[i].filter_data), error) : NULL;
+			ep_char8_t *provider_name = ep_rt_utf16_to_utf8_string ((const ep_char16_t *)(providers[i].provider_name));
+			ep_char8_t *filter_data = ep_rt_utf16_to_utf8_string ((const ep_char16_t *)(providers[i].filter_data));
 			ep_provider_config_init (
 				&config_providers[i],
 				provider_name,
 				providers [i].keywords,
 				(EventPipeEventLevel)providers [i].logging_level,
 				filter_data);
-			g_free (provider_name);
-			g_free (filter_data);
+			ep_rt_utf8_string_free (provider_name);
+			ep_rt_utf8_string_free (filter_data);
 		}
 	}
 

src/native/eventpipe/ds-eventpipe-protocol.c

@@ -80,7 +80,7 @@ bool
 eventpipe_collect_tracing_command_try_parse_event_ids (
 	uint8_t **buffer,
 	uint32_t *buffer_len,
-	uint32_t length,
+	uint32_t event_ids_len,
 	uint32_t **event_ids);
 
 static
@@ -95,7 +95,7 @@ bool
 eventpipe_collect_tracing_command_try_parse_tracepoint_sets (
 	uint8_t **buffer,
 	uint32_t *buffer_len,
-	uint32_t length,
+	uint32_t tracepoint_sets_len,
 	EventPipeProviderTracepointSet **tracepoint_sets);
 
 static
@@ -319,7 +319,7 @@ bool
 eventpipe_collect_tracing_command_try_parse_event_ids (
 	uint8_t **buffer,
 	uint32_t *buffer_len,
-	uint32_t length,
+	uint32_t event_ids_len,
 	uint32_t **event_ids)
 {
 	EP_ASSERT (buffer != NULL);
@@ -329,12 +329,12 @@ eventpipe_collect_tracing_command_try_parse_event_ids (
 	bool result = false;
 	*event_ids = NULL;
 
-	if (length == 0)
+	if (event_ids_len == 0)
 		return true;
 
-	*event_ids = ep_rt_object_array_alloc (uint32_t, length);
+	*event_ids = ep_rt_object_array_alloc (uint32_t, event_ids_len);
 	ep_raise_error_if_nok (*event_ids != NULL);
-	for (uint32_t i = 0; i < length; ++i)
+	for (uint32_t i = 0; i < event_ids_len; ++i)
 		ep_raise_error_if_nok (ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, &(*event_ids)[i]));
 
 	result = true;
@@ -398,7 +398,7 @@ bool
 eventpipe_collect_tracing_command_try_parse_tracepoint_sets (
 	uint8_t **buffer,
 	uint32_t *buffer_len,
-	uint32_t length,
+	uint32_t tracepoint_sets_len,
 	EventPipeProviderTracepointSet **tracepoint_sets)
 {
 	EP_ASSERT (buffer != NULL);
@@ -408,13 +408,13 @@ eventpipe_collect_tracing_command_try_parse_tracepoint_sets (
 	bool result = false;
 	*tracepoint_sets = NULL;
 
-	if (length == 0)
+	if (tracepoint_sets_len == 0)
 		return false;
 
-	*tracepoint_sets = ep_rt_object_array_alloc (EventPipeProviderTracepointSet, length);
+	*tracepoint_sets = ep_rt_object_array_alloc (EventPipeProviderTracepointSet, tracepoint_sets_len);
 	ep_raise_error_if_nok (*tracepoint_sets != NULL);
 
-	for (uint32_t i = 0; i < length; ++i) {
+	for (uint32_t i = 0; i < tracepoint_sets_len; ++i) {
 		ep_raise_error_if_nok (ds_ipc_message_try_parse_string_utf16_t_string_utf8_t_alloc (buffer, buffer_len, &(*tracepoint_sets)[i].tracepoint_name));
 		ep_raise_error_if_nok (!ep_rt_utf8_string_is_null_or_empty ((*tracepoint_sets)[i].tracepoint_name));
 
@@ -429,7 +429,7 @@ eventpipe_collect_tracing_command_try_parse_tracepoint_sets (
 	return result;
 
 ep_on_error:
-	eventpipe_collect_tracing_command_free_tracepoint_sets (*tracepoint_sets, length);
+	eventpipe_collect_tracing_command_free_tracepoint_sets (*tracepoint_sets, tracepoint_sets_len);
 	*tracepoint_sets = NULL;
 	ep_exit_error_handler ();
 }
@@ -901,7 +901,7 @@ eventpipe_protocol_helper_collect_tracing (
 		return false;
 	}
 
-	int user_events_data_fd = 0;
+	int user_events_data_fd = -1;
 	if (payload->session_type == EP_SESSION_TYPE_USEREVENTS) {
 		if (!ds_ipc_stream_read_fd (stream, &user_events_data_fd)) {
 			ds_ipc_message_send_error (stream, DS_IPC_E_BAD_ENCODING);

src/native/eventpipe/ep-session-provider.c

@@ -157,7 +157,7 @@ session_provider_tracepoint_register (
 	int user_events_data_fd)
 {
 	EP_ASSERT (tracepoint != NULL);
-	EP_ASSERT (user_events_data_fd != 0);
+	EP_ASSERT (user_events_data_fd != -1);
 	struct user_reg reg = {0};
 
 	reg.size = sizeof(reg);
@@ -182,7 +182,7 @@ session_provider_tracepoint_unregister (
 	int user_events_data_fd)
 {
 	EP_ASSERT (tracepoint != NULL);
-	EP_ASSERT (user_events_data_fd != 0);
+	EP_ASSERT (user_events_data_fd != -1);
 	struct user_unreg unreg = {0};
 
 	unreg.size = sizeof(unreg);
@@ -229,9 +229,9 @@ ep_session_provider_register_tracepoints (
 	int user_events_data_fd)
 {
 	EP_ASSERT (session_provider != NULL);
-	EP_ASSERT (user_events_data_fd > 0);
+	EP_ASSERT (user_events_data_fd != -1);
 
-	if (user_events_data_fd <= 0)
+	if (user_events_data_fd < 0)
 		return false;
 
 	EventPipeSessionProviderTracepointConfiguration *tracepoint_config = session_provider->tracepoint_config;
@@ -267,9 +267,9 @@ ep_session_provider_unregister_tracepoints (
 	int user_events_data_fd)
 {
 	EP_ASSERT (session_provider != NULL);
-	EP_ASSERT (user_events_data_fd > 0);
+	EP_ASSERT (user_events_data_fd != -1);
 
-	if (user_events_data_fd <= 0)
+	if (user_events_data_fd < 0)
 		return;
 
 	if (session_provider->tracepoint_config == NULL)

src/native/eventpipe/ep-session.c

@@ -221,7 +221,7 @@ session_user_events_tracepoints_init (
 	EventPipeSessionProviderList *providers = session->providers;
 	EP_ASSERT (providers != NULL);
 
-	ep_raise_error_if_nok (user_events_data_fd > 0);
+	ep_raise_error_if_nok (user_events_data_fd != -1);
 	session->user_events_data_fd = user_events_data_fd;
 
 	DN_LIST_FOREACH_BEGIN (EventPipeSessionProvider *, session_provider, ep_session_provider_list_get_providers (providers)) {
@@ -625,7 +625,7 @@ void
 session_disable_user_events (EventPipeSession *session)
 {
 	EP_ASSERT (session != NULL);
-	ep_return_void_if_nok (session->session_type == EP_SESSION_TYPE_USEREVENTS && session->user_events_data_fd > 0);
+	ep_return_void_if_nok (session->session_type == EP_SESSION_TYPE_USEREVENTS && session->user_events_data_fd != -1);
 
 	ep_requires_lock_held ();
 
@@ -919,7 +919,7 @@ ep_session_write_event (
 			result = true;
 			break;
 		case EP_SESSION_TYPE_USEREVENTS:
-			EP_ASSERT (session->user_events_data_fd != 0);
+			EP_ASSERT (session->user_events_data_fd != -1);
 			result = session_tracepoint_write_event (
 				session,
 				thread,

src/native/eventpipe/ep-types.h

@@ -225,7 +225,7 @@ struct _EventPipeProviderTracepointSet {
 #endif
 
 void
-eventpipe_collect_tracing_command_free_tracepoint_sets (EventPipeProviderTracepointSet *tracepoint_set, uint32_t length);
+eventpipe_collect_tracing_command_free_tracepoint_sets (EventPipeProviderTracepointSet *tracepoint_sets, uint32_t tracepoint_sets_len);
 
 /*
  * EventPipeProviderTracepointConfiguration.

src/native/eventpipe/ep.c

@@ -369,16 +369,16 @@ eventpipe_collect_tracing_command_free_event_filter (EventPipeProviderEventFilte
 }
 
 void
-eventpipe_collect_tracing_command_free_tracepoint_sets (EventPipeProviderTracepointSet *tracepoint_set, uint32_t length)
+eventpipe_collect_tracing_command_free_tracepoint_sets (EventPipeProviderTracepointSet *tracepoint_sets, uint32_t tracepoint_sets_len)
 {
-	ep_return_void_if_nok (tracepoint_set != NULL);
+	ep_return_void_if_nok (tracepoint_sets != NULL);
 
-	for (uint32_t i = 0; i < length; ++i) {
-		ep_rt_utf8_string_free (tracepoint_set->tracepoint_name);
-		ep_rt_object_array_free (tracepoint_set->event_ids);
+	for (uint32_t i = 0; i < tracepoint_sets_len; ++i) {
+		ep_rt_utf8_string_free (tracepoint_sets[i].tracepoint_name);
+		ep_rt_object_array_free (tracepoint_sets[i].event_ids);
 	}
 
-	ep_rt_object_array_free (tracepoint_set);
+	ep_rt_object_array_free (tracepoint_sets);
 }
 
 void
@@ -524,7 +524,7 @@ static bool check_options_valid (const EventPipeSessionOptions *options)
 	if (options->session_type == EP_SESSION_TYPE_IPCSTREAM && options->stream == NULL)
 		return false;
 	// More UserEvents specific checks can be added here.
-	if (options->session_type == EP_SESSION_TYPE_USEREVENTS && options->user_events_data_fd == 0)
+	if (options->session_type == EP_SESSION_TYPE_USEREVENTS && options->user_events_data_fd == -1)
 		return false;
 
 	return true;