Merge ping6 to ping
The front end code parses only -4 and -6 options, and decides whether to call ping or ping6.
Submitted by: Ján Sučan <sucanjan@gmail.com>
Sponsored by: Google LLC (Google Summer of Code 2019)
Details
Details
- Reviewers
jansucan - Group Reviewers
manpages - Commits
- rS368045: Merge ping6 to ping
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| sbin/ping/ping.8 | ||
|---|---|---|
| 63 ↗ | (On Diff #61157) | It's dumb to call this argument iface when it must be an address. What's the difference between -I and -S anyway? |
| 78 ↗ | (On Diff #61157) | When sorting alphabetically, BIG letters like A come before small ones like a. |
| 113 ↗ | (On Diff #61157) | Was this line simply an error in the old version? |
| 118 ↗ | (On Diff #61157) | Needs an article here: "an IPv6 target". |
| 132 ↗ | (On Diff #61157) | Instead of "target of unknown IP version", couldn't you simply say "hostname"? |
| 137 ↗ | (On Diff #61157) | Need another article: "the specific IP version". |
| 170 ↗ | (On Diff #61157) | Need another article: "an IPv4 target". |
| 200 ↗ | (On Diff #61157) | Ditch the comma. |
| 203 ↗ | (On Diff #61157) | "an IPv4 target" |
| 209 ↗ | (On Diff #61157) | "an IPv6 target" |
| 234 ↗ | (On Diff #61157) | "an IPv4 target" |
| 239 ↗ | (On Diff #61157) | "an IPv6 target". Is the default hard-coded or defined by the net.inet6.ip6.hlim sysctl? |
| 308 ↗ | (On Diff #61157) | s/target/targets/ |
| 311 ↗ | (On Diff #61157) | I would simply say "Use IPv4 regardless of how the target is resolved" |
| 404 ↗ | (On Diff #61157) | "hostname or IPv4 address" |
| 408 ↗ | (On Diff #61157) | s/target/targets/ |
| 411 ↗ | (On Diff #61157) | As above, I would say "Use IPv6 regardless of how the target is resolved" |
| 770 ↗ | (On Diff #61157) | Let's keep things in chronological order. Can you move the GSoC 2019 sentence to below "FreeBSD 4.0"? |
| 786 ↗ | (On Diff #61157) | I would say "ignore the IPv4 RECORD_ROUTE option" |
| sbin/ping/ping.c | ||
| 313 ↗ | (On Diff #61157) | s/options/option/ |
| sbin/ping/ping6.c | ||
| 359 ↗ | (On Diff #61157) | s/options/option/ |
| sbin/ping/ping.8 | ||
|---|---|---|
| 113 ↗ | (On Diff #61157) | Yes. More precisely, the bell is included only for a non-flood ping. Should I try to mention this in the manual page? |
| 132 ↗ | (On Diff #61157) | Fixed. |
| 311 ↗ | (On Diff #61157) | Fixed. |
| 404 ↗ | (On Diff #61157) | Fixed. |
| 411 ↗ | (On Diff #61157) | Fixed. |
| 770 ↗ | (On Diff #61157) | Fixed. |
| 786 ↗ | (On Diff #61157) | Fixed. |
| sbin/ping/ping.8 | ||
|---|---|---|
| 113 ↗ | (On Diff #61157) | Probably not necessary. I doubt anybody will be disappointed when they don't hear a bell 1000 times per second when they try to flood ping. |
Comment Actions
Now it doesn't even build:
/usr/home/somers/freebsd/base/head/sbin/ping/main.c:249:1: error: redefinition
of 'main'
main(int argc, char *argv[])
^
/usr/home/somers/freebsd/base/head/sbin/ping/main.c:59:1: note: previous
definition is here
main(int argc, char *argv[])
^
/usr/home/somers/freebsd/base/head/sbin/ping/main.c:331:1: error: redefinition
of 'usage'
usage(void)
^
/usr/home/somers/freebsd/base/head/sbin/ping/main.c:141:1: note: previous
definition is here
usage(void)
^
2 errors generated.Comment Actions
It will take me some time to find out why. On my system it builds. Could you please send me your main.c? It's interesting that there are double definitions of the functions.
Comment Actions
Sorry, my bad. It's a side effect of arcanist. If you apply a patch, revert the directory, and reapply the patch, any newly created file gets doubled. You need to rm newly created files after "svn revert". I forgot to do that.
| sbin/ping/ping.8 | ||
|---|---|---|
| 78 ↗ | (On Diff #61157) | Fixed. |
| sbin/ping/ping.8 | ||
|---|---|---|
| 239 ↗ | (On Diff #61157) |
It's not hard-coded, but there is no explicit getting of net.inet6.ip6.hlim value either. If the -m is not used, ping6 doesn't set hoplimit on the socket and the system probably uses net.inet6.ip6.hlim. I can try to find out if that's really true. |
| sbin/ping/ping.8 | ||
|---|---|---|
| 239 ↗ | (On Diff #61157) | Don't worry about it. It isn't ping.8's job to document the kernel. |
| sbin/ping/ping.8 | ||
|---|---|---|
| 63 ↗ | (On Diff #61157) | Well, ping6's -I is less equal to ping's I than it should be. For ping6, argument to the -I is an interface name which is converted to an interface index by if_nametoindex and saved to "struct in6_pktinfo.ipi6_ifindex" or "struct sockaddr_in6.sin6_scope_id". An argument to the -S is resolved by cap_getaddrinfo and causes bind to be called. I lack some knowledge to tell how redundant those two options are. I will try to work on it. |
Comment Actions
You still need to add entries to UPDATING and ObsoleteFiles.inc. Other than that, it looks good.
| sbin/ping/ping.8 | ||
|---|---|---|
| 63 ↗ | (On Diff #61157) | Don't worry about it for now; the problem predates you and it's not really related to this review. |
Comment Actions
Add entries to UPDATING and ObsoleteFiles.inc.
Remove ping6 from tools/build/mk/OptionalObsoleteFiles.inc.
Comment Actions
Is there any interest to get this into -CURRENT?
I am working on a few small ping updates that could be benefit from getting this into tree first.
Comment Actions
Thanks for reminding me, @gbe . I think I'll have time to work on this on Halloween weekend. Feel free to remind me if I haven't done anything by November.
Comment Actions
Arcanist won't let me update the review because of this bug: https://secure.phabricator.com/T10608 . But the required changes are small enough that I don't think they need additional review. There were a few related to rebasing. And there were a few changes required to fix the tests. I'll commit them if the tinderbox build passes.