[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 12:22:44 +0200 CEST
Hi Yin,
"Yin Qiu" <allenchue@xxxxxxxxx> wrote:
> Here's the updated patch.
Thanks!
> I have corrected most of the issued you mentioned except the
> following:
> >> --- src/add-ons/kernel/network/protocols/ipv4/ipv4.cpp
> > > (revision
> >> 27207)
> >> +++ src/add-ons/kernel/network/protocols/ipv4/ipv4.cpp
> > > (working
> >> copy)
> > [...]
> >> net_timer fTimer;
> >> + net_buffer *fLastBuffer;
> >> };
> > Indentation is wrong: we use a tab size of 4.
> Sorry but I don't know what you mean here by a tab size of 4.
Usually in a text editor when you press the tab key, the indentation is
set to 8 spaces. Ie. visually, 8 spaces are the same than one tab.
However, we don't use 8 spaces, we use 4. Every editor I know of can be
configured to use a tab size (or width) of 4 instead of 8.
It often isn't visible if you use 4 or 8 in a source file, but with
some indentation (like the "if" line below) and alignments (like the
field type/name above) you notice that you're using the wrong tab
width.
For example, if you use "pe", the setting can be found in "Preferences/
Editor" as "Spaces for Tab" (as well as in "File Options". With vim,
you would set it via "set ts=4", with Kate it's under "Settings/
Configure Kate/Editing/Tabulators" and there "Tab width".
> >> +++ src/add-ons/kernel/network/protocols/icmp/icmp.cpp
> > > (working
> >> copy)
> > [...]
> >> + uint32 gateway;
> >> struct {
> >> - in_addr_t gateway;
> > in_addr_t should be the type of this member here, as it contains an
> > address, not some arbitrary number.
> The gateway field here is just for convenient manipulation, not only
> stands for the gateway information in a Redirect message. It
> represents the 32-bit content in an ICMP header, no matter the actual
> content is the gateway, unused, or (identifier+sequence).
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.
> > icmp.h looks like it wasn't written with a tab size of 4 either,
> > and
> > shares the same problems as ipv4.h.
> Again I don't understand which part of the code you are referring to
> when saying "a tab size of 4".
I hope you understood my explanation above.
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.
> The technical review may be more miserable :) I'll try my best to
> catch up with. Anyway thanks for your effort.
I'll get to that later today.
Bye,
Axel.
Other related posts:
