Re: [PATCH V6 09/20] rtla: Add osnoise tool

On 10/27/21 18:30, Tao Zhou wrote:

On Wed, Oct 27, 2021 at 12:06:20AM +0200, Daniel Bristot de Oliveira wrote:

quoted

+/*
+ * osnoise_set_cpus - configure osnoise to run on *cpus
+ *
+ * "osnoise/cpus" file is used to set the cpus in which osnoise/timerlat
+ * will run. This function opens this file, saves the current value,
+ * and set the cpus passed as argument.
+ */
+int osnoise_set_cpus(struct osnoise_context *context, char *cpus)
+{
+	char *osnoise_cpus = tracefs_get_tracing_file("osnoise/cpus");
+	char curr_cpus[1024];
+	int retval;
+
+	context->cpus_fd = open(osnoise_cpus, O_RDWR);
+	if (!context->cpus_fd)
+		goto out_err;

The above check should be "context->cpus_fd < 0".

Revisited all open/read/write!

quoted

+	retval = read(context->cpus_fd, &curr_cpus, sizeof(curr_cpus));
+	if (!retval)
+		goto out_close;
+	context->orig_cpus = strdup(curr_cpus);
+	if (!context->orig_cpus)
+		goto out_err;

Need to close ->cpus_fd;

  if (!context->orig_cpus)
    goto out_close;

quoted

+	retval = write(context->cpus_fd, cpus, strlen(cpus) + 1);
+	if (!retval)
+		goto out_err;

Same as above. Use "goto out_close" instead.

yep! fixed in the next version.

quoted

+	tracefs_put_tracing_file(osnoise_cpus);
+
+	return 0;
+
+out_close:
+	close(context->cpus_fd);
+	context->cpus_fd = -1;
+out_err:
+	tracefs_put_tracing_file(osnoise_cpus);
+	return 1;
+}
+
+/*
+ * osnoise_restore_cpus - restore the original "osnoise/cpus"
+ *
+ * osnoise_set_cpus() saves the original data for the "osnoise/cpus"
+ * file. This function restore the original config it was previously
+ * modified.
+ */
+void osnoise_restore_cpus(struct osnoise_context *context)
+{
+	int retval;
+
+	if (!context->orig_cpus)
+		return;
+
+	retval = write(context->cpus_fd, context->orig_cpus, strlen(context->orig_cpus));

__osnoise_write_runtime() check "context->cpus_fd == -1".
Is it possible here we need to check "context->cpus_fd == -1".

So, yeah, this was inconsistent. In some parts I checked the fd, on other I
checked if the original value was load, so the file was opened. In the next
version I am checking if the original value was loaded, and then using it to
define if the fd is open.

Thanks for your review Tao!
-- Daniel