Srivinas,
On 02-05-2013 00:50, Srinivas Pandruvada wrote:
> Hi Valentin,
>
> Any further thoughts?
Yeah,
>
> Thanks,
> Srinivas
>
> On 04/26/2013 04:32 PM, Srinivas Pandruvada wrote:
>> Hi Valentin,
>>
>> I planned to use that before , but it will change the semantics here.
>>
>> - Here comparison is using a case in-sensitive version, sysfs_streq is
>> case sensitive.
Indeed. That changes the semantics. On the other hand, I do not see why
we need to be case in-sensitive. Depending on how you see this, it might
infringe the sysfs rules. Besides, I am favor to have 1 to 1 relation
between array of chars and their meaning.
On the other hand, changing the semantics might break userland. I
personally do not have any userland application that uses this sysfs
nodes. Neither I am aware of any. But I request Rui and Durga s opinions
here.
>> - I am not sure if there is any protection for length. If we pass a
>> more than THERM_NAME_LENGTH string, then whether sysfs_streq can
>> handle. so we need to pre-check for length.
sysfs_streq does not check for length, only for string ending char and
CRLF. In fact we would need to check for string ending before using it.
Side note, I believe we might consider the following:
diff --git a/lib/string.c b/lib/string.c
index e5878de..b83eee7 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -522,7 +522,7 @@ EXPORT_SYMBOL(strsep);
*/
bool sysfs_streq(const char *s1, const char *s2)
{
- while (*s1 && *s1 == *s2) {
+ while (*s1 && *s2 && *s1 == *s2) {
s1++;
s2++;
}
>> - some stupid echo command with CRLF, will still fail
sure about that? sysfs_streq() was actually introduced to avoid it. If
yes, then we have a bug on it.
>> I see that some other syfs string write (eg. cpufreq.c) decided not to
>> use sysfs_streq, may be historical.
>>
yeah, maybe. But have you git greped for sysfs_streq()?
Obviously, your patch provides more protection. On the other hand, I
believe we need to check if we fix this locally or if we need to fix
sysfs_streq() too. Providing a sysfs_strneq() would also be a good thing.
>> Thanks,
>> Srinivas
>>
>>
>> On 04/26/2013 02:21 PM, Eduardo Valentin wrote:
>>> Hello Srinivas,
>>>
>>> On 26-04-2013 16:35, Srinivas Pandruvada wrote:
>>>> Setting policy results in invalid value error.
>>>> >echo "step_wise" > policy
>>>> >echo: write error: Invalid argument
>>>>
>>>> Need clean up of the buffer which "echo" may add based on the
>>>> arguments, before comparing aganist list of governor names.
>>>>
>>>> Signed-off-by: Srinivas Pandruvada
>>>> <srinivas.pandruvada@xxxxxxxxxxxxxxx>
>>>> ---
>>>> drivers/thermal/thermal_core.c | 19 +++++++++++++++----
>>>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.c
>>>> b/drivers/thermal/thermal_core.c
>>>> index 4cdc3e3..ed6904f 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -696,16 +696,27 @@ static ssize_t
>>>> policy_store(struct device *dev, struct device_attribute *attr,
>>>> const char *buf, size_t count)
>>>> {
>>>> - int ret = -EINVAL;
>>>> + int ret;
>>>> struct thermal_zone_device *tz = to_thermal_zone(dev);
>>>> struct thermal_governor *gov;
>>>> + char str_gov[THERMAL_NAME_LENGTH];
>>>> + char format[6]; /* enough for 3 digit format width */
>>>> +
>>>> + ret = snprintf(format, sizeof(format), "%%%ds",
>>>> + THERMAL_NAME_LENGTH - 1);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + ret = sscanf(buf, format, str_gov);
>>>> + if (ret <= 0)
>>>> + return -EINVAL;
>>>
>>>
>>> Is this due to trainling \n? Why not using sysfs_streq()? I believe
>>> it is better approach. Can you please check if the following solves
>>> the issue?
>>>
>>> https://gitorious.org/thermal-framework/thermal-framework/commit/810a33a629b40adfa92a164883281bbdfad80516
>>>
>>>
>>> commit 810a33a629b40adfa92a164883281bbdfad80516
>>> Author: Eduardo Valentin <eduardo.valentin@xxxxxx>
>>> Date: Fri Apr 26 17:12:30 2013 -0400
>>>
>>> thermal: better string treatment while finding governors
>>>
>>> To avoid returning error value just because of trailing
>>> \n, this patch changes the function to find governors
>>> by name to use sysfs_streq().
>>>
>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@xxxxxx>
>>>
>>> diff --git a/drivers/thermal/thermal_core.c
>>> b/drivers/thermal/thermal_core.c
>>> index f36cd44..08ea62c 100644
>>> --- a/drivers/thermal/thermal_core.c
>>> +++ b/drivers/thermal/thermal_core.c
>>> @@ -58,7 +58,7 @@ static struct thermal_governor
>>> *__find_governor(const char *name)
>>> struct thermal_governor *pos;
>>>
>>> list_for_each_entry(pos, &thermal_governor_list, governor_list)
>>> - if (!strnicmp(name, pos->name, THERMAL_NAME_LENGTH))
>>> + if (sysfs_streq(name, pos->name))
>>> return pos;
>>>
>>> return NULL;
>>>
>>>>
>>>> mutex_lock(&thermal_governor_lock);
>>>>
>>>> - gov = __find_governor(buf);
>>>> - if (!gov)
>>>> + gov = __find_governor(str_gov);
>>>> + if (!gov) {
>>>> + ret = -EINVAL;
>>>> goto exit;
>>>> -
>>>> + }
>>>> tz->governor = gov;
>>>> ret = count;
>>>>
>>>>
>>>
>>>
>>
>>
>
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
