[ECOS] RedBoot sending incorrect TCP window size

[ECOS] RedBoot sending incorrect TCP window size

[Grant Edwards]

I'm having problems with some legacy applications because
Redboot (on ARM) advertises a TCP window size of 1458.  The
legacy stuff breaks if a TCP write of 1460 bytes doesn't go in
a single Ethernet frame.

Yes, I know, the legacy code is broken because it trying to use
TCP as a datagram service instead of a byte stream service.
That battle was lost before I stated working here.  :(

The problem is triggered because Redboot uses the expression
"sizeof (eth_header_t)" to decide how many bytes there are in 
an Ethernet header.  With ARM gcc, that expression has a value
of 16.  There are 14 bytes in an Ethernet header.  The reduces
the window size from 1460 (the usual max size of a TCP payload
in an Ethernet frame) to 1458.  Thus making my admittedly-broken
stuff not work.

It appears that there is no easy way to prevent gcc from
padding structs.

I've complained about this for years.  The response has always
been a scolding for using struct definitions for anything that
had to match up with the outside world's expectations.  

According to the gcc folks, you are suppose to memcpy()
everything to/from hard-wired offsets in communications frames
and memory-mapped I/O.  You are not supposed to use structs for
such things.

The sizeof operation on IP and TCP header structs yields the
expected result because they are an exact multiple of 32 bits.
This is just a lucky feature of gcc, and is not guaranteed to
be portable between architectures, versions, or anything else.

All occurrences of "sizeof (eth_header_t)" in net.h, tcp.c, udp.c
need to be replace by a hard coded constant.  I'll send a patch
later today...

-- 
Grant Edwards
grante@visi.com

Re: [ECOS] RedBoot sending incorrect TCP window size

[Grant Edwards]

On Thu, Feb 08, 2001 at 10:51:36AM -0600, Grant Edwards wrote:

> All occurrences of "sizeof (eth_header_t)" in net.h, tcp.c, udp.c
> need to be replace by a hard coded constant.  I'll send a patch
> later today...

And here it is.

-- 
Grant Edwards
grante@visi.com

Re: [ECOS] RedBoot sending incorrect TCP window size

[Donnat Eric]

Grant Edwards wrote:

> All occurrences of "sizeof (eth_header_t)" in net.h, tcp.c, udp.c
> need to be replace by a hard coded constant.  I'll send a patch
> later today...
>
> --
> Grant Edwards
> grante@visi.com

You can always avoid the hard coded constant by computing the
"real" limit of the struct as the offset to the last field + the
size of this last field. This should always work if the last field
is not a struct/bitfield ! (See stddef.h)

To finnish with the "sizeof" story, you will see with following sample that
the reported value of sizeof is compatible with the data representation
and obviously with pointer arithmetic. Look at gernerated asm.

--
#include <stddef.h>

typedef unsigned char t[6];

typedef struct {
  t x1;
  t x2;
  unsigned short s;
} st;

const int is = sizeof(s);
const st stt[2] = { { {1,2,3,4,5,6}, {1,2,3,4,5,6}, 1000 },
                    { {1,2,3,4,5,6}, {1,2,3,4,5,6}, 2000 } };

--
Eric DONNAT

Silicomp Research Institute
2 avenue de Vignate, 38610 Gieres, France.
http://www.ri.silicomp.com

Re: [ECOS] RedBoot sending incorrect TCP window size

[Grant Edwards]

On Thu, Feb 08, 2001 at 09:15:29PM +0100, Donnat Eric wrote:

> > All occurrences of "sizeof (eth_header_t)" in net.h, tcp.c, udp.c
> > need to be replace by a hard coded constant.  I'll send a patch
> > later today...
> 
> You can always avoid the hard coded constant by computing the
> "real" limit of the struct as the offset to the last field +
> the size of this last field. This should always work if the
> last field is not a struct/bitfield ! (See stddef.h)

For some things that might be a better solution.  On a more
pragmatic note, the size of an Ethernet header is etched in
billions of pieces of silicon spread all over the world.  It's
14. Always. Everywhere. It's not going to change.

> To finnish with the "sizeof" story, you will see with following
> sample that the reported value of sizeof is compatible with the
> data representation and obviously with pointer arithmetic. Look
> at gernerated asm.

I agree that the sizeof value is consistent with the code
generated by the compiler.  It's just not consistent with the
size of an Ethernet header.  :)

-- 
Grant Edwards
grante@visi.com

Re: [ECOS] RedBoot sending incorrect TCP window size

[Bart Veer]

>>>>> "Grant" == Grant Edwards <grante@visi.com> writes:

    Grant> On Thu, Feb 08, 2001 at 09:15:29PM +0100, Donnat Eric wrote:
    >> > All occurrences of "sizeof (eth_header_t)" in net.h, tcp.c, udp.c
    >> > need to be replace by a hard coded constant.  I'll send a patch
    >> > later today...
    >> 
    >> You can always avoid the hard coded constant by computing the
    >> "real" limit of the struct as the offset to the last field +
    >> the size of this last field. This should always work if the
    >> last field is not a struct/bitfield ! (See stddef.h)

    Grant> For some things that might be a better solution. On a more
    Grant> pragmatic note, the size of an Ethernet header is etched in
    Grant> billions of pieces of silicon spread all over the world.
    Grant> It's 14. Always. Everywhere. It's not going to change.

    >> To finnish with the "sizeof" story, you will see with following
    >> sample that the reported value of sizeof is compatible with the
    >> data representation and obviously with pointer arithmetic. Look
    >> at gernerated asm.

    Grant> I agree that the sizeof value is consistent with the code
    Grant> generated by the compiler. It's just not consistent with
    Grant> the size of an Ethernet header. :)

I suspect that there have been arguments about the behaviour of
sizeof() in gcc since the early days of the compiler. In fact the
issue will have arisen with most C compilers. There simply is no good
solution. The definition of sizeof() in the C specification is not
sufficiently precise. Sometimes you want sizeof(struct) to be a
minimal value, 14 bytes in your case, and for different structures you
may want a value of 16 bytes that takes external padding into account,
e.g. for figuring out what is using up all of the heap. Most C
programmers get caught out by this problem sooner or later.

Possibly there should be a gcc extension __packed_sizeof(), returning a
number that excludes external padding. This would not be a complete
solution: people would still use sizeof() in a non-portable fashion.
However it might make it easier to write portable code.

Any further discussion of such an extension should happen in the
appropriate gcc mailing list.

Bart

Re: [ECOS] RedBoot sending incorrect TCP window size

[Jesper Skov]

>>>>> "Bart" == Bart Veer <bartv@redhat.com> writes:
Bart> Possibly there should be a gcc extension __packed_sizeof(),
Bart> returning a number that excludes external padding. This would
Bart> not be a complete solution: people would still use sizeof() in a
Bart> non-portable fashion.  However it might make it easier to write
Bart> portable code.

IIRC GCC treats one-off struct variables different from arrays of the
same struct. So you may have something like:

typedef struct {
  int  foo;
  char bar;
} __attribute__ ((packed)) icky;

icky a;
icky b[10];

sizeof(a) == 5
sizeof(b[0]) == 8

Or something to that effect (*hand wave*)

Jesper