[haiku-gsoc] Re: Fwd: Re: [HCD08] ICMP error handling & propagation project
- From: Stephan Assmus <superstippi@xxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Wed, 30 Jul 2008 13:00:07 +0200
Hi Yin,
thanks a lot for your work! I cannot really comment on what actually
happens in the patch, but it does contain a lot of coding style violations.
Also the end of the patch appears to add a lot of code to udp.cpp, but in
my copy, this code is already there. A file system corruption on your side?
You may have to backup the code you changed in the same directory, delete
the directory from your tree, svn update to get it fresh from the server
and put your backed up code back into placed.
Here are a couple things I spotted:
@@ -1422,11 +1401,28 @@
return bufferHeader.Status();
ipv4_header &header = bufferHeader.Data();
+ // Preserve the ipv4 header
+ ipv4_header *cloned_header = new ipv4_header;
+ cloned_header->total_length = header.totoal_length;
+ cloned_header->version = header.version;
...
You need to use "new (std::nothrow)" instead of just "new" and check the
result against NULL before proceeding.
+ if (header.time_to_live == 0)
+ // Send ICMP Error: Time to Live Exceeded in Transit
+ return sDomain->module->error_reply(NULL, buffer,
NET_ERROR_TIMEEX_IN_TRANSIT, NULL);
For if bodies containing of more than one line, you need to use parenthesis:
+ if (header.time_to_live == 0) {
+ // Send ICMP Error: Time to Live Exceeded in Transit
+ return sDomain->module->error_reply(NULL, buffer,
NET_ERROR_TIMEEX_IN_TRANSIT, NULL);
+ }
On the other hand, if there is only one line, no paranthesis are to be used:
+ net_protocol_module_info *protocol_module =
receiving_protocol(data->protocol);
+ if (protocol_module != NULL)
+ {
+ return protocol_module->error_received(code, data);
+ }
return B_ERROR;
->
+ net_protocol_module_info *protocol_module =
receiving_protocol(data->protocol);
+ if (protocol_module != NULL)
+ return protocol_module->error_received(code, data);
+
return B_ERROR;
In general, you use the wrong style for parenthesis according to our coding
style:
+ uint8 icmp_type = (uint8)(code / 14);
+ uint8 icmp_code = (uint8)(code % 14);
+ switch (icmp_type)
+ {
+ case ICMP_TYPE_UNREACH:
+ if (icmp_code == ICMP_CODE_FRAG_NEEDED)
+ {
+ // TODO: PMTU discovery
+ }
+ break;
+ case ICMP_TYPE_SOURCE_QUENCH:
->
+ uint8 icmp_type = (uint8)(code / 14);
+ uint8 icmp_code = (uint8)(code % 14);
+ switch (icmp_type) {
+ case ICMP_TYPE_UNREACH:
+ if (icmp_code == ICMP_CODE_FRAG_NEEDED) {
+ // TODO: PMTU discovery
+ }
+ break;
+ case ICMP_TYPE_SOURCE_QUENCH:
In the above code, I would personally prefer bit shift and mask operations
over the / 14 and % 14 thing (preferrably with #defines for the mask and
shift values). But that may just be me. :-)
Here you used the correct parenthesis style, but again, it's only one line
in the if body:
@@ -230,12 +220,22 @@
icmp_receive_data(net_buffer *buffer)
{
TRACE(("ICMP received some data, buffer length %lu\n", buffer->size));
+ net_domain *domain;
+ if (buffer->interface != NULL) {
+ domain = buffer->interface->domain;
+ } else
+ domain = sStackModule->get_domain(buffer->source->sa_family);
->
+ net_domain *domain;
+ if (buffer->interface != NULL)
+ domain = buffer->interface->domain;
+ else
+ domain = sStackModule->get_domain(buffer->source->sa_family);
My last comment is that you need to honour the 80 char per line limit. A
lot of us work with editors configured to this width and it is annoying if
lines are longer than 80 chars.
That's it from me, I hope the others have more useful comments with regard
to the actual implementation.
Best regards,
-Stephan
Other related posts:
