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

[Daniel Bristot de Oliveira]

On 10/27/21 18:30, Tao Zhou wrote:
> On Wed, Oct 27, 2021 at 12:06:20AM +0200, Daniel Bristot de Oliveira wrote:
> 
>> +/*
>> + * 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!

>> +	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;
> 
>> +	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.

>> +	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