[haiku-gsoc] Re: Fwd: Re: [HCD08] ICMP error handling & propagation project
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Mon, 15 Sep 2008 20:15:59 +0200 CEST
Hi Yin,
"Yin Qiu" <allenchue@xxxxxxxxx> wrote:
> I might fail to send my reply just now, so I'm doing this again.
> Sorry
> if you receive two.
Got just one until now :-)
> I just wondered how you figured out I was using tabs of size 8 while
> \t's are invisible in source files. Now I understand what you meant
> and have set ts=4 in my vim.
Okay, great, if you look through the sources (also the updated patch),
you'll find many occasions where you indented incorrectly (like
net_buffer.h, just to name a new one :-)).
> > But it's a union, and if you use the "gateway" argument, it should
> > be
> > in_addr_t, not uint32. If you need a separate uint32 value that has
> > nothing to do with a gateway (you already have for example the id+
> > sequence fields separately), you should add another field to this
> > union
> > that fits your usage. If you use "gateway" as generic uint32
> > container,
> > then this is misleading.
> You're right. I'll remove this confusion.
Okay.
> > And another problem you seem to have introduced with this patch:
> >> + if (!domain->address_module->
> > > equal_addresses(
> >> + buffer->interface->address, buffer->
> > > destination))
> > The second line should be indented two tabs more (not just one), as
> > it
> > doesn't continue the "if" but the "equal_addresses" part.
> I'll correct this though it was not my original.
The original code was correctly indented, though. Also, you seem to
have forgotten to update udp.cpp, and icmp.h, at least there are still
a lot of coding style violations I already pointed out.
Also, the UDP error handler is still IPv4 specific. And one problem of
using the ICMP constants instead of dedicated NETWORK_ERROR_* constants
(as I suggested earlier) is also that it is IPv4 specific. Looking at
the IPv6 spec will probably tell us more if we can keep it this way
anyway, or not (but that's nothing you have to worry about as part of
the HCD).
> Since this is a minor change, I won't bother sending the patch again.
Unfortunately with this patch, the network stack doesn't seem to work
correctly anymore. DHCP doesn't work, "ping" doesn't work, etc. IIRC
the previous patch worked, though, but I don't have it anymore. You
must have broken something with that last patch.
Also, "big_datagram" crashes with larger packets (like 4000 bytes). I
spotted several problems in that code, like: ip_len is incorrect (must
be in multiple of 4). chksum() should get the "size >> 1", but gets
ip_len instead (ntohs() would need to be used there).
The rest of the code looks good AFAIK, but I haven't looked into the
ICMP specifics yet. It would certainly help to have a patch that at
least works, though :-)
Bye,
Axel.
Other related posts:
