Re: [PATCH V5 4/4] x86/resctrl: Use appropriate API for strings terminated by newline

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Andy,

On a high level the changes from v4 to v5 aimed to address your feedback
and also ensure that original user interface behavior is maintained.

On 5/19/2020 1:28 AM, Andy Shevchenko wrote:
> On Tue, May 19, 2020 at 2:50 AM Reinette Chatre
> <reinette.chatre@intel.com> wrote:
> 
> ...
> 
>> +       ret = sysfs_match_string(rdt_mode_str, buf);
>> +       if (ret < 0) {
>> +               rdt_last_cmd_puts("Unknown or unsupported mode\n");
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }

From your previous email ...

>> +       ret = sysfs_match_string(rdt_mode_str, buf);
>> +       if (ret < 0) {
>> +               rdt_last_cmd_puts("Unknown or unsupported mode\n");
>
>> +               ret = -EINVAL;
>
> This is redundant.

I understand that shadowing an error code is generally of concern. In
this case the error code is set to -EINVAL to ensure that it is the same
error code that was returned to user space originally and will continue
to be so no matter what changes may come to sysfs_match_string().

>> +
>> +       user_m = ret;
> 
>> +       } else if (user_m == RDT_MODE_PSEUDO_LOCKED) {
>>                 rdt_last_cmd_puts("Unknown or unsupported mode\n");
>>                 ret = -EINVAL;
>> +               goto out;
>>         }
> 
> Can't we unify latter with a former like
> 
>   if (ret == RDT_MODE_PSEUDO_LOCKED)
>     ret = -EINVAL;
>   if (ret < 0) {
>       rdt_last_cmd_puts("Unknown or unsupported mode\n");
>       goto out;
>   }
> 
> ?
> 
> Or closer to initial like
> 
>   if (ret < 0 || ret == RDT_MODE_PSEUDO_LOCKED) {
>     rdt_last_cmd_puts("Unknown or unsupported mode\n");
>     ret = -EINVAL;
>     goto out;
>   }
> 

This would have been ideal if done from the start but currently "0" is
returned if the current mode is pseudo-locked and user attempts to
change the mode to pseudo-locked. Thus, to maintain the current user
interface the check if user wants to set pseudo-locked mode is moved
after the check if new mode is same as existing mode and thus not
unified because that will result in an error returned always when user
requests pseudo-locked mode.

Reinette