[RFC] [WIP] incorporate picotcp into barebox: a small demo

[RFC] [WIP] incorporate picotcp into barebox: a small demo

[Antony Pavlov]

Hi all!

I have adapted barebox for work with picotcp network stack.

Picotcp is not a small piece of code so I can't easyly send
a patch to the barebox maillist. I have put results of my work on github,
see mini-howto below.

This picotcp integration is a dirty hack in many ways.
We need additional effors for adapting barebox and picotcp
for more easy joint operation.

Please express your opinion on my work. I'm awaiting your comments.

Many thanks to Alessandro Rubini for his pointing on picotcp!


barebox picotcp mini-howto
==========================

1. clone and build sandbox barebox

  $ git clone https://github.com/frantony/barebox -b picotcp.20140525
  ...
  $ make sandbox_defconfig
  ...
  $ make

2.  run sandbox barebox

  # ./barebox

3.  Inside sandbox barebox initialize the 'eth' network interface
and check routes:

  barebox@barebox sandbox:/ ifconfig eth 10.0.0.1 255.255.255.0
  Assigned ipv4 10.0.0.1 to device eth
  barebox@barebox sandbox:/ route
  picotcp IPv4 routing table
  Destination     Gateway         Genmask         Metric Iface
  10.0.0.0        0.0.0.0         255.255.255.0   1      eth

4. on host initialize the 'barebox' tap interface (you may need
root account for this and bellow operations on host):

  # ifconfig barebox 10.0.0.2

and check barebox accessibility:

  # ping -c 1 10.0.0.1
  PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
  64 bytes from 10.0.0.1: icmp_seq=1 ttl=64 time=0.526 ms

  --- 10.0.0.1 ping statistics ---
  1 packets transmitted, 1 received, 0% packet loss, time 0ms
  rtt min/avg/max/mdev = 0.526/0.526/0.526/0.000 ms

5. inside sandbox barebox check host accessibility.
Note: use picoping, not ping!

  barebox@barebox sandbox:/ picoping 10.0.0.2
  barebox@barebox sandbox:/ 48 bytes from 10.0.0.2: icmp_req=1 ttl=64 time=304 ms
  48 bytes from 10.0.0.2: icmp_req=2 ttl=64 time=0 ms
  48 bytes from 10.0.0.2: icmp_req=3 ttl=64 time=0 ms
  48 bytes from 10.0.0.2: icmp_req=4 ttl=64 time=0 ms
  48 bytes from 10.0.0.2: icmp_req=5 ttl=64 time=0 ms
  48 bytes from 10.0.0.2: icmp_req=6 ttl=64 time=0 ms
  48 bytes from 10.0.0.2: icmp_req=7 ttl=64 time=0 ms
  48 bytes from 10.0.0.2: icmp_req=8 ttl=64 time=0 ms
  48 bytes from 10.0.0.2: icmp_req=9 ttl=64 time=0 ms
  48 bytes from 10.0.0.2: icmp_req=10 ttl=64 time=0 ms

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Daniele Lacamera]

On Sun, May 25, 2014 at 11:58 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> Hi all!
>
> I have adapted barebox for work with picotcp network stack.
>

Hello Antony,

Great news! Thanks for announcing this. Here is a few comments from our side.

I've taken a look at the code. I like the general approach, and it
seems that some functionality could already be replaced with picotcp
modules and components (e.g. ARP, DHCP, DNS.), and some new can easily
be integrated if the project would benefit of some other service
available from cli, such as SLAAC, sNTP, mDNS, DHCP server, etc.

>  We need additional effors for adapting barebox and picotcp
>  for more easy joint operation.


Regarding this, we would like to help with a cleaner integration, so
feel free to submit requests in the form of "issues" on our github if
you need to introduce new pico_err's, or any suggestion you might have
regarding the APIs for the standalone tool support.

The whole PicoTCP team is very enthusiast about what you guys started,
and we keep ourselves at your disposal for whatever you might need
from our side.

I have to add, it is possibly interesting for barebox community, we
recently added the support for blocking sockets with a POSIX-compliant
API, which would make socket development much easier if properly
connected to the execution model. You might want to have a look at
this repository we just created:

https://github.com/tass-belgium/picotcp-bsd

Last but not least, I would love to see some ipv6 integration, as soon
as your interface is in the expected status, I could try and add the
support for ipv6 addresses and routing myself, even if I don't know
what are the plans of barebox regarding this.

Kudos to you all for this great initiative, and a special "thank you"
to Alessandro for endorsing our free TCP/IP stack :)

Have a nice day

--

Daniele Lacamera
Senior Software Architect
TASS.BE  - Part of Altran

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Lucas Stach]

Am Sonntag, den 25.05.2014, 13:58 +0400 schrieb Antony Pavlov:
> Hi all!
> 
> I have adapted barebox for work with picotcp network stack.
> 
> Picotcp is not a small piece of code so I can't easyly send
> a patch to the barebox maillist. I have put results of my work on github,
> see mini-howto below.
> 
> This picotcp integration is a dirty hack in many ways.
> We need additional effors for adapting barebox and picotcp
> for more easy joint operation.
> 
> Please express your opinion on my work. I'm awaiting your comments.
> 
You forgot to mention one important thing: Why is this change beneficial
to barebox?
Please don't get me wrong this is in no way a criticism on your work. I
only skimmed through the branch as of now and can't really comment on
the change. So please help me out: do you feel the code is
leaner/cleaner than the existing barebox network support? Or what's your
motivation for this?

Regards,
Lucas


-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Antony Pavlov]

On Mon, 26 May 2014 11:45:57 +0200
Lucas Stach <l.stach@pengutronix.de> wrote:

> Am Sonntag, den 25.05.2014, 13:58 +0400 schrieb Antony Pavlov:
> > Hi all!
> > 
> > I have adapted barebox for work with picotcp network stack.
> > 
> > Picotcp is not a small piece of code so I can't easyly send
> > a patch to the barebox maillist. I have put results of my work on github,
> > see mini-howto below.
> > 
> > This picotcp integration is a dirty hack in many ways.
> > We need additional effors for adapting barebox and picotcp
> > for more easy joint operation.
> > 
> > Please express your opinion on my work. I'm awaiting your comments.
> > 
> You forgot to mention one important thing: Why is this change beneficial
> to barebox?

