usr: fix the ping build and its ICMP reply parsing - #321
Merged
Conversation
ping did not build any more: the kernel-side lwIP headers (lwip/icmp.h,
lwip/inet.h, lwip/netif.h) are not on the user-space include path, and
they are not needed either — musl's <netinet/ip_icmp.h> already provides
ICMP_ECHO/ICMP_ECHOREPLY and struct iphdr.
Dropping them alone would still have left the reply check wrong. The
magic 69 it used to compare against was never an ICMP type: it is 0x45,
the first byte of the IP header (version 4, IHL 5). A raw socket hands
over the whole IP datagram — lwIP delivers the pbuf to the raw pcb with
the payload still pointing at the IP header — so the ICMP message starts
at iphdr->ihl * 4, not at offset 0.
Parse the reply accordingly:
- receive into a buffer sized for the IP header plus the ICMP message;
the previous 64-byte buffer truncated the payload,
- reject a reply too short to hold an ICMP header,
- check type/code at the right offset, against ICMP_ECHOREPLY,
- report the TTL that came back rather than the one we asked for.
While here, initialise rtt_total (it was summed into uninitialised) and
close the socket on the way out instead of leaving the close commented
out under a dead label.
This tree's default target is SO3, so the first thing the build container suggests should be the SO3 BSP, not the Linux one. Switch the examples in the container banner, in dbuild.sh's header and --help, in docker/README.md, in the Dockerfile header and in the build-system chapter; the bsp-linux mention stays as the alternative.
--ensure runs after every build and deploy, including on the soft- and hard-storage platforms that never publish a TEZI feed, so an absent feed directory is the normal case there — yet each command ended with a "feed directory does not exist yet / run a deploy first" pair on stderr. Test for the feed quietly in do_ensure and leave the explaining to check_feed, which now only runs when the server is asked for explicitly (foreground invocation), where the hint is what the user wants.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
build.sh bsp-so3was failing inusr-so3:do_build:Those lwIP headers are kernel-side (
so3/so3/include/net/lwip/) and are noton the user-space include path — nor needed: musl's
<netinet/ip_icmp.h>already provides
ICMP_ECHO/ICMP_ECHOREPLYandstruct iphdr.Removing the includes alone would have left the reply check broken. The magic
69the code compared against was not an ICMP type but0x45, the first byteof the IP header (version 4, IHL 5): a raw socket delivers the whole IP
datagram (lwIP calls the raw pcb's receive callback with the payload still on
the IP header, see
net/lwip/core/raw.c:raw_input), so the ICMP message startsat
iphdr->ihl * 4. The reply is now parsed at that offset, the receive bufferis sized for
iphdr + PING_PKT_LEN(the old 64-byte one truncated thepayload), a too-short reply is rejected, and the TTL reported is the one that
came back rather than the one requested.
rtt_totalwas also summed intowithout being initialised, and
close(s)was commented out under a dead label.Two build-experience fixes ride along, one commit each:
dbuild.sh's header/--help,docker/README.md,the Dockerfile header and the build-system chapter now lead with
bsp-so3;this tree's default target is SO3,
bsp-linuxstays as the alternative;tezi-feed-serve.sh --ensureruns after every build and deploy, including onsoft/hard-storage platforms that never publish a TEZI feed, and printed
"feed directory does not exist yet / run a deploy first" every single time.
It is silent now when there is nothing to serve; the hint is kept for an
explicit foreground invocation.
Testing
build.sh bsp-so3anddeploy.sh bsp-so3on virt64, clean.st.sh) to the SO3 shell;ping.elfpresent in the rootfs.clang-format --dry-run -Werror so3/usr/src/ping.cclean.Note:
pingcannot be exercised on virt32/virt64 as shipped —CONFIG_NETisoff in every defconfig and the only NIC driver is
smc911x, which QEMU'svirtmachine does not provide (doc/source/lwip.rstalready documents this).The fix is therefore verified by build and inspection, not by a live echo reply.