When an ICMP packet contains an IP packet in its payload, and that original IP packet contains options, these options were not displayed accordingly in pr_iph().
pr_iph() is a function that prints the original "quoted packet" IP header, with only an IP struct as an argument. The IP struct does not contain IP options, and it is not guaranteed that the options will be contiguous in memory to the IP struct after d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52.
Pass the raw ICMP data along with the IP struct, in order to print the options, if any.
Details
Details
- Reviewers
asomers markj thj - Group Reviewers
network - Commits
- rG20b41303140e: ping: Print the IP options of the original packet
DISCLAIMER: My preferred route would be to implement D38431, however for reasons detailed in that review, we'll resort to fixing what we currently have.
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sbin/ping/ping.c | ||
|---|---|---|
| 1356 | How is oip initialized? I don't understand how this can work. | |
| sbin/ping/ping.c | ||
|---|---|---|
| 1356 | Maybe working with a MWE helps: The gist is that we should use a pointer to an IP struct, instead of an IP struct itself, as its size is not always fixed. The implementation chosen was one that produces a minimal diff. | |
| sbin/ping/ping.c | ||
|---|---|---|
| 1356 | I see, thanks. It looks strange to me to copy a pointer with memcpy, and doing so hides alignment warnings. If we can't get an aligned pointer to the IP header, we should make a copy of the header. | |
Comment Actions
Address suggestions (not sure if this fits the bill).
EDIT: Let me know if adding const to a bunch of required places is preferred.
Comment Actions
Apologies for the delay, 13.2 release process is ongoing, so the team and myself are mostly working on addressing the arising issues. Hope we'll be able to get to this (and other) ping diffs closed to the end of the week.
Thank you for being so patient!
Comment Actions
No problem at all. Take all the time needed. I can imagine how things are at the moment. I have also stalled the submission of other, minor fixes planned until the slush curfew is lifted.
| sbin/ping/ping.c | ||
|---|---|---|
| 1356 | Doesn't this effectively undo d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52 ? Do we want to simply ignore alignment warnings in favour of simplifying the code? | |
Comment Actions
Try a different approach.
Given this is the base revision for a number of others, some merge conflicts may arise. At any rate,w2 I have a branch with all the other accepted revisions already rebased just in case.
The code inside pr_retip() is only done here for completeness, as it will be completely removed later on.
| sbin/ping/ping.c | ||
|---|---|---|
| 1159–1160 | oicmp_raw is effectively unused. | |
| 1190–1191 | This guard can be removed. | |
| 1536 | I think the name oicmp_raw is deceiving. | |
| 1669 | We could avoid a magic number here and use sizeof(struct ip)? | |
Comment Actions
All the "important" ones are in a branch called ping-enchilada (for the lack of a better name):
https://github.com/jlduran/freebsd-src/tree/ping-enchilada
The interrupt changes and unification of statistics are in ping-interrupts-ping6-unification. This branch is already rebased over ping-enchilada:
https://github.com/jlduran/freebsd-src/tree/ping-interrupts-ping6-unification
Items marked WIP are still under review.
There are a couple of trivial commits that start with TODO, those were not submitted yet, feel free to discard/commit them if you see fit.
The base branch includes a commit to be dropped, it essentially runs the tests using the Cirrus CI infrastructure, in order to validate the changes.
So, main -> base -> ping-enchilada -> ping-interrupts-ping6-unification.
Thank you!