[Patch v2] netpoll: warn when there are spaces in parameters

[Patch v2] netpoll: warn when there are spaces in parameters

[Amerigo Wang]

v2: update according to Frans' comments.

Currently, if we leave spaces before dst port,
netconsole will silently accept it as 0. Warn about this.

Also, when spaces appear in other places, make them
visible in error messages.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: David Miller <davem@davemloft.net>

---

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 7aa6972..6df1863 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -614,7 +614,7 @@ void netpoll_print_options(struct netpoll *np)
 			 np->name, np->local_port);
 	printk(KERN_INFO "%s: local IP %pI4\n",
 			 np->name, &np->local_ip);
-	printk(KERN_INFO "%s: interface %s\n",
+	printk(KERN_INFO "%s: interface '%s'\n",
 			 np->name, np->dev_name);
 	printk(KERN_INFO "%s: remote port %d\n",
 			 np->name, np->remote_port);
@@ -661,6 +661,9 @@ int netpoll_parse_options(struct netpoll *np, char *opt)
 		if ((delim = strchr(cur, '@')) == NULL)
 			goto parse_failed;
 		*delim = 0;
+		if (*cur == ' ' || *cur == '\t')
+			printk(KERN_INFO "%s: warning: whitespace"
+					"is not allowed\n", np->name);
 		np->remote_port = simple_strtol(cur, NULL, 10);
 		cur = delim;
 	}
@@ -708,7 +711,7 @@ int netpoll_parse_options(struct netpoll *np, char *opt)
 	return 0;
 
  parse_failed:
-	printk(KERN_INFO "%s: couldn't parse config at %s!\n",
+	printk(KERN_INFO "%s: couldn't parse config at '%s'!\n",
 	       np->name, cur);
 	return -1;
 }

Re: [Patch v2] netpoll: warn when there are spaces in parameters

[Neil Horman]

On Mon, Mar 22, 2010 at 04:59:58AM -0400, Amerigo Wang wrote:
> v2: update according to Frans' comments.
> 
> Currently, if we leave spaces before dst port,
> netconsole will silently accept it as 0. Warn about this.
> 
> Also, when spaces appear in other places, make them
> visible in error messages.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: David Miller <davem@davemloft.net>
> 
> ---
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 7aa6972..6df1863 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -614,7 +614,7 @@ void netpoll_print_options(struct netpoll *np)
>  			 np->name, np->local_port);
>  	printk(KERN_INFO "%s: local IP %pI4\n",
>  			 np->name, &np->local_ip);
> -	printk(KERN_INFO "%s: interface %s\n",
> +	printk(KERN_INFO "%s: interface '%s'\n",
>  			 np->name, np->dev_name);
>  	printk(KERN_INFO "%s: remote port %d\n",
>  			 np->name, np->remote_port);
> @@ -661,6 +661,9 @@ int netpoll_parse_options(struct netpoll *np, char *opt)
>  		if ((delim = strchr(cur, '@')) == NULL)
>  			goto parse_failed;
>  		*delim = 0;
> +		if (*cur == ' ' || *cur == '\t')
> +			printk(KERN_INFO "%s: warning: whitespace"
> +					"is not allowed\n", np->name);
>  		np->remote_port = simple_strtol(cur, NULL, 10);
>  		cur = delim;
>  	}
> @@ -708,7 +711,7 @@ int netpoll_parse_options(struct netpoll *np, char *opt)
>  	return 0;
>  
>   parse_failed:
> -	printk(KERN_INFO "%s: couldn't parse config at %s!\n",
> +	printk(KERN_INFO "%s: couldn't parse config at '%s'!\n",
>  	       np->name, cur);
>  	return -1;
>  }
Acked-by: Neil Horman <nhorman@tuxdriver.com>

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [Patch v2] netpoll: warn when there are spaces in parameters

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 22 Mar 2010 08:30:46 -0400

> On Mon, Mar 22, 2010 at 04:59:58AM -0400, Amerigo Wang wrote:
>> v2: update according to Frans' comments.
>> 
>> Currently, if we leave spaces before dst port,
>> netconsole will silently accept it as 0. Warn about this.
>> 
>> Also, when spaces appear in other places, make them
>> visible in error messages.
>> 
>> Signed-off-by: WANG Cong <amwang@redhat.com>
 ...
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied, thanks!

Re: [Patch v2] netpoll: warn when there are spaces in parameters

[Matt Mackall]

On Mon, 2010-03-22 at 04:59 -0400, Amerigo Wang wrote:
> +			printk(KERN_INFO "%s: warning: whitespace"
> +					"is not allowed\n", np->name);

Is it a warning or is it info? If it's a warning, then we probably need
to add "netpoll" or whatever to the message so that people who've got a
warning-level threshold will know what it's about.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [Patch v2] netpoll: warn when there are spaces in parameters

[Cong Wang]

Matt Mackall wrote:
> On Mon, 2010-03-22 at 04:59 -0400, Amerigo Wang wrote:
>> +			printk(KERN_INFO "%s: warning: whitespace"
>> +					"is not allowed\n", np->name);
> 
> Is it a warning or is it info? If it's a warning, then we probably need
> to add "netpoll" or whatever to the message so that people who've got a
> warning-level threshold will know what it's about.
> 

If you mean KERN_INFO, yeah, I want to keep it in the same level
as other messages around.

Also, I already put "np->name" which will be "netconsole" when
we use netconsole.

Thanks.

Re: [Patch v2] netpoll: warn when there are spaces in parameters

[Matt Mackall]

On Tue, 2010-03-23 at 09:44 +0800, Cong Wang wrote:
> Matt Mackall wrote:
> > On Mon, 2010-03-22 at 04:59 -0400, Amerigo Wang wrote:
> >> +			printk(KERN_INFO "%s: warning: whitespace"
> >> +					"is not allowed\n", np->name);
> > 
> > Is it a warning or is it info? If it's a warning, then we probably need
> > to add "netpoll" or whatever to the message so that people who've got a
> > warning-level threshold will know what it's about.
> > 
> 
> If you mean KERN_INFO, yeah, I want to keep it in the same level
> as other messages around.

I should probably be more direct: I think that's the wrong thing to do.
It IS a warning (it even says so!) telling users that something probably
won't work and why and they might miss it if the severity is INFO and
then come and ask us why things aren't working. So use KERN_WARN,
please.

The other messages are INFO because when things are working, they're not
interesting.

> Also, I already put "np->name" which will be "netconsole" when
> we use netconsole.

Ahh, right, I have a brain cell somewhere that knew that.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [Patch v2] netpoll: warn when there are spaces in parameters

[Cong Wang]

Matt Mackall wrote:
> On Tue, 2010-03-23 at 09:44 +0800, Cong Wang wrote:
>> Matt Mackall wrote:
>>> On Mon, 2010-03-22 at 04:59 -0400, Amerigo Wang wrote:
>>>> +			printk(KERN_INFO "%s: warning: whitespace"
>>>> +					"is not allowed\n", np->name);
>>> Is it a warning or is it info? If it's a warning, then we probably need
>>> to add "netpoll" or whatever to the message so that people who've got a
>>> warning-level threshold will know what it's about.
>>>
>> If you mean KERN_INFO, yeah, I want to keep it in the same level
>> as other messages around.
> 
> I should probably be more direct: I think that's the wrong thing to do.
> It IS a warning (it even says so!) telling users that something probably
> won't work and why and they might miss it if the severity is INFO and
> then come and ask us why things aren't working. So use KERN_WARN,
> please.


They _are_ working, 0 will be assigned by default.

> 
> The other messages are INFO because when things are working, they're not
> interesting.
> 

They are not all working, take this as an example:

         printk(KERN_INFO "%s: couldn't parse config at %s!\n",
                np->name, cur);

Re: [Patch v2] netpoll: warn when there are spaces in parameters

[Matt Mackall]

On Tue, 2010-03-23 at 12:34 +0800, Cong Wang wrote:
> Matt Mackall wrote:
> > On Tue, 2010-03-23 at 09:44 +0800, Cong Wang wrote:
> >> Matt Mackall wrote:
> >>> On Mon, 2010-03-22 at 04:59 -0400, Amerigo Wang wrote:
> >>>> +			printk(KERN_INFO "%s: warning: whitespace"
> >>>> +					"is not allowed\n", np->name);
> >>> Is it a warning or is it info? If it's a warning, then we probably need
> >>> to add "netpoll" or whatever to the message so that people who've got a
> >>> warning-level threshold will know what it's about.
> >>>
> >> If you mean KERN_INFO, yeah, I want to keep it in the same level
> >> as other messages around.
> > 
> > I should probably be more direct: I think that's the wrong thing to do.
> > It IS a warning (it even says so!) telling users that something probably
> > won't work and why and they might miss it if the severity is INFO and
> > then come and ask us why things aren't working. So use KERN_WARN,
> > please.
> 
> 
> They _are_ working, 0 will be assigned by default.

If this patch has any point at all other than filling the logs, it
should be WARN.

> > 
> > The other messages are INFO because when things are working, they're not
> > interesting.
> > 
> 
> They are not all working, take this as an example:
> 
>          printk(KERN_INFO "%s: couldn't parse config at %s!\n",
>                 np->name, cur);

Yeah, that's obviously wrong too. Let's not add more wrongness just for
consistency's sake.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [Patch v2] netpoll: warn when there are spaces in parameters

[Cong Wang]

Matt Mackall wrote:
> On Tue, 2010-03-23 at 12:34 +0800, Cong Wang wrote:
>> Matt Mackall wrote:
>>> On Tue, 2010-03-23 at 09:44 +0800, Cong Wang wrote:
>>>> Matt Mackall wrote:
>>>>> On Mon, 2010-03-22 at 04:59 -0400, Amerigo Wang wrote:
>>>>>> +			printk(KERN_INFO "%s: warning: whitespace"
>>>>>> +					"is not allowed\n", np->name);
>>>>> Is it a warning or is it info? If it's a warning, then we probably need
>>>>> to add "netpoll" or whatever to the message so that people who've got a
>>>>> warning-level threshold will know what it's about.
>>>>>
>>>> If you mean KERN_INFO, yeah, I want to keep it in the same level
>>>> as other messages around.
>>> I should probably be more direct: I think that's the wrong thing to do.
>>> It IS a warning (it even says so!) telling users that something probably
>>> won't work and why and they might miss it if the severity is INFO and
>>> then come and ask us why things aren't working. So use KERN_WARN,
>>> please.
>>
>> They _are_ working, 0 will be assigned by default.
> 
> If this patch has any point at all other than filling the logs, it
> should be WARN.

Others too.

> 
>>> The other messages are INFO because when things are working, they're not
>>> interesting.
>>>
>> They are not all working, take this as an example:
>>
>>          printk(KERN_INFO "%s: couldn't parse config at %s!\n",
>>                 np->name, cur);
> 
> Yeah, that's obviously wrong too. Let's not add more wrongness just for
> consistency's sake.
> 

Since you insist, please send a patch for all of these messages
in netpoll. This patch, from $subject, is aimed to warn spaces.

Thanks.