[haiku-gsoc] Re: Fwd: Re: [HCD08] ICMP error handling & propagation project
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Wed, 30 Jul 2008 16:05:51 +0200 CEST
Hi Yin,
just a quick review from me, coding style notes mentioning only what
Stephan didn't yet:
> + ipv4_header *cloned_header = new ipv4_header;
> + net_protocol_module_info *protocol_module =
> receiving_protocol(data->protocol);
> + uint8 icmp_type = (uint8)(code / 14);
> + uint8 icmp_code = (uint8)(code % 14);
> + NetBufferHeaderReader<icmp_header> new_header(reply);
Naming style would be clonedHeader, protocolModule, icmpType, icmpCode,
newHeader.
> +status_t
> +icmp_error_reply(net_protocol *protocol, net_buffer *causedError,
> uint32 code,
> + void *errorData);
> +
> +static bool
> +is_icmp_error(uint8 type)
> +{
> + return type == ICMP_TYPE_UNREACH || type == ICMP_TYPE_SOURCE_QUENCH
> || type == ICMP_TYPE_REDIRECT || type == ICMP_TYPE_TIME_EXCEEDED ||
type == ICMP_TYPE_PARAM_PROBLEM;
> +}
> +
> net_protocol *
a) Prototype declarations don't have a newline after the return type.
b) two blank lines between functions.
c) 80 character line limit.
> + if (buffer->interface != NULL) {
> + domain = buffer->interface->domain;
>
> + } else
No {} when there is no need to, no useless blank lines like this one
(only to separate actual code lines).
> + struct timeval tv;
> + if (gettimeofday(&tv, NULL) == 0)
> + {
> + uint32 time = tv.tv_sec * 1000 + tv.tv_usec /
> 1000;
> + net_buffer *reply =
> gBufferModule->duplicate(buffer);
No need to use gettimeofday() here; prefer native
real_time_clock_usecs() here.
> + // Make sure i'm an error
> + uint8 err_code = header.type * 14 + header.code;
> + return domain->module->error_received(err_code,
> buffer);
14 is an odd number, anyway. Using bitfields (like 8 bit for the type,
and 8 for the code) sounds like the better alternative.
> + ipv4_header *cloned_header = new ipv4_header;
> + cloned_header->total_length = header.totoal_length;
> + cloned_header->version = header.version;
> + cloned_header->service_type = header.service_type;
> + cloned_header->id = header.id;
> + cloned_header->fragment_offset = header.fragment_offset;
> + cloned_header->time_to_live = header.time_to_live;
> + cloned_header->protocol = header.protocol;
> + cloned_header->checksum = header.checksum;
> + cloned_header->source = header.source;
> + cloned_header->destination = header.destination;
Use memcpy() or = instead.
> + 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);
80 character limit, always use {} for multiline statements.
The net_buffer.h header changes are missing. I'm not sure if all the
error messages are correct under the given circumstances (ie. if
reassemble_fragments() fails because of out of memory, it's not really
a "time exceeded" error). Also, please use more descriptive naming than
NET_ERROR_TIMEEX_IN_TRANSIT - NET_ERROR_TIME_EXCEEDED would be clearer
and even shorter (the "in transit" is pretty much superfluous IMO).
tcp.cpp:
> + else {
> + // Should not send ICMP UNREACH msg
> + // Should send TCP RST response
> + segmentAction = RESET;
> + }
I don't think this is correct; when a host receives a RESET, it doesn't
make much sense to reply with a RESET again.
> + uint8 icmp_type = (uint8)(code / 14);
> + uint8 icmp_code = (uint8)(code % 14);
Instead of doing this all over the code, a handy macro would be
preferrable. But is it really necessary for the protocol to get both,
the type and the code? And since this is part of the same API, I would
prefer if the NET_ERROR_* defines are reused instead.
Now to your questions:
> 1. I have put the icmp_header struct definition and ICMP types/codes
> macro
> in net/icmp.h since these are shared among various protocols.
As I mentioned above, I don't really like this approach.
> 2. I'm not quite sure about my implementation of the error_reply
> hooks.
> Generally, ipv4_error_reply directly obtains the ICMP module info and
> then
> delegates the execution to icmp_error_reply. Would you please give me
> some
> advice on these two hooks?
I'm not sure what you mean; at least the error_reply() code looks good
to me.
> 3. Error handling mechanisms in TCP and UDP protocols are not yet
> fully
> implemented. However, maybe you can give some advice by reviewing the
> skeleton code in tcp.cpp and udp.cpp.
Not sure what advice you are seeking for. Do you mean how the protocol
should react on the different errors?
> 4. Source quench problems is not handled neither at the receiver nor
> the
> sender side. After a glance at the Linux implementation, it appears
> that it
> did not handle that either. I'll look into this further.
Okay; getting the general architecture in place is more important, of
course :-)
The details can be filled out later.
> 5. Some invocation of error_reply currently lacks additional
> parameter, such
> as for ICMP type Parameter Problem. This will be resolved soon.
Defining different NET_ERROR_* macros should be the way to go, or what
do you intend to do about it?
> 6. Now that error_reply hook is a way to send ICMP error messages, I
> don't
> notice any function that takes care of sending ICMP query messages
> like ECHO
> and TIMESTAMP. Is there anything I missed? Or is icmp_send_data() for
> this
> purpose?
Not sure if I understand you; what's wrong with how it behaves now, ie.
in ICMP_TYPE_ECHO_REQUEST it does:
status_t status = domain->module->send_data(NULL,
reply);
if (status < B_OK) {
gBufferModule->free(reply);
return status;
}
HTH,
Bye,
Axel.
Other related posts:
