Re: [PATCH v4 6/6] trace-cmd: Reafctored reset_max_latency() to utilize write_instance_file()

[Steven Rostedt <rostedt@goodmis.org>]

[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]

On Mon, 11 Mar 2019 10:33:39 +0200
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:


> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index bdf0c02..dfbb0cd 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -814,19 +814,49 @@ static void clear_trace(void)
>  	fclose(fp);
>  }
>  
> -static void reset_max_latency(void)
> +static int write_file(const char *file, const char *str, const char *type)
> +{
> +	char buf[BUFSIZ];
> +	int fd;
> +	int ret;
> +
> +	fd = open(file, O_WRONLY | O_TRUNC);
> +	if (fd < 0)
> +		die("opening to '%s'", file);
> +	ret = write(fd, str, strlen(str));
> +	close(fd);
> +	if (ret < 0 && type) {
> +		/* write failed */
> +		fd = open(file, O_RDONLY);
> +		if (fd < 0)
> +			die("writing to '%s'", file);
> +		/* the filter has the error */
> +		while ((ret = read(fd, buf, BUFSIZ)) > 0)
> +			fprintf(stderr, "%.*s", ret, buf);
> +		die("Failed %s of %s\n", type, file);
> +		close(fd);
> +	}
> +	return ret;
> +}
> +
> +static int
> +write_instance_file(struct buffer_instance *instance,
> +		    const char *file, const char *str, const char *type)
>  {
> -	FILE *fp;
>  	char *path;
> +	int ret;
>  
> -	/* reset the trace */
> -	path = tracecmd_get_tracing_file("tracing_max_latency");
> -	fp = fopen(path, "w");
> -	if (!fp)
> -		die("writing to '%s'", path);
> +	path = get_instance_file(instance, file);
> +	ret = write_file(path, str, type);
>  	tracecmd_put_tracing_file(path);
> -	fwrite("0", 1, 1, fp);
> -	fclose(fp);
> +
> +	return ret;

To make this easier to review and understand, I added a change before
patch 4 that moved write_instance_file() up further. This way, there's
a single patch that moves the code. Especially, since you moved it
twice. Sometimes its cleaner to do the move as a separate change.
Especially when you are modifying the change near the move.

I rebased the changes on top of the move, and the result is attached.

-- Steve

[-- Attachment #2: 0001-trace-cmd-Move-write_instance_file-up-in-file.patch --]
[-- Type: text/x-patch, Size: 2804 bytes --]

From cbd567febd2559b055576ec48312fa53f46f76f5 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Tue, 12 Mar 2019 21:13:19 -0400
Subject: [PATCH 1/4] trace-cmd: Move write_instance_file() up in file

To allow for reset_max_latency() and add_filter_pid() to utilize
write_instance_file(), move it up before those functions. It is in a better
location now as well.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tracecmd/trace-record.c | 78 ++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 00dc5ad7..f1f5d29d 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -776,6 +776,45 @@ get_instance_dir(struct buffer_instance *instance)
 	return path;
 }
 
+static int write_file(const char *file, const char *str, const char *type)
+{
+	char buf[BUFSIZ];
+	int fd;
+	int ret;
+
+	fd = open(file, O_WRONLY | O_TRUNC);
+	if (fd < 0)
+		die("opening to '%s'", file);
+	ret = write(fd, str, strlen(str));
+	close(fd);
+	if (ret < 0 && type) {
+		/* write failed */
+		fd = open(file, O_RDONLY);
+		if (fd < 0)
+			die("writing to '%s'", file);
+		/* the filter has the error */
+		while ((ret = read(fd, buf, BUFSIZ)) > 0)
+			fprintf(stderr, "%.*s", ret, buf);
+		die("Failed %s of %s\n", type, file);
+		close(fd);
+	}
+	return ret;
+}
+
+static int
+write_instance_file(struct buffer_instance *instance,
+		    const char *file, const char *str, const char *type)
+{
+	char *path;
+	int ret;
+
+	path = get_instance_file(instance, file);
+	ret = write_file(path, str, type);
+	tracecmd_put_tracing_file(path);
+
+	return ret;
+}
+
 static void __clear_trace(struct buffer_instance *instance)
 {
 	FILE *fp;
@@ -1596,45 +1635,6 @@ static void reset_events(void)
 		reset_events_instance(instance);
 }
 
-static int write_file(const char *file, const char *str, const char *type)
-{
-	char buf[BUFSIZ];
-	int fd;
-	int ret;
-
-	fd = open(file, O_WRONLY | O_TRUNC);
-	if (fd < 0)
-		die("opening to '%s'", file);
-	ret = write(fd, str, strlen(str));
-	close(fd);
-	if (ret < 0 && type) {
-		/* write failed */
-		fd = open(file, O_RDONLY);
-		if (fd < 0)
-			die("writing to '%s'", file);
-		/* the filter has the error */
-		while ((ret = read(fd, buf, BUFSIZ)) > 0)
-			fprintf(stderr, "%.*s", ret, buf);
-		die("Failed %s of %s\n", type, file);
-		close(fd);
-	}
-	return ret;
-}
-
-static int
-write_instance_file(struct buffer_instance *instance,
-		    const char *file, const char *str, const char *type)
-{
-	char *path;
-	int ret;
-
-	path = get_instance_file(instance, file);
-	ret = write_file(path, str, type);
-	tracecmd_put_tracing_file(path);
-
-	return ret;
-}
-
 enum {
 	STATE_NEWLINE,
 	STATE_SKIP,
-- 
2.20.1


[-- Attachment #3: 0002-trace-cmd-Refactore-add_event_pid-to-utilize-write_i.patch --]
[-- Type: text/x-patch, Size: 1911 bytes --]

From cd5a23168280917774094eeb77b5a359e4f4a292 Mon Sep 17 00:00:00 2001
From: Tzvetomir Stoyanov <tstoyanov@vmware.com>
Date: Mon, 11 Mar 2019 10:33:37 +0200
Subject: [PATCH 2/4] trace-cmd: Refactore add_event_pid()to utilize
 write_instance_file()

This patch changes add_event_pid() to utilize write_instance_file() for
writing set_event_pid instance file, instead of directly opening it.

Link: http://lore.kernel.org/linux-trace-devel/20190311083339.21581-5-tstoyanov@vmware.com

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>
Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tracecmd/trace-record.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index f1f5d29d..801dad24 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -1133,30 +1133,25 @@ static void update_sched_events(struct buffer_instance *instance, int pid)
 static int open_instance_fd(struct buffer_instance *instance,
 			    const char *file, int flags);
 
-static void add_event_pid(const char *buf, int len)
+static void add_event_pid(const char *buf)
 {
 	struct buffer_instance *instance;
-	int fd;
 
-	for_all_instances(instance) {
-		fd = open_instance_fd(instance, "set_event_pid", O_WRONLY);
-		write(fd, buf, len);
-		close(fd);
-	}
+	for_all_instances(instance)
+		write_instance_file(instance, "set_event_pid", buf, "event_pid");
 }
 
 static void add_new_filter_pid(int pid)
 {
 	struct buffer_instance *instance;
 	char buf[100];
-	int len;
 
 	add_filter_pid(pid, 0);
-	len = sprintf(buf, "%d", pid);
+	sprintf(buf, "%d", pid);
 	update_ftrace_pid(buf, 0);
 
 	if (have_set_event_pid)
-		return add_event_pid(buf, len);
+		return add_event_pid(buf);
 
 	common_pid_filter = append_pid_filter(common_pid_filter, "common_pid", pid);
 
-- 
2.20.1


[-- Attachment #4: 0004-trace-cmd-Refactore-reset_max_latency-to-utilize-wri.patch --]
[-- Type: text/x-patch, Size: 2507 bytes --]

From 7b4254303277ee6c5833f427fea7f00fa161bd74 Mon Sep 17 00:00:00 2001
From: Tzvetomir Stoyanov <tstoyanov@vmware.com>
Date: Mon, 11 Mar 2019 10:33:39 +0200
Subject: [PATCH 4/4] trace-cmd: Refactore reset_max_latency() to utilize
 write_instance_file()

This patch changes reset_max_latency() to utilize write_instance_file() for
writing set_event_pid instance file, instead of directly opening it. It also
changes the function to work per instance.

Link: http://lore.kernel.org/linux-trace-devel/20190311083339.21581-7-tstoyanov@vmware.com

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>
Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tracecmd/trace-record.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index fe22ad08..c16abe2f 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -853,19 +853,10 @@ static void clear_trace(void)
 	fclose(fp);
 }
 
-static void reset_max_latency(void)
+static void reset_max_latency(struct buffer_instance *instance)
 {
-	FILE *fp;
-	char *path;
-
-	/* reset the trace */
-	path = tracecmd_get_tracing_file("tracing_max_latency");
-	fp = fopen(path, "w");
-	if (!fp)
-		die("writing to '%s'", path);
-	tracecmd_put_tracing_file(path);
-	fwrite("0", 1, 1, fp);
-	fclose(fp);
+	 write_instance_file(instance,
+			     "tracing_max_latency", "0", "max_latency");
 }
 
 static void add_filter_pid(int pid, int exclude)
@@ -1931,6 +1922,14 @@ static int read_tracing_on(struct buffer_instance *instance)
 	return ret;
 }
 
+static void reset_max_latency_instance(void)
+{
+	struct buffer_instance *instance;
+
+	for_all_instances(instance)
+		reset_max_latency(instance);
+}
+
 void tracecmd_enable_tracing(void)
 {
 	struct buffer_instance *instance;
@@ -1941,7 +1940,7 @@ void tracecmd_enable_tracing(void)
 		write_tracing_on(instance, 1);
 
 	if (latency)
-		reset_max_latency();
+		reset_max_latency_instance();
 }
 
 void tracecmd_disable_tracing(void)
@@ -3802,7 +3801,6 @@ static void reset_event_pid(void)
 	add_event_pid("");
 }
 
-
 static void clear_triggers(void)
 {
 	struct buffer_instance *instance;
@@ -4507,6 +4505,7 @@ void trace_reset(int argc, char **argv)
 	/* set clock to "local" */
 	reset_clock();
 	reset_event_pid();
+	reset_max_latency_instance();
 	tracecmd_remove_instances();
 	clear_func_filters();
 	/* restore tracing_on to 1 */
-- 
2.20.1