[haiku-gsoc] Re: Fwd: Re: [HCD08] ICMP error handling & propagation project
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Sat, 13 Sep 2008 15:10:02 +0200 CEST
"Yin Qiu" <allenchue@xxxxxxxxx> wrote:
> Any comments are appreciated.
Sorry for the delay, I was on vacation, and I guess Philippe as well (I
don't know for sure, though).
Anyway, I'm currently reading through the patch. I won't go into the
technical details right now, I just have to make lots of coding style
comments (but it's nice to see so many test apps):
> --- src/tests/kits/net/icmp_tests/tcp_unreachable.c (revision 0)
> +++ src/tests/kits/net/icmp_tests/tcp_unreachable.c (revision 0)
[...]
> + if (argc != 3)
> + {
The opening brackets go on the same line for statements - this is done
incorrectly at many many places, though not everywhere. Please always
try to be consistent with your coding style, we do have coding style
guidelines (http://www.haiku-os.org/documents/dev/haiku_coding_guidelines),
and when in doubt, always try to copy what's there in the file.
We do have a coding style for a reason, and it's pretty annoying to see
it ignored that badly.
> --- src/tests/kits/net/icmp_tests/big_datagram.c (revision 0)
> +++ src/tests/kits/net/icmp_tests/big_datagram.c (revision 0)
> @@ -0,0 +1,165 @@
> +#define _BSD_SOURCE
License header is missing from all test files - not overly important,
but still a good thing to have.
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#include <netdb.h>
> +
> +#include <netinet/ip.h>
> +#include <netinet/udp.h>
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include <errno.h>
Header ordering is usually alphabetically, see coding style guide for
more details.
> +u_int16_t
> +cksum (u_int16_t *buf, int nwords)
No blank between function name and opening parenthesis.
[...]
> + if ( (n = getaddrinfo(host, serv, &hints, &res)) != 0)
> + {
No blank between opening parenthesis, the opening bracket goes to the
same line as the if.
> --- 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.
> @@ -699,6 +692,14 @@
>
> if (raw->Socket()->protocol == buffer->protocol)
> raw->SocketEnqueue(buffer);
> + if (raw->AvailableData() >
> + raw->Socket()->receive.buffer_size *
> + IP_BUFFER_HIGH_WATERMARK)
This and many other places: the logical and arithmetic operators go to
the next line!
> @@ -1422,20 +1423,58 @@
> return bufferHeader.Status();
>
> ipv4_header &header = bufferHeader.Data();
> + /*
> + uint16 destMtu = sDomain->module->get_mtu(NULL, buffer->
> destination);
> + if ((header.FragmentOffset() & IP_DONT_FRAGMENT) &&
&& goes to the next line, if you commit commented code, please always
have a note why it is commented out.
> - if (packetLength > buffer->size
> - || headerLength < sizeof(ipv4_header))
> - return B_BAD_DATA;
> + if (packetLength > buffer->size)
> + {
> + uint32 errOctet = htonl(2<<24); // Total length
> + return sDomain->module->error_reply(NULL, buffer,
> + icmp_encode(ICMP_TYPE_PARAM_PROBLEM, 0), &errOctet);
> + }
The return value is most probably wrong here, ie. you'll likely leak
the net_buffer. If you return B_OK (and this is what will happen if the
reply could be sent correctly), you take over ownership of the buffer
passed in.
This happens in all those cases that I've seen.
> + // Extracts the IP header in the ICMP message
> + NetBufferField<ipv4_header, 8> iphField(data);
Prefer constants like "sizeof(some_header)" over direct numbers. This
construct is used in other places as well.
> @@ -1540,7 +1596,14 @@
[...]
> + if (icmpModule) {
> + return icmpModule->error_reply(protocol, causedError,
> + code, errorData);
> + }
> + else
> + return B_ERROR;
The else goes to the line with the closing bracket, ie. "} else".
> +tcp_error_received(uint32 code, net_buffer* data)
[...]
> + switch (icmpType)
> + {
Opening bracket on the same line...
> + NetBufferFieldReader<struct ip, 8> iph(data);
> + NetBufferFieldReader<struct tcp_header, sizeof(struct ip)>
> tcph(data);
> + struct sockaddr_in source, dest;
> + memset(&source, 0, sizeof(struct sockaddr_in));
> + memset(&dest, 0, sizeof(struct sockaddr_in));
> + source.sin_family = dest.sin_family = AF_INET;
> + source.sin_addr = iph->ip_src;
> + source.sin_len = dest.sin_len = sizeof(struct sockaddr_in);
> + source.sin_port = tcph->source_port;
> + dest.sin_addr = iph->ip_dst;
> + dest.sin_port = tcph->destination_port;
This is IPv4 dependent - you need to use the address module that is
part of the IPv4 add-on instead. For things where this won't work for
some reason, you need to at least special case the family, and fail
with an error if it is not supported.
> +++ 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.
> +static bool
> +is_icmp_error(uint8 type)
> +{
> + return type == ICMP_TYPE_UNREACH ||
> + type == ICMP_TYPE_PARAM_PROBLEM ||
> + type == ICMP_TYPE_REDIRECT ||
> + type == ICMP_TYPE_TIME_EXCEEDED ||
> + type == ICMP_TYPE_SOURCE_QUENCH;
> +}
|| operator always goes to the next line, too.
> +++ headers/private/net/ipv4.h (revision 0)
[...]
> +} _PACKED;
> +
> +
> +#define IP_BUFFER_HIGH_WATERMARK 0.9
> +
> +
> +#define IP_VERSION 4
> +
> +#endif
The last #endif misses the /* IPV4_H */. Shared system headers like
this one should get an extra '_' in front of their header guard.
Also, please don't use white space and indentation so inconsistently.
And BTW, it's 2008 already (all your new headers have 2006-2007
copyright, but should only have 2008).
icmp.h looks like it wasn't written with a tab size of 4 either, and
shares the same problems as ipv4.h.
I'll look into the technical details later, and reply to your mail
properly, although it looks good on a first glance (besides leaking the
net buffers as pointed out above).
Please correct the issues pointed out above as well as all the style
problems and resend the patch.
Bye,
Axel.
Other related posts:
