diff mbox series

[v2,2/6] libtracefs: Change APIs to work with constant strings

Message ID 20201110122249.911664-3-tz.stoyanov@gmail.com (mailing list archive)
State Superseded
Headers show
Series libtracefs fixes and improvements | expand

Commit Message

Tzvetomir Stoyanov (VMware) Nov. 10, 2020, 12:22 p.m. UTC 
Some tracefs library APIs retrieve trace instance parameters as srings.
As these strings are not meant to be changed by the API callers, set
them to be constant. These APIs are affected:
 tracefs_instance_get_name()
 tracefs_event_systems()
 tracefs_system_events()
 tracefs_tracers()
 tracefs_list_free()

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/tracefs/tracefs.h      | 10 ++++----
 lib/tracefs/tracefs-events.c   | 47 ++++++++++++++--------------------
 lib/tracefs/tracefs-instance.c |  4 +--
 tracecmd/trace-record.c        |  2 +-
 utest/tracefs-utest.c          | 23 +++++++++--------
 5 files changed, 39 insertions(+), 47 deletions(-)

Comments

Steven Rostedt Nov. 11, 2020, 10:43 p.m. UTC | #1 
On Tue, 10 Nov 2020 14:22:45 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Some tracefs library APIs retrieve trace instance parameters as srings.
> As these strings are not meant to be changed by the API callers, set
> them to be constant. These APIs are affected:

>  tracefs_instance_get_name()

Correct, because we pass back a pointer to a stored internal name.


>  tracefs_event_systems()
>  tracefs_system_events()
>  tracefs_tracers()
>  tracefs_list_free()

I don't think we need to make the above const char*. We are allocating them
and these strings don't point to anything internal. In other words, they
are OK to modify without causing issues.

I think we only need the tracefs_instance_get_name() to be const.

Previously, I thought we were passing back internal pointers, but if we are
making strings with strdup, then we don't need to use const char *.

-- Steve
diff mbox series

Patch

diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
index 8ee7ba6e..8d193853 100644
--- a/include/tracefs/tracefs.h
+++ b/include/tracefs/tracefs.h
@@ -24,7 +24,7 @@  struct tracefs_instance *tracefs_instance_alloc(const char *name);
 void tracefs_instance_free(struct tracefs_instance *instance);
 int tracefs_instance_create(struct tracefs_instance *instance);
 int tracefs_instance_destroy(struct tracefs_instance *instance);
-char *tracefs_instance_get_name(struct tracefs_instance *instance);
+const char *tracefs_instance_get_name(struct tracefs_instance *instance);
 char *
 tracefs_instance_get_file(struct tracefs_instance *instance, const char *file);
 char *tracefs_instance_get_dir(struct tracefs_instance *instance);
@@ -37,9 +37,9 @@  bool tracefs_file_exists(struct tracefs_instance *instance, char *name);
 bool tracefs_dir_exists(struct tracefs_instance *instance, char *name);
 
 /* events */
-void tracefs_list_free(char **list);
-char **tracefs_event_systems(const char *tracing_dir);
-char **tracefs_system_events(const char *tracing_dir, const char *system);
+void tracefs_list_free(const char **list);
+const char **tracefs_event_systems(const char *tracing_dir);
+const char **tracefs_system_events(const char *tracing_dir, const char *system);
 int tracefs_iterate_raw_events(struct tep_handle *tep,
 				struct tracefs_instance *instance,
 				int (*callback)(struct tep_event *,
@@ -47,7 +47,7 @@  int tracefs_iterate_raw_events(struct tep_handle *tep,
 						int, void *),
 				void *callback_context);
 
-char **tracefs_tracers(const char *tracing_dir);
+const char **tracefs_tracers(const char *tracing_dir);
 
 struct tep_handle *tracefs_local_events(const char *tracing_dir);
 struct tep_handle *tracefs_local_events_system(const char *tracing_dir,
diff --git a/lib/tracefs/tracefs-events.c b/lib/tracefs/tracefs-events.c
index 8e825f50..19f98058 100644
--- a/lib/tracefs/tracefs-events.c
+++ b/lib/tracefs/tracefs-events.c
@@ -227,7 +227,7 @@  static char *append_file(const char *dir, const char *name)
  *
  *@list pointer to a list of strings, the last one must be NULL
  */
-void tracefs_list_free(char **list)
+void tracefs_list_free(const char **list)
 {
 	int i;
 
@@ -235,7 +235,7 @@  void tracefs_list_free(char **list)
 		return;
 
 	for (i = 0; list[i]; i++)
-		free(list[i]);
+		free((char *)list[i]);
 
 	free(list);
 }
@@ -245,11 +245,11 @@  void tracefs_list_free(char **list)
  * @tracing_dir: directory holding the "events" directory
  *		 if NULL, top tracing directory is used
  *
- * Returns an allocated list of system names. Both the names and
- * the list must be freed with tracefs_list_free()
- * The list returned ends with a "NULL" pointer
+ *
+ * Returns an allocated list of system names, the last element
+ * is a NULL pointer. The list must be freed with tracefs_list_free()*
  */
-char **tracefs_event_systems(const char *tracing_dir)
+const char **tracefs_event_systems(const char *tracing_dir)
 {
 	struct dirent *dent;
 	char **systems = NULL;
@@ -311,7 +311,7 @@  char **tracefs_event_systems(const char *tracing_dir)
 
  out_free:
 	free(events_dir);
-	return systems;
+	return (const char **)systems;
 }
 
 /**
@@ -319,11 +319,10 @@  char **tracefs_event_systems(const char *tracing_dir)
  * @tracing_dir: directory holding the "events" directory
  * @system: the system to return the events for
  *
- * Returns an allocated list of event names. Both the names and
- * the list must be freed with tracefs_list_free()
- * The list returned ends with a "NULL" pointer
+ * Returns an allocated list of event names, the last element
+ * is a NULL pointer. The list must be freed with tracefs_list_free()
  */
-char **tracefs_system_events(const char *tracing_dir, const char *system)
+const char **tracefs_system_events(const char *tracing_dir, const char *system)
 {
 	struct dirent *dent;
 	char **events = NULL;
@@ -376,17 +375,17 @@  char **tracefs_system_events(const char *tracing_dir, const char *system)
  out_free:
 	free(system_dir);
 
-	return events;
+	return (const char **)events;
 }
 
 /**
  * tracefs_tracers - returns an array of available tracers
  * @tracing_dir: The directory that contains the tracing directory
  *
- * Returns an allocate list of plugins. The array ends with NULL
- * Both the plugin names and array must be freed with free()
+ * Returns an allocated list of trace plugin names, the last element
+ * is a NULL pointer. The list must be freed with tracefs_list_free()*
  */
-char **tracefs_tracers(const char *tracing_dir)
+const char **tracefs_tracers(const char *tracing_dir)
 {
 	char *available_tracers;
 	struct stat st;
@@ -438,14 +437,14 @@  char **tracefs_tracers(const char *tracing_dir)
  out_free:
 	free(available_tracers);
 
-	return plugins;
+	return (const char **)plugins;
 }
 
 static int load_events(struct tep_handle *tep,
 		       const char *tracing_dir, const char *system)
 {
+	const char **events = NULL;
 	int ret = 0, failure = 0;
-	char **events = NULL;
 	struct stat st;
 	int len = 0;
 	int i;
@@ -481,11 +480,7 @@  next_event:
 			failure = ret;
 	}
 
-	if (events) {
-		for (i = 0; events[i]; i++)
-			free(events[i]);
-		free(events);
-	}
+	tracefs_list_free(events);
 	return failure;
 }
 
@@ -532,7 +527,7 @@  static int fill_local_events_system(const char *tracing_dir,
 				    const char * const *sys_names,
 				    int *parsing_failures)
 {
-	char **systems = NULL;
+	const char **systems = NULL;
 	int ret;
 	int i;
 
@@ -564,11 +559,7 @@  static int fill_local_events_system(const char *tracing_dir,
 	/* always succeed because parsing failures are not critical */
 	ret = 0;
 out:
-	if (systems) {
-		for (i = 0; systems[i]; i++)
-			free(systems[i]);
-		free(systems);
-	}
+	tracefs_list_free(systems);
 	return ret;
 }
 
diff --git a/lib/tracefs/tracefs-instance.c b/lib/tracefs/tracefs-instance.c
index 50e88534..e9ec3401 100644
--- a/lib/tracefs/tracefs-instance.c
+++ b/lib/tracefs/tracefs-instance.c
@@ -169,7 +169,7 @@  char *tracefs_instance_get_dir(struct tracefs_instance *instance)
  * Returns the name of the given @instance.
  * The returned string must *not* be freed.
  */
-char *tracefs_instance_get_name(struct tracefs_instance *instance)
+const char *tracefs_instance_get_name(struct tracefs_instance *instance)
 {
 	if (instance)
 		return instance->name;
@@ -252,8 +252,8 @@  static bool check_file_exists(struct tracefs_instance *instance,
 			     char *name, bool dir)
 {
 	char file[PATH_MAX];
-	struct stat st;
 	char *path;
+	struct stat st;
 	int ret;
 
 	path = tracefs_instance_get_dir(instance);
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 72a5c8c9..8cd44dd0 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3861,7 +3861,7 @@  static void connect_to_agent(struct buffer_instance *instance)
 	unsigned int *ports;
 	int i, *fds = NULL;
 	bool use_fifos = false;
-	char *name;
+	const char *name;
 
 	name = tracefs_instance_get_name(instance->tracefs);
 	if (!no_fifos) {
diff --git a/utest/tracefs-utest.c b/utest/tracefs-utest.c
index 1c146576..31a700ff 100644
--- a/utest/tracefs-utest.c
+++ b/utest/tracefs-utest.c
@@ -180,7 +180,7 @@  static void test_instance_file(void)
 {
 	struct tracefs_instance *instance = NULL;
 	const char *name = get_rand_str();
-	char *inst_name = NULL;
+	const char *inst_name = NULL;
 	const char *tdir;
 	char *inst_file;
 	char *inst_dir;
@@ -308,9 +308,9 @@  static void test_check_files(const char *fdir, char **files)
 
 static void test_system_event(void)
 {
+	const char **systems;
+	const char **events;
 	const char *tdir;
-	char **systems;
-	char **events;
 	char *sdir = NULL;
 
 	tdir  = tracefs_get_tracing_dir();
@@ -324,13 +324,13 @@  static void test_system_event(void)
 
 	asprintf(&sdir, "%s/events/%s", tdir, systems[0]);
 	CU_TEST(sdir != NULL);
-	test_check_files(sdir, events);
+	test_check_files(sdir, (char **)events);
 	free(sdir);
 	sdir = NULL;
 
 	asprintf(&sdir, "%s/events", tdir);
 	CU_TEST(sdir != NULL);
-	test_check_files(sdir, systems);
+	test_check_files(sdir, (char **)systems);
 
 	tracefs_list_free(systems);
 	tracefs_list_free(events);
@@ -340,8 +340,8 @@  static void test_system_event(void)
 
 static void test_tracers(void)
 {
+	const char **tracers;
 	const char *tdir;
-	char **tracers;
 	char *tfile;
 	char *tracer;
 	int i;
@@ -356,7 +356,7 @@  static void test_tracers(void)
 
 	tracer = strtok(tfile, " ");
 	while (tracer) {
-		exclude_string(tracers, tracer);
+		exclude_string((char **)tracers, tracer);
 		tracer = strtok(NULL, " ");
 	}
 
@@ -367,7 +367,8 @@  static void test_tracers(void)
 	free(tfile);
 }
 
-static void test_check_events(struct tep_handle *tep, char *system, bool exist)
+static void test_check_events(struct tep_handle *tep,
+			      const char *system, bool exist)
 {
 	struct dirent *dent;
 	char file[PATH_MAX];
@@ -409,8 +410,8 @@  static void test_check_events(struct tep_handle *tep, char *system, bool exist)
 static void test_local_events(void)
 {
 	struct tep_handle *tep;
+	const char **systems;
 	const char *tdir;
-	char **systems;
 	char *lsystems[3];
 	int i;
 
@@ -430,9 +431,9 @@  static void test_local_events(void)
 	memset(lsystems, 0, sizeof(lsystems));
 	for (i = 0; systems[i]; i++) {
 		if (!lsystems[0])
-			lsystems[0] = systems[i];
+			lsystems[0] = (char *)systems[i];
 		else if (!lsystems[2])
-			lsystems[2] = systems[i];
+			lsystems[2] = (char *)systems[i];
 		else
 			break;
 	}