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

Amerigo Wang

unread,

Mar 17, 2010, 6:20:02 AM3/17/10

to

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 <amw...@redhat.com>
Cc: David Miller <da...@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: white spaces"
+ "are 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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Frans Pop

unread,

Mar 17, 2010, 7:00:03 AM3/17/10

to

Amerigo Wang wrote:
> + if (*cur == ' ' || *cur == '\t')
> + printk(KERN_INFO "%s: warning: white spaces"
> + "are not allowed.\n", np->name);
>

The term "white spaces" not correct. Please change the message to
"whitespace is not allowed".

Also, it's normally not necessary to close kernel messages with a period;
most kernel messages do not have a closing period. Messages are not
sentences and the periods only help increase the kernel size.

Thanks,
FJP

Cong Wang

unread,

Mar 17, 2010, 10:00:01 PM3/17/10

to

Frans Pop wrote:
> Amerigo Wang wrote:
>> + if (*cur == ' ' || *cur == '\t')
>> + printk(KERN_INFO "%s: warning: white spaces"
>> + "are not allowed.\n", np->name);
>>
>
> The term "white spaces" not correct. Please change the message to
> "whitespace is not allowed".
>
> Also, it's normally not necessary to close kernel messages with a period;
> most kernel messages do not have a closing period. Messages are not
> sentences and the periods only help increase the kernel size.
>

Ok, I will update it.

Thanks.

Amerigo Wang

unread,

Mar 22, 2010, 5:10:01 AM3/22/10

to

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 <amw...@redhat.com>
Cc: David Miller <da...@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;
}

Neil Horman

unread,

Mar 22, 2010, 8:40:02 AM3/22/10

to

Acked-by: Neil Horman <nho...@tuxdriver.com>

Matt Mackall

unread,

Mar 22, 2010, 6:30:02 PM3/22/10

to

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

Cong Wang

unread,

Mar 22, 2010, 9:50:02 PM3/22/10

to

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.

David Miller

unread,

Mar 22, 2010, 11:10:01 PM3/22/10

to

From: Neil Horman <nho...@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 <amw...@redhat.com>

...
> Acked-by: Neil Horman <nho...@tuxdriver.com>

Applied, thanks!

Matt Mackall

unread,

Mar 23, 2010, 12:30:02 AM3/23/10

to

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

Cong Wang

unread,

Mar 23, 2010, 12:40:01 AM3/23/10

to

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);

Matt Mackall

unread,

Mar 23, 2010, 12:50:02 AM3/23/10

to

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

Cong Wang

unread,

Mar 23, 2010, 1:00:03 AM3/23/10

to

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.