[PATCH] atm/clip: Use seq_puts() in svc_addr()

[PATCH] atm/clip: Use seq_puts() in svc_addr()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 6 Jan 2018 22:34:12 +0100

Two strings should be quickly put into a sequence by two function calls.
Thus use the function "seq_puts" instead of "seq_printf".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 net/atm/clip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/atm/clip.c b/net/atm/clip.c
index d4f6029d5109..62a852165b19 100644
--- a/net/atm/clip.c
+++ b/net/atm/clip.c
@@ -708,11 +708,11 @@ static void svc_addr(struct seq_file *seq, struct sockaddr_atmsvc *addr)
 	static int e164[] = { 1, 8, 4, 6, 1, 0 };
 
 	if (*addr->sas_addr.pub) {
-		seq_printf(seq, "%s", addr->sas_addr.pub);
+		seq_puts(seq, addr->sas_addr.pub);
 		if (*addr->sas_addr.prv)
 			seq_putc(seq, '+');
 	} else if (!*addr->sas_addr.prv) {
-		seq_printf(seq, "%s", "(none)");
+		seq_puts(seq, "(none)");
 		return;
 	}
 	if (*addr->sas_addr.prv) {
-- 
2.15.1

Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[Stefano Brivio]

On Sat, 6 Jan 2018 22:44:08 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 6 Jan 2018 22:34:12 +0100
> 
> Two strings should be quickly put into a sequence by two function calls.
> Thus use the function "seq_puts" instead of "seq_printf".
>
> This issue was detected by using the Coccinelle software.

Can you please explain what the issue really is and what you're trying
to do here? One shouldn't need to dig into Coccinelle patterns to find
out what you mean, and "strings should be quickly put into a sequence"
isn't terribly helpful.

-- 
Stefano

Re: atm/clip: Use seq_puts() in svc_addr()

[SF Markus Elfring]

>> Two strings should be quickly put into a sequence by two function calls.
>> Thus use the function "seq_puts" instead of "seq_printf".
>>
>> This issue was detected by using the Coccinelle software.
> 
> Can you please explain what the issue really is and what you're trying
> to do here?

Is the function "seq_puts" a bit more efficient for the desired output
of a single string in comparison to calling the function "seq_printf"
for this purpose?


> One shouldn't need to dig into Coccinelle patterns to find
> out what you mean,

Why did an attribution for a software tool confuse you?


> and "strings should be quickly put into a sequence"
> isn't terribly helpful.

Which wording would you find more appropriate for the suggested
adjustment of these function calls?

Regards,
Markus

Re: atm/clip: Use seq_puts() in svc_addr()

[Stefano Brivio]

On Sun, 7 Jan 2018 09:19:17 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >> Two strings should be quickly put into a sequence by two function calls.
> >> Thus use the function "seq_puts" instead of "seq_printf".
> >>
> >> This issue was detected by using the Coccinelle software.  
> > 
> > Can you please explain what the issue really is and what you're trying
> > to do here?  
> 
> Is the function "seq_puts" a bit more efficient for the desired output
> of a single string in comparison to calling the function "seq_printf"
> for this purpose?

Will you please be so kind and tell us?

> > One shouldn't need to dig into Coccinelle patterns to find
> > out what you mean,  
> 
> Why did an attribution for a software tool confuse you?

I'm not confused. I'm saying that one shouldn't need to dig into
Coccinelle patterns to find out what you mean.

> > and "strings should be quickly put into a sequence"
> > isn't terribly helpful.  
> 
> Which wording would you find more appropriate for the suggested
> adjustment of these function calls?

Whatever describes the actual issue and what you're doing about it.
Turn your rhetorical question above into a commit message, done.

Compare that with your original commit message, on the other hand,
and you should understand what I mean.

-- 
Stefano

Re: atm/clip: Use seq_puts() in svc_addr()

[SF Markus Elfring]

>> Is the function "seq_puts" a bit more efficient for the desired output
>> of a single string in comparison to calling the function "seq_printf"
>> for this purpose?
> 
> Will you please be so kind and tell us?

How do you think about to get the run time characteristics for these
sequence output functions better documented?
https://elixir.free-electrons.com/linux/v4.15-rc6/source/fs/seq_file.c#L660

Can an information like “WARNING: Prefer seq_puts to seq_printf”
(from the script “checkpatch.pl”) be another incentive?


>>> and "strings should be quickly put into a sequence"
>>> isn't terribly helpful.  
>>
>> Which wording would you find more appropriate for the suggested
>> adjustment of these function calls?
> 
> Whatever describes the actual issue and what you're doing about it.
> Turn your rhetorical question above into a commit message, done.
> 
> Compare that with your original commit message, on the other hand,
> and you should understand what I mean.

Which descriptions are you really missing for the affected data output?

Regards,
Markus

Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[Andy Shevchenko]

On Sat, Jan 6, 2018 at 11:44 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 6 Jan 2018 22:34:12 +0100
>
> Two strings should be quickly put into a sequence by two function calls.
> Thus use the function "seq_puts" instead of "seq_printf".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  net/atm/clip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/atm/clip.c b/net/atm/clip.c
> index d4f6029d5109..62a852165b19 100644
> --- a/net/atm/clip.c
> +++ b/net/atm/clip.c
> @@ -708,11 +708,11 @@ static void svc_addr(struct seq_file *seq, struct sockaddr_atmsvc *addr)
>         static int e164[] = { 1, 8, 4, 6, 1, 0 };
>
>         if (*addr->sas_addr.pub) {
> -               seq_printf(seq, "%s", addr->sas_addr.pub);
> +               seq_puts(seq, addr->sas_addr.pub);

Which opens a lot of security concerns.
Never do this again.

>                 if (*addr->sas_addr.prv)
>                         seq_putc(seq, '+');
>         } else if (!*addr->sas_addr.prv) {

> -               seq_printf(seq, "%s", "(none)");
> +               seq_puts(seq, "(none)");

...while this one is okay per se, better to keep above pattern (same
style over the piece of code / function).

>                 return;
>         }
>         if (*addr->sas_addr.prv) {
> --
> 2.15.1
>

P.S. I'm wondering what would be first, Markus starts looking into the
actual code, or most (all) of the maintainers just ban him.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[SF Markus Elfring]

>> @@ -708,11 +708,11 @@ static void svc_addr(struct seq_file *seq, struct sockaddr_atmsvc *addr)
>>         static int e164[] = { 1, 8, 4, 6, 1, 0 };
>>
>>         if (*addr->sas_addr.pub) {
>> -               seq_printf(seq, "%s", addr->sas_addr.pub);
>> +               seq_puts(seq, addr->sas_addr.pub);
> 
> Which opens a lot of security concerns.

How? - The passed string is just copied into a buffer finally, isn't it?


> Never do this again.

Why do you not like such a small source code transformation at the moment?


> P.S. I'm wondering what would be first,

I am curious on how communication difficulties can be adjusted.


> Markus starts looking into the actual code,

I inspected the original source code to some degree.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/fs/seq_file.c?id=895c0dde398510a5b5ded60e5064c11b94bd30ca#n682
https://elixir.free-electrons.com/linux/v4.15-rc6/source/fs/seq_file.c#L660


> or most (all) of the maintainers just ban him.

The change acceptance is varying for various reasons by the involved contributors.

Regards,
Markus