Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a handler for ICMP Destination Unreachable #2544

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

audeoudh
Copy link

@audeoudh audeoudh commented Dec 5, 2018

Previously, they were silently ignored, because of no handler. Now, some debug information message is written on serial, that's all.

Nodes was already able to send Destination Unreachable messages, but was
not able to parse them.

If DEBUG is true in `core/net/ipv6/uip-icmp6.c`, then a message is
printed on the serial line.
@g-oikonomou
Copy link
Contributor

In principle that's fine, but... perhaps we should wrap everything inside DEBUG to make sure we can actually disable it in its entirety? I mean including the handler definition and registration

@audeoudh
Copy link
Author

audeoudh commented Dec 5, 2018

I'm OK for a ICMP_DESTINATION_UNREACHABLE_SUPPORT macro, that deactivates these code — even though one more new macro is debatable — but I don't see why we should process this type of message only in debug mode. We can expect the system reacts to a Destination Unreachable error (error pushed to the TCP stack, or even in the UDP one, not implemented now), so having no handler at all for this type of message is surprising.

@g-oikonomou
Copy link
Contributor

I was thinking more along the lines of wrapping things inside #if DEBUG blocks, not of establishing a new macro. As you say, it makes sense to actually process this message and do something useful when we receive it. But this pull does not add any functionality other than debugging and I am disinclined to increase code/RAM footprint (even if minimal) because of code that does exactly nothing if DEBUG is 0.

audeoudh added a commit to drakkar-lig/contiki that referenced this pull request Dec 5, 2018
Completely disable the handler (even registration) if DEBUG is not
activated.  Rationale: the handler does not add any functionality other
than debugging.

contiki-os#2544
audeoudh added a commit to drakkar-lig/contiki that referenced this pull request Dec 5, 2018
Completely disable the handler (even registration) if DEBUG is not
activated.  Rationale: the handler does not add any functionality other
than debugging.

contiki-os#2544
@audeoudh
Copy link
Author

audeoudh commented Dec 5, 2018

I understand your point. I did not think at the memory footprint. However, I do not share the conclusion. Not really about the memory footprint itself, but because of the code clarity. I feel unclear to find a debug-independant code in a #if DEBUG block, because the rationale of these blocks is obscure when one hasn't read this debate.

Feel free to cherry-pick the commit I did for my tests if you stick to your opinion : drakkar-lig/contiki@6708fae

For information (not for the debate : I understand this is not a question of quantity, but a question of principle), I tested it: the handler & registration code is 64B more in .txt, 12B in .data, and 8B in .bss.

alexrayne pushed a commit to alexrayne/contiki that referenced this pull request Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants