You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Sebastien Lorquet <se...@lorquet.fr> on 2022/06/08 22:45:04 UTC
Suggestion to turn a nwarn into an ninfo in ipv4_input
Hi,
Currently in ipv4_input.c we have a warning that a packet was not for us
and was dropped. This pollutes the output of the console for no real
value when connected to a real network.
I suggest we turn this warning into an info to keep things clean.
The file contains real warnings that are much more important and
relevant that this line.
Could be configurable (with a potential CONFIG_NET_IP_NOTFORUS_WARN) or
fixed, here is the relevant code.
I can send a PR but I would like your general feeling on this issue
first, thank you!
Sebastien
diff --git a/net/devif/ipv4_input.c b/net/devif/ipv4_input.c
index bb16940cbf..bb109f353d 100644
--- a/net/devif/ipv4_input.c
+++ b/net/devif/ipv4_input.c
@@ -310,8 +310,13 @@ int ipv4_input(FAR struct net_driver_s *dev)
* packet.
*/
+#ifdef CONFIG_NET_IP_NOTFORUS_WARN
nwarn("WARNING: Not destined for us; not forwardable... "
"Dropping!\n");
+#else
+ ninfo("WARNING: Not destined for us; not forwardable... "
+ "Dropping!\n");
+#endif
#ifdef CONFIG_NET_STATISTICS
g_netstats.ipv4.drop++;
Re: Suggestion to turn a nwarn into an ninfo in ipv4_input
Posted by Abdelatif Guettouche <ab...@gmail.com>.
+1 for making it an ninfo (without the extra option).
On Thu, Jun 9, 2022 at 8:45 AM Michael Jung <mi...@secore.ly> wrote:
>
> Hi Sebastien,
>
> I have this warning degraded to an informational message in my local
> repository for the exact same reason. In my opinion it should just be
> changed to informational. I.e. no need to make this configurable.
>
> Bye,
> Michael
>
> On Thu, Jun 9, 2022 at 12:45 AM Sebastien Lorquet <se...@lorquet.fr>
> wrote:
>
> > Hi,
> >
> > Currently in ipv4_input.c we have a warning that a packet was not for us
> > and was dropped. This pollutes the output of the console for no real
> > value when connected to a real network.
> >
> > I suggest we turn this warning into an info to keep things clean.
> >
> > The file contains real warnings that are much more important and
> > relevant that this line.
> >
> >
> > Could be configurable (with a potential CONFIG_NET_IP_NOTFORUS_WARN) or
> > fixed, here is the relevant code.
> >
> > I can send a PR but I would like your general feeling on this issue
> > first, thank you!
> >
> > Sebastien
> >
> >
> > diff --git a/net/devif/ipv4_input.c b/net/devif/ipv4_input.c
> > index bb16940cbf..bb109f353d 100644
> > --- a/net/devif/ipv4_input.c
> > +++ b/net/devif/ipv4_input.c
> > @@ -310,8 +310,13 @@ int ipv4_input(FAR struct net_driver_s *dev)
> > * packet.
> > */
> >
> > +#ifdef CONFIG_NET_IP_NOTFORUS_WARN
> > nwarn("WARNING: Not destined for us; not forwardable... "
> > "Dropping!\n");
> > +#else
> > + ninfo("WARNING: Not destined for us; not forwardable... "
> > + "Dropping!\n");
> > +#endif
> >
> > #ifdef CONFIG_NET_STATISTICS
> > g_netstats.ipv4.drop++;
> >
> >
Re: Suggestion to turn a nwarn into an ninfo in ipv4_input
Posted by Sebastien Lorquet <se...@lorquet.fr>.
Here it is, I hope it does not take two full days for merging :)
https://github.com/apache/incubator-nuttx/pull/6399
Sebastien
Le 09/06/2022 à 14:30, Sebastien Lorquet a écrit :
> Hello,
>
> I also agree an option is useless. I was just suggesting this in case
> some people still want to see it as warning.
>
> I'll send a pull request a bit later.
>
> Thanks for the feedback!
>
> Sebastien
>
> Le 09/06/2022 à 14:17, Alan Carvalho de Assis a écrit :
>> I completely agree!
>>
>> We need to reduce the number of configurations, not increase it.
>>
>> Just converting it to ninfo() is enough.
>>
>> Other OS has a good documentation to educate developers to avoid
>> creating unnecessary Kconfig entries:
>>
>> https://doc.riot-os.org/kconfig-in-riot.html
>> https://docs.zephyrproject.org/2.6.0/guides/kconfig/tips.html
>>
>> BR,
>>
>> Alan
>>
>> On 6/9/22, Michael Jung <mi...@secore.ly> wrote:
>>> Hi Sebastien,
>>>
>>> I have this warning degraded to an informational message in my local
>>> repository for the exact same reason. In my opinion it should just be
>>> changed to informational. I.e. no need to make this configurable.
>>>
>>> Bye,
>>> Michael
>>>
>>> On Thu, Jun 9, 2022 at 12:45 AM Sebastien Lorquet
>>> <se...@lorquet.fr>
>>> wrote:
>>>
>>>> Hi,
>>>>
>>>> Currently in ipv4_input.c we have a warning that a packet was not
>>>> for us
>>>> and was dropped. This pollutes the output of the console for no real
>>>> value when connected to a real network.
>>>>
>>>> I suggest we turn this warning into an info to keep things clean.
>>>>
>>>> The file contains real warnings that are much more important and
>>>> relevant that this line.
>>>>
>>>>
>>>> Could be configurable (with a potential
>>>> CONFIG_NET_IP_NOTFORUS_WARN) or
>>>> fixed, here is the relevant code.
>>>>
>>>> I can send a PR but I would like your general feeling on this issue
>>>> first, thank you!
>>>>
>>>> Sebastien
>>>>
>>>>
>>>> diff --git a/net/devif/ipv4_input.c b/net/devif/ipv4_input.c
>>>> index bb16940cbf..bb109f353d 100644
>>>> --- a/net/devif/ipv4_input.c
>>>> +++ b/net/devif/ipv4_input.c
>>>> @@ -310,8 +310,13 @@ int ipv4_input(FAR struct net_driver_s *dev)
>>>> * packet.
>>>> */
>>>>
>>>> +#ifdef CONFIG_NET_IP_NOTFORUS_WARN
>>>> nwarn("WARNING: Not destined for us; not
>>>> forwardable... "
>>>> "Dropping!\n");
>>>> +#else
>>>> + ninfo("WARNING: Not destined for us; not
>>>> forwardable... "
>>>> + "Dropping!\n");
>>>> +#endif
>>>>
>>>> #ifdef CONFIG_NET_STATISTICS
>>>> g_netstats.ipv4.drop++;
>>>>
>>>>
Re: Suggestion to turn a nwarn into an ninfo in ipv4_input
Posted by Sebastien Lorquet <se...@lorquet.fr>.
Hello,
I also agree an option is useless. I was just suggesting this in case
some people still want to see it as warning.
I'll send a pull request a bit later.
Thanks for the feedback!
Sebastien
Le 09/06/2022 à 14:17, Alan Carvalho de Assis a écrit :
> I completely agree!
>
> We need to reduce the number of configurations, not increase it.
>
> Just converting it to ninfo() is enough.
>
> Other OS has a good documentation to educate developers to avoid
> creating unnecessary Kconfig entries:
>
> https://doc.riot-os.org/kconfig-in-riot.html
> https://docs.zephyrproject.org/2.6.0/guides/kconfig/tips.html
>
> BR,
>
> Alan
>
> On 6/9/22, Michael Jung <mi...@secore.ly> wrote:
>> Hi Sebastien,
>>
>> I have this warning degraded to an informational message in my local
>> repository for the exact same reason. In my opinion it should just be
>> changed to informational. I.e. no need to make this configurable.
>>
>> Bye,
>> Michael
>>
>> On Thu, Jun 9, 2022 at 12:45 AM Sebastien Lorquet <se...@lorquet.fr>
>> wrote:
>>
>>> Hi,
>>>
>>> Currently in ipv4_input.c we have a warning that a packet was not for us
>>> and was dropped. This pollutes the output of the console for no real
>>> value when connected to a real network.
>>>
>>> I suggest we turn this warning into an info to keep things clean.
>>>
>>> The file contains real warnings that are much more important and
>>> relevant that this line.
>>>
>>>
>>> Could be configurable (with a potential CONFIG_NET_IP_NOTFORUS_WARN) or
>>> fixed, here is the relevant code.
>>>
>>> I can send a PR but I would like your general feeling on this issue
>>> first, thank you!
>>>
>>> Sebastien
>>>
>>>
>>> diff --git a/net/devif/ipv4_input.c b/net/devif/ipv4_input.c
>>> index bb16940cbf..bb109f353d 100644
>>> --- a/net/devif/ipv4_input.c
>>> +++ b/net/devif/ipv4_input.c
>>> @@ -310,8 +310,13 @@ int ipv4_input(FAR struct net_driver_s *dev)
>>> * packet.
>>> */
>>>
>>> +#ifdef CONFIG_NET_IP_NOTFORUS_WARN
>>> nwarn("WARNING: Not destined for us; not forwardable... "
>>> "Dropping!\n");
>>> +#else
>>> + ninfo("WARNING: Not destined for us; not forwardable... "
>>> + "Dropping!\n");
>>> +#endif
>>>
>>> #ifdef CONFIG_NET_STATISTICS
>>> g_netstats.ipv4.drop++;
>>>
>>>
Re: Suggestion to turn a nwarn into an ninfo in ipv4_input
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
I completely agree!
We need to reduce the number of configurations, not increase it.
Just converting it to ninfo() is enough.
Other OS has a good documentation to educate developers to avoid
creating unnecessary Kconfig entries:
https://doc.riot-os.org/kconfig-in-riot.html
https://docs.zephyrproject.org/2.6.0/guides/kconfig/tips.html
BR,
Alan
On 6/9/22, Michael Jung <mi...@secore.ly> wrote:
> Hi Sebastien,
>
> I have this warning degraded to an informational message in my local
> repository for the exact same reason. In my opinion it should just be
> changed to informational. I.e. no need to make this configurable.
>
> Bye,
> Michael
>
> On Thu, Jun 9, 2022 at 12:45 AM Sebastien Lorquet <se...@lorquet.fr>
> wrote:
>
>> Hi,
>>
>> Currently in ipv4_input.c we have a warning that a packet was not for us
>> and was dropped. This pollutes the output of the console for no real
>> value when connected to a real network.
>>
>> I suggest we turn this warning into an info to keep things clean.
>>
>> The file contains real warnings that are much more important and
>> relevant that this line.
>>
>>
>> Could be configurable (with a potential CONFIG_NET_IP_NOTFORUS_WARN) or
>> fixed, here is the relevant code.
>>
>> I can send a PR but I would like your general feeling on this issue
>> first, thank you!
>>
>> Sebastien
>>
>>
>> diff --git a/net/devif/ipv4_input.c b/net/devif/ipv4_input.c
>> index bb16940cbf..bb109f353d 100644
>> --- a/net/devif/ipv4_input.c
>> +++ b/net/devif/ipv4_input.c
>> @@ -310,8 +310,13 @@ int ipv4_input(FAR struct net_driver_s *dev)
>> * packet.
>> */
>>
>> +#ifdef CONFIG_NET_IP_NOTFORUS_WARN
>> nwarn("WARNING: Not destined for us; not forwardable... "
>> "Dropping!\n");
>> +#else
>> + ninfo("WARNING: Not destined for us; not forwardable... "
>> + "Dropping!\n");
>> +#endif
>>
>> #ifdef CONFIG_NET_STATISTICS
>> g_netstats.ipv4.drop++;
>>
>>
>
Re: Suggestion to turn a nwarn into an ninfo in ipv4_input
Posted by Michael Jung <mi...@secore.ly>.
Hi Sebastien,
I have this warning degraded to an informational message in my local
repository for the exact same reason. In my opinion it should just be
changed to informational. I.e. no need to make this configurable.
Bye,
Michael
On Thu, Jun 9, 2022 at 12:45 AM Sebastien Lorquet <se...@lorquet.fr>
wrote:
> Hi,
>
> Currently in ipv4_input.c we have a warning that a packet was not for us
> and was dropped. This pollutes the output of the console for no real
> value when connected to a real network.
>
> I suggest we turn this warning into an info to keep things clean.
>
> The file contains real warnings that are much more important and
> relevant that this line.
>
>
> Could be configurable (with a potential CONFIG_NET_IP_NOTFORUS_WARN) or
> fixed, here is the relevant code.
>
> I can send a PR but I would like your general feeling on this issue
> first, thank you!
>
> Sebastien
>
>
> diff --git a/net/devif/ipv4_input.c b/net/devif/ipv4_input.c
> index bb16940cbf..bb109f353d 100644
> --- a/net/devif/ipv4_input.c
> +++ b/net/devif/ipv4_input.c
> @@ -310,8 +310,13 @@ int ipv4_input(FAR struct net_driver_s *dev)
> * packet.
> */
>
> +#ifdef CONFIG_NET_IP_NOTFORUS_WARN
> nwarn("WARNING: Not destined for us; not forwardable... "
> "Dropping!\n");
> +#else
> + ninfo("WARNING: Not destined for us; not forwardable... "
> + "Dropping!\n");
> +#endif
>
> #ifdef CONFIG_NET_STATISTICS
> g_netstats.ipv4.drop++;
>
>