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
|