Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: Qmbtw2F8zNZipQY9deifv2fvr/GHaJHWjEfubbHmsOqKnt2S46OAlbwDadhju4oFzabZfRfrcF
 qcIobJ6jClXg==
IronPort-SDR: w50aPhiGKgfyEp2ixKa+Ql5GA/p/3PIzAErG1Sfc3HLpOw1HrIe3LRA73fAIsyhI68Ja/TYrXO
 hjR03SIr9B0A==
Subject: Re: [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings
 terminated by newline
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     tglx@linutronix.de, fenghua.yu@intel.com, bp@alien8.de,
        tony.luck@intel.com, kuo-lang.tseng@intel.com,
        ravi.v.shankar@intel.com, mingo@redhat.com, babu.moger@amd.com,
        hpa@zytor.com, x86@kernel.org, linux-kernel@vger.kernel.org
References: <cover.1589652468.git.reinette.chatre@intel.com>
 <0c04b58148f181ba182b9a487465e4853e4eadbb.1589652468.git.reinette.chatre@intel.com>
 <20200518115026.GA1634618@smile.fi.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
Message-ID: <060e3cf6-8fcb-6b43-01ef-63ef0a5ff64a@intel.com>
Date:   Mon, 18 May 2020 09:06:00 -0700
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <20200518115026.GA1634618@smile.fi.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Andy,

On 5/18/2020 4:50 AM, Andy Shevchenko wrote:
> On Sat, May 16, 2020 at 11:28:41AM -0700, Reinette Chatre wrote:
>> The user input to files in the resctrl filesystem are expected to be
>> terminated with a newline. Testing the user input includes a test for
>> the presence of a newline and then replacing the newline with NUL
>> byte followed by comparison using strcmp().
>>
>> sysfs_streq() exists to test if strings are equal, treating both NUL and
>> newline-then-NUL as equivalent string terminations. Even more,
>> sysfs_match_string() exists to match a given string in an array using
>> sysfs_streq().
>>
>> Replace existing strcmp() comparisons of strings that are terminated
>> with a newline with more appropriate sysfs_streq() via the
>> sysfs_match_string() API that can perform the match across the different
>> mode strings that are already maintained in an array.
> 
> Sorry for late comment, but just have noticed...

No problem. I do appreciate your feedback because it helps me to improve
the code.

> 
>>  	if (mode == RDT_MODE_PSEUDO_LOCKED) {
>> @@ -1445,14 +1450,14 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
>>  		goto out;
>>  	}
>>  
>> -	if (!strcmp(buf, "shareable")) {
>> +	if (user_m == RDT_MODE_SHAREABLE) {
>>  		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>>  			ret = rdtgroup_locksetup_exit(rdtgrp);
>>  			if (ret)
>>  				goto out;
>>  		}
> 
>>  		rdtgrp->mode = RDT_MODE_SHAREABLE;
> 
> ...can we simple move this and similar (in other branches) to...
> 
> 
>> -	} else {
>> -		rdt_last_cmd_puts("Unknown or unsupported mode\n");
>> -		ret = -EINVAL;
>>  	}
> 
> ...here as
> 
> 		rdtgrp->mode = user_m;

Will do.

It also looks like the only reason for the "mode" local variable was to
make those earlier "strcmp" lines shorter. With those long lines removed
in this patch this local variable is no longer needed and I will remove
it also.

Reinette