[PATCH 05/12] genirq/debugfs: Replace strncmp with str_has_prefix

[PATCH 05/12] genirq/debugfs: Replace strncmp with str_has_prefix

[Chuhong Yuan]

strncmp(str, const, len) is error-prone.
We had better use newly introduced
str_has_prefix() instead of it.

Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
 kernel/irq/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index c1eccd4f6520..85d1e9aeb8ea 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -188,7 +188,7 @@ static ssize_t irq_debug_write(struct file *file, const char __user *user_buf,
 	if (copy_from_user(buf, user_buf, size))
 		return -EFAULT;
 
-	if (!strncmp(buf, "trigger", size)) {
+	if (str_has_prefix(buf, "trigger")) {
 		unsigned long flags;
 		int err;
 
-- 
2.20.1

Re: [PATCH 05/12] genirq/debugfs: Replace strncmp with str_has_prefix

[Thomas Gleixner]

On Mon, 29 Jul 2019, Chuhong Yuan wrote:

> strncmp(str, const, len) is error-prone.
> We had better use newly introduced
> str_has_prefix() instead of it.

Can you please provide a proper explanation why the below strncmp() is
error prone?

Just running a script and copying some boiler plate changelog saying
'strncmp() is error prone' does not cut it.

> -	if (!strncmp(buf, "trigger", size)) {
> +	if (str_has_prefix(buf, "trigger")) {

Especially when the resulting code is not equivalent.

Thanks,

	tglx

Re: [PATCH 05/12] genirq/debugfs: Replace strncmp with str_has_prefix

[Chuhong Yuan]

Thomas Gleixner <tglx@linutronix.de> 于2019年7月30日周二 下午5:17写道：
>
> On Mon, 29 Jul 2019, Chuhong Yuan wrote:
>
> > strncmp(str, const, len) is error-prone.
> > We had better use newly introduced
> > str_has_prefix() instead of it.
>
> Can you please provide a proper explanation why the below strncmp() is
> error prone?
>

If the size is less than 7, for example, 2, then even if buf is "tr", the
result will still be true. This is an error.
strncmp(str, const, len) is error-prone mainly because the len is easy
to be wrong.

> Just running a script and copying some boiler plate changelog saying
> 'strncmp() is error prone' does not cut it.
>
> > -     if (!strncmp(buf, "trigger", size)) {
> > +     if (str_has_prefix(buf, "trigger")) {
>
> Especially when the resulting code is not equivalent.
>

I think here the semantic is the comparison should only return true
when buf is "trigger".
The buf's size is 8 and the string's size is at most 7.
Since str_has_prefix()'s implementation is strncmp(str, prefix, strlen(prefix)),
here strlen(prefix) = 7, I think it satisfies the requirement.

Regards,
Chuhong

> Thanks,
>
>         tglx

Re: [PATCH 05/12] genirq/debugfs: Replace strncmp with str_has_prefix

[Marc Zyngier]

On 30/07/2019 11:58, Chuhong Yuan wrote:
> Thomas Gleixner <tglx@linutronix.de> 于2019年7月30日周二 下午5:17写道：
>>
>> On Mon, 29 Jul 2019, Chuhong Yuan wrote:
>>
>>> strncmp(str, const, len) is error-prone.
>>> We had better use newly introduced
>>> str_has_prefix() instead of it.
>>
>> Can you please provide a proper explanation why the below strncmp() is
>> error prone?
>>
> 
> If the size is less than 7, for example, 2, then even if buf is "tr", the
> result will still be true. This is an error.
> strncmp(str, const, len) is error-prone mainly because the len is easy
> to be wrong.
> 
>> Just running a script and copying some boiler plate changelog saying
>> 'strncmp() is error prone' does not cut it.
>>
>>> -     if (!strncmp(buf, "trigger", size)) {
>>> +     if (str_has_prefix(buf, "trigger")) {
>>
>> Especially when the resulting code is not equivalent.
>>
> 
> I think here the semantic is the comparison should only return true
> when buf is "trigger".

Not quite. It will satisfy the condition for 't', 'tr', 'trig',
'trigger', and of course 'triggerthissillyinterruptwhichImdebugging'.

I agree that the semantic is a bit bizarre and maybe not quite expected,
but still... You seem to be changing the semantic without any
justification other than "this is safer".

Thanks,

	M.
-- 
Jazz is not dead, it just smells funny...

Re: [PATCH 05/12] genirq/debugfs: Replace strncmp with str_has_prefix

[Chuhong Yuan]

Marc Zyngier <maz@kernel.org> 于2019年7月30日周二 下午7:13写道：
>
> On 30/07/2019 11:58, Chuhong Yuan wrote:
> > Thomas Gleixner <tglx@linutronix.de> 于2019年7月30日周二 下午5:17写道：
> >>
> >> On Mon, 29 Jul 2019, Chuhong Yuan wrote:
> >>
> >>> strncmp(str, const, len) is error-prone.
> >>> We had better use newly introduced
> >>> str_has_prefix() instead of it.
> >>
> >> Can you please provide a proper explanation why the below strncmp() is
> >> error prone?
> >>
> >
> > If the size is less than 7, for example, 2, then even if buf is "tr", the
> > result will still be true. This is an error.
> > strncmp(str, const, len) is error-prone mainly because the len is easy
> > to be wrong.
> >
> >> Just running a script and copying some boiler plate changelog saying
> >> 'strncmp() is error prone' does not cut it.
> >>
> >>> -     if (!strncmp(buf, "trigger", size)) {
> >>> +     if (str_has_prefix(buf, "trigger")) {
> >>
> >> Especially when the resulting code is not equivalent.
> >>
> >
> > I think here the semantic is the comparison should only return true
> > when buf is "trigger".
>
> Not quite. It will satisfy the condition for 't', 'tr', 'trig',
> 'trigger', and of course 'triggerthissillyinterruptwhichImdebugging'.
>
> I agree that the semantic is a bit bizarre and maybe not quite expected,
> but still... You seem to be changing the semantic without any
> justification other than "this is safer".
>

I am sorry about that... It is my fault.
I will improve my script and avoid such mistakes.
Thanks for your correction.

Regards,
Chuhong

> Thanks,
>
>         M.
> --
> Jazz is not dead, it just smells funny...