1. For end user barebox in many ways behave like linux shell console

   We have files, standard shell commands, environment etc. It is very convenient!

   But current network code breaks this illusion (e.g. you can't ping a barebox running board).
   The details of network subsystem realisation shape barebox user network modus operandi.

   Adding full-grown but tiny network stack to barebox makes barebox' behaviour
   (from user's point of view) more close to linux shell console.

2. tcp

   IMHO __optional__ TCP support can be beneficial for bootloader.
   E.g. I would like to use widely used telnet protocol for network console.

   See also U-boot mod (https://forum.openwrt.org/viewtopic.php?id=43237),
   http-server enabled U-Boot, less than 64K!

3. ipv6

   Current IPv4 address space is near totaly exhausted
   (see https://www.icann.org/news/announcement-2-2014-05-20-en).
   I suppose with the lapse of time IPv6 will be used even in bootloaders :)
   picotcp gives you IPv6 just now.

4. several simultaneously running network interfaces support

   Imagine a small cluster system.
   The processors of this cluster are connected via special interconnect, and only one
   "master" processor has ethernet connection to the surrounding world. with current
   "single active network device" conception you can't use barebox for connecting "slave" cluster'
   processors to the surrounding world using "master" processor as a gateway.

> Please don't 4get me wrong this is in no way a criticism on your work.

My current picotcp integration is a quick-and-dirty hack. It is below criticism!

> I only skimmed through the branch as of now and can't really comment on
> the change. So please help me out: do you feel the code is
> leaner/cleaner than the existing barebox network support?

It is very difficult to compare existing barebox (u-boot) network code and picotcp.
Picotcp is much more capable!

Picotcp need some work for cleaning. I think that on average barebox code is more clean
that picotcp code (there are too many #ifdefs, some compiler warnings, formatting),
but IMHO it is not very difficult to make it cleaner.

> Or what's your motivation for this?

See my points above. In the end adding full-grown network stack will make barebox
more favourable for users:

  1. new barebox users with linux shell experience will be happy to see habitual 'ifconfig',
'route' commands instead of manipulations with environment variables.

  2. new network stack eventually can result in new applications for barebox.

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Sascha Hauer]

On Mon, May 26, 2014 at 04:09:33PM +0400, Antony Pavlov wrote:
> On Mon, 26 May 2014 11:45:57 +0200
> Lucas Stach <l.stach@pengutronix.de> wrote:
> 
> 
> 1. For end user barebox in many ways behave like linux shell console
> 
>    We have files, standard shell commands, environment etc. It is very convenient!
> 
>    But current network code breaks this illusion (e.g. you can't ping a barebox running board).
>    The details of network subsystem realisation shape barebox user network modus operandi.
> 
>    Adding full-grown but tiny network stack to barebox makes barebox' behaviour
>    (from user's point of view) more close to linux shell console.
> 
> 2. tcp
> 
>    IMHO __optional__ TCP support can be beneficial for bootloader.
>    E.g. I would like to use widely used telnet protocol for network console.
> 
>    See also U-boot mod (https://forum.openwrt.org/viewtopic.php?id=43237),
>    http-server enabled U-Boot, less than 64K!
> 
> 3. ipv6
> 
>    Current IPv4 address space is near totaly exhausted
>    (see https://www.icann.org/news/announcement-2-2014-05-20-en).
>    I suppose with the lapse of time IPv6 will be used even in bootloaders :)
>    picotcp gives you IPv6 just now.

These features sound very nice. I hope we can get the binary size
impacts within sensible limits. Is it possible to disable TCP support in
picotcp?

> 
> 4. several simultaneously running network interfaces support
> 
>    Imagine a small cluster system.
>    The processors of this cluster are connected via special interconnect, and only one
>    "master" processor has ethernet connection to the surrounding world. with current
>    "single active network device" conception you can't use barebox for connecting "slave" cluster'
>    processors to the surrounding world using "master" processor as a gateway.

That sound more like you should start Linux earlier.

An integration of picotcp which allows to play with real hardware would
be great, even if it's quick and dirty. That would allow us to see the
size impact and the behaviour of the stack without interrupts.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Daniele Lacamera]

On Tue, May 27, 2014 at 7:30 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

>> 3. ipv6
>>
>>    Current IPv4 address space is near totaly exhausted
>>    (see https://www.icann.org/news/announcement-2-2014-05-20-en).
>>    I suppose with the lapse of time IPv6 will be used even in bootloaders :)
>>    picotcp gives you IPv6 just now.
>
> These features sound very nice. I hope we can get the binary size
> impacts within sensible limits. Is it possible to disable TCP support in
> picotcp?
>

Yes, you can disable each single component. E.g., to compile TCP out,
use TCP=0, or just leave PICO_SUPPORT_TCP out. Some binary size
figures for picotcp (compiled for arm, with -Os, symbols stripped):

    1433      pico_arp.o (ex ./build/lib/libpicotcp.a)
    1715      pico_dev_loop.o (ex ./build/lib/libpicotcp.a)
    2956      pico_dhcp_client.o (ex ./build/lib/libpicotcp.a)
     284      pico_dhcp_common.o (ex ./build/lib/libpicotcp.a)
    1152      pico_dhcp_server.o (ex ./build/lib/libpicotcp.a)
    2340      pico_dns_client.o (ex ./build/lib/libpicotcp.a)
    1170      pico_icmp4.o (ex ./build/lib/libpicotcp.a)
    1897      pico_icmp6.o (ex ./build/lib/libpicotcp.a)
    3624      pico_igmp.o (ex ./build/lib/libpicotcp.a)
     660      pico_ipfilter.o (ex ./build/lib/libpicotcp.a)
    5560      pico_ipv4.o (ex ./build/lib/libpicotcp.a)
    1216      pico_ipv6_nd.o (ex ./build/lib/libpicotcp.a)
    4089      pico_ipv6.o (ex ./build/lib/libpicotcp.a)
    2476      pico_mdns.o (ex ./build/lib/libpicotcp.a)
    1428      pico_nat.o (ex ./build/lib/libpicotcp.a)
    3294      pico_olsr.o (ex ./build/lib/libpicotcp.a)
     612      pico_slaacv4.o (ex ./build/lib/libpicotcp.a)
     928      pico_sntp_client.o (ex ./build/lib/libpicotcp.a)
     766      pico_socket_tcp.o (ex ./build/lib/libpicotcp.a)
     644      pico_socket_udp.o (ex ./build/lib/libpicotcp.a)
   10542      pico_tcp.o (ex ./build/lib/libpicotcp.a)
     684      pico_udp.o (ex ./build/lib/libpicotcp.a)
    1375      pico_device.o (ex ./build/lib/libpicotcp.a)
     434      pico_frame.o (ex ./build/lib/libpicotcp.a)
     601      pico_protocol.o (ex ./build/lib/libpicotcp.a)
    2636      pico_socket_multicast.o (ex ./build/lib/libpicotcp.a)
    5018      pico_socket.o (ex ./build/lib/libpicotcp.a)
    3079      pico_stack.o (ex ./build/lib/libpicotcp.a)
    1188      pico_tree.o (ex ./build/lib/libpicotcp.a)
   63801      (TOTALS)

/d

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Daniele Lacamera]

Antony, regarding your comments on picotcp "cleaning", could you
please elaborate a bit more, taking into account my comments below?

On Mon, May 26, 2014 at 2:09 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> Picotcp need some work for cleaning. I think that on average barebox code is more clean
> that picotcp code (there are too many #ifdefs, some compiler warnings, formatting),
> but IMHO it is not very difficult to make it cleaner.
>

- The amount of #ifdefs in the code is due to the size optimizations.
Types are left out when modules are disabled, would be much more
difficult to achieve the same code size, e.g. on a 8-bit machine, if
we used empty proxies instead, like for instance Linux does. Keep in
mind that saving a few bytes is the key for some of our projects, so I
would accept no modification for the sack of aesthetic if it would
impact on code size. Our quality processes ensure that the branching
is kept under control, and our continuous integration takes into
account enabling and disabling the modules.

- compiler warnings: AFAIK, our code is warning free on gcc, it might
be that a specific platform config could trigger some. We are
interested about your experience, please share your findings, but
please do not report -Wshadow warnings obtained with broken gcc (<=
4.6).
Our default set of warning flags:  -Wall -Wdeclaration-after-statement
-W -Wextra -Wshadow -Wcast-qual -Wwrite-strings
-Wmissing-field-initializers -Wconversion -Wcast-align

- formatting: Could it be that you checked an intermediate
masterbranch version? We run uncrustify periodically on the code, and
our formatting is consistent with our own rules.

Thanks


/d

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Antony Pavlov]

On Tue, 27 May 2014 11:46:29 +0200
Daniele Lacamera <daniele.lacamera@tass.be> wrote:

> Antony, regarding your comments on picotcp "cleaning", could you
> please elaborate a bit more, taking into account my comments below?
> 
> On Mon, May 26, 2014 at 2:09 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > Picotcp need some work for cleaning. I think that on average barebox code is more clean
> > that picotcp code (there are too many #ifdefs, some compiler warnings, formatting),
> > but IMHO it is not very difficult to make it cleaner.
> >
> 
> - The amount of #ifdefs in the code is due to the size optimizations.
> Types are left out when modules are disabled, would be much more
> difficult to achieve the same code size, e.g. on a 8-bit machine, if
> we used empty proxies instead, like for instance Linux does. Keep in
> mind that saving a few bytes is the key for some of our projects, so I
> would accept no modification for the sack of aesthetic if it would
> impact on code size. Our quality processes ensure that the branching
> is kept under control, and our continuous integration takes into
> account enabling and disabling the modules.

linux kernel and barebox use not so much #ifdef's because they use
they use IS_ENABLED() macro with just the same result.

Please see linux.git/drivers/gpu/drm/tegra/dsi.c
(https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/tegra/dsi.c#L628):

        if (IS_ENABLED(CONFIG_DEBUG_FS)) {
                err = tegra_dsi_debugfs_init(dsi, tegra->drm->primary);
                if (err < 0)
                        dev_err(dsi->dev, "debugfs setup failed: %d\n", err);
        }

so the code under IS_ENABLED() is always parsed by C compiler;
if there is an syntax error in this code then the compilation will be stopped.

In the examople above, if CONFIG_DEBUG_FS is not activated then
clever optimizing compiler will drop all code under IS_ENABLED().

If you use #ifdef:

        #ifdef CONFIG_DEBUG_FS
                err = tegra_dsi_debugfs_init(dsi, tegra->drm->primary);
                if (err < 0)
                        dev_err(dsi->dev, "debugfs setup failed: %d\n", err);
        #endif
   
then you get jut the same result but without checking the code under #ifdef CONFIG_DEBUG_FS.

> - compiler warnings: AFAIK, our code is warning free on gcc, it might
> be that a specific platform config could trigger some. We are
> interested about your experience, please share your findings, but
> please do not report -Wshadow warnings obtained with broken gcc (<=
> 4.6).
> Our default set of warning flags:  -Wall -Wdeclaration-after-statement
> -W -Wextra -Wshadow -Wcast-qual -Wwrite-strings
> -Wmissing-field-initializers -Wconversion -Wcast-align

$ gcc --version
gcc (Debian 4.8.3-1) 4.8.3

Here is my 'make -s' output for sandbox barebox:

net/picotcp/modules/pico_ipv4.c: In function ‘pico_ipv4_rebound_large’:
net/picotcp/modules/pico_ipv4.c:1582:24: warning: unused variable ‘fr’ [-Wunused-variable]
     struct pico_frame *fr;
                        ^
net/picotcp/modules/pico_ipv4.c:1581:14: warning: unused variable ‘len’ [-Wunused-variable]
     uint32_t len = f->transport_len;
              ^
net/picotcp/modules/pico_ipv4.c:1580:14: warning: unused variable ‘total_payload_written’ [-Wunused-variable]
     uint32_t total_payload_written = 0;
              ^
net/picotcp/stack/pico_socket.c: In function ‘pico_socket_add’:
net/picotcp/stack/pico_socket.c:397:5: warning: "DEBUG_SOCKET_TREE" is not defined [-Wundef]
 #if DEBUG_SOCKET_TREE
     ^
net/picotcp/stack/pico_socket.c: At top level:
net/picotcp/stack/pico_socket.c:1037:13: warning: ‘pico_socket_xmit_first_fragment_setup’ defined but not used [-Wunused-function]
 static void pico_socket_xmit_first_fragment_setup(struct pico_frame *f, int space, int hdr_offset)
             ^
net/picotcp/stack/pico_socket.c:1049:13: warning: ‘pico_socket_xmit_next_fragment_setup’ defined but not used [-Wunused-function]
 static void pico_socket_xmit_next_fragment_setup(struct pico_frame *f, int hdr_offset, int total_payload_written, int len)
             ^
net/test_picotcp.c: In function ‘do_test_ipv4’:
net/test_picotcp.c:51:48: warning: comparison between pointer and integer [enabled by default]
         fail_unless(pico_ipv4_link_find(&a[i]) == dev[i], "Error finding link");
                                                ^
net/test_picotcp.c:11:6: note: in definition of macro ‘fail_if’
  if (a) { \
      ^
net/test_picotcp.c:51:9: note: in expansion of macro ‘fail_unless’
         fail_unless(pico_ipv4_link_find(&a[i]) == dev[i], "Error finding link");
         ^

> 
> - formatting: Could it be that you checked an intermediate
> masterbranch version? We run uncrustify periodically on the code, and
> our formatting is consistent with our own rules.


Yes, I see your 'Enforced coding style via uncrustify' commit.

But in linux kernel, barebox, qemu and IMHO many other projects
there are:

  * CODING_STYLE documentation file;
  * scripts/checkpatch.pl

The scripts/checkpatch.pl is used by project maintainer on git patches
before applying them, so there is very small possibility for
bad formatted patches to penetrate into repository.

There is even cruel practice running checkpatch.pl script via git pre-commit
hook so one can't even locally commit bad formatted changes!
(see http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html)


-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Daniele Lacamera]

Antony, thank you very much for the comments and the bug reports on
our github. See comments below.

On Tue, May 27, 2014 at 4:04 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> On Tue, 27 May 2014 11:46:29 +0200
> Daniele Lacamera <daniele.lacamera@tass.be> wrote:
>
>
> linux kernel and barebox use not so much #ifdef's because they use
> they use IS_ENABLED() macro with just the same result.
[...]
> so the code under IS_ENABLED() is always parsed by C compiler;
> if there is an syntax error in this code then the compilation will be stopped.

We will discuss this and see if it is applicable for PicoTCP. The
macro you are suggesting is indeed not increasing code size, but our
static checkers might be not so happy about all this preprocessor
machinery.

>
> $ gcc --version
> gcc (Debian 4.8.3-1) 4.8.3
>
> Here is my 'make -s' output for sandbox barebox:
>
[cut]

Thank You!! This *IS* very helpful input. We are going to add more -W
flags and quickly fix these warnings.

> But in linux kernel, barebox, qemu and IMHO many other projects
> there are:
>
>   * CODING_STYLE documentation file;
>   * scripts/checkpatch.pl
>

All good points. Being a kernel developer myself, I know the
checkpatch.pl approach, and indeed it is a good thing to distribute
coding rules if you accept contributions. Except it will never apply
to us, because:

- We have our company internal coding style which we apply to all our
projects and we don't feel the need to distribute externally (keep in
mind that we never planned to accept external contributions). Instead,
the repository contains the uncrustify configuration file, which is
pretty much self-explanatory.
- A few of us are checking every single commit, and running uncrustify
every now and then. We are comfortable with this approach
- Linux coding rules are generally recognized as possibly not the best
in the world. I especially dislike tabs.
- commit hooks are tedious

I am afraid we are not going to change our formatting style or policy,
but thanks anyway for the suggestions!

/d

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Sascha Hauer]

On Tue, May 27, 2014 at 06:04:11PM +0400, Antony Pavlov wrote:
> On Tue, 27 May 2014 11:46:29 +0200
> Daniele Lacamera <daniele.lacamera@tass.be> wrote:
> 
> $ gcc --version
> gcc (Debian 4.8.3-1) 4.8.3
> 
> Here is my 'make -s' output for sandbox barebox:
> 
> net/picotcp/modules/pico_ipv4.c: In function ‘pico_ipv4_rebound_large’:
> net/picotcp/modules/pico_ipv4.c:1582:24: warning: unused variable ‘fr’ [-Wunused-variable]
>      struct pico_frame *fr;
>                         ^
> net/picotcp/modules/pico_ipv4.c:1581:14: warning: unused variable ‘len’ [-Wunused-variable]
>      uint32_t len = f->transport_len;
>               ^
> net/picotcp/modules/pico_ipv4.c:1580:14: warning: unused variable ‘total_payload_written’ [-Wunused-variable]
>      uint32_t total_payload_written = 0;
>               ^

That's another advantage of IS_ENABLED. This happens because the variables are
only used under an ifdef. Fixing this would mean to put the definitions under
the same ifdef which requires another ifdef.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Juergen Borleis]

Hi Antony,

On Tuesday 27 May 2014 16:04:11 Antony Pavlov wrote:
> [...]
> If you use #ifdef:
>
>         #ifdef CONFIG_DEBUG_FS
>                 err = tegra_dsi_debugfs_init(dsi, tegra->drm->primary);
>                 if (err < 0)
>                         dev_err(dsi->dev, "debugfs setup failed: %d\n", err);
>         #endif 
>    
> then you get jut the same result but without checking the code under #ifdef
> CONFIG_DEBUG_FS.

A syntax check is always done by the pre-processor. So, the code between the
#ifdef/#endif must be valid C code or be commented out.
But there is no further check, because the lines between the #ifdef/#endif are
never seen by the compiler.

jbe

-- 
Pengutronix e.K.                              | Juergen Borleis             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Antony Pavlov]

On Wed, 28 May 2014 09:23:30 +0200
Juergen Borleis <jbe@pengutronix.de> wrote:

> Hi Antony,
> 
> On Tuesday 27 May 2014 16:04:11 Antony Pavlov wrote:
> > [...]
> > If you use #ifdef:
> >
> >         #ifdef CONFIG_DEBUG_FS
> >                 err = tegra_dsi_debugfs_init(dsi, tegra->drm->primary);
> >                 if (err < 0)
> >                         dev_err(dsi->dev, "debugfs setup failed: %d\n", err);
> >         #endif 
> >    
> > then you get jut the same result but without checking the code under #ifdef
> > CONFIG_DEBUG_FS.
> 
> A syntax check is always done by the pre-processor. So, the code between the

Yes, the C pre-processor checks the syntax for input, but only the C pre-procesor directive syntax!
But here I mean exactly C code checking.

> #ifdef/#endif must be valid C code or be commented out.
> But there is no further check, because the lines between the #ifdef/#endif are
> never seen by the compiler.
> 

That's exactly what I mean! E.g.

antony@doce:~$ cat > test.c <<EOF;
int main()
{
#ifdef SOME_UNDEFINED_MACRO
        foo bar;
        фу бар;
#endif
	return 0;
}
EOF
antony@doce:~$ gcc test.c 
antony@doce:~$ ./a.out 
antony@doce:~$ ./a.out 
antony@doce:~$ echo $
0

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Antony Pavlov]

On Tue, 27 May 2014 19:26:14 +0200
Daniele Lacamera <daniele.lacamera@tass.be> wrote:

> Antony, thank you very much for the comments and the bug reports on
> our github. See comments below.
> 
> On Tue, May 27, 2014 at 4:04 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > On Tue, 27 May 2014 11:46:29 +0200
> > Daniele Lacamera <daniele.lacamera@tass.be> wrote:
> >
> >
> > linux kernel and barebox use not so much #ifdef's because they use
> > they use IS_ENABLED() macro with just the same result.
> [...]
> > so the code under IS_ENABLED() is always parsed by C compiler;
> > if there is an syntax error in this code then the compilation will be stopped.
> 
> We will discuss this and see if it is applicable for PicoTCP. The
> macro you are suggesting is indeed not increasing code size, but our
> static checkers might be not so happy about all this preprocessor
> machinery.
> 
> >
> > $ gcc --version
> > gcc (Debian 4.8.3-1) 4.8.3
> >
> > Here is my 'make -s' output for sandbox barebox:
> >
> [cut]
> 
> Thank You!! This *IS* very helpful input. We are going to add more -W
> flags and quickly fix these warnings.
> 
> > But in linux kernel, barebox, qemu and IMHO many other projects
> > there are:
> >
> >   * CODING_STYLE documentation file;
> >   * scripts/checkpatch.pl
> >
> 
> All good points. Being a kernel developer myself, I know the
> checkpatch.pl approach, and indeed it is a good thing to distribute
> coding rules if you accept contributions. Except it will never apply
> to us, because:
> 
> - We have our company internal coding style which we apply to all our
> projects and we don't feel the need to distribute externally (keep in
> mind that we never planned to accept external contributions). Instead,
> the repository contains the uncrustify configuration file, which is
> pretty much self-explanatory.
> - A few of us are checking every single commit, and running uncrustify
> every now and then. We are comfortable with this approach

Be very carefull with this approach.

It looks like uncrustify changes not only whitespaces and newline symbols!
I suppose that it can change code logic!

E.g. please see this commit

commit 81f52a4ad8fd31edcedd7c91945c801899153d36
Author: Daniele Lacamera <daniele.lacamera@tass.be>
Date:   Wed Mar 12 12:29:21 2014 +0100

    Enforced coding style

Here is a notable fragment

--- a/modules/pico_mm.c
+++ b/modules/pico_mm.c
@@ -75,80 +79,80 @@ typedef union block_internals           block_internals;
         <---------------------------------------------------------------------->
 
 
-        +------------<------------+----------<-----------+
-        |                         ^                      ^
+   +------------<------------+----------<-----------+
+ |                         ^                      ^
         v                         |                      |
-        +---------+------------+--+----+---------------+-+-----+---------------+
-        |         |            |       |               |       |               |
-        |  pico_  |            | pico_ |               | pico_ |               |
-        |  mem_   | ...HEAP... | mem_  |     slab      | mem_  |     slab      |
-        |  page   |            | block |               | block |               |
-        |         |            |       |               |       |               |
-        +---------+------------+-------+-----+---------+-------+----------+----+
+ |+---------+------------+--+----+---------------+-+-----+---------------+
+ |         |            |       |               |       |               |
+ |  pico_  |            | pico_ |               | pico_ |               |
+ |  mem_   | ...HEAP... | mem_  |     slab      | mem_  |     slab      |
+ |  page   |            | block |               | block |               |
+ |         |            |       |               |       |               |
+ |+---------+------------+-------+-----+---------+-------+----------+----+

The number of '|' symbols is changed! After this commit you have at least
two additional '|' symbols. TIP: look at the start of the last line of the fragment.


> - Linux coding rules are generally recognized as possibly not the best
> in the world. I especially dislike tabs.
> - commit hooks are tedious
> 
> I am afraid we are not going to change our formatting style or policy,
> but thanks anyway for the suggestions!

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox