Page MenuHomeFreeBSD

ping: merge ping6 to ping
ClosedPublic

Authored by asomers on Aug 23 2019, 2:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 18, 7:40 AM
Unknown Object (File)
Sun, Mar 10, 11:23 AM
Unknown Object (File)
Sun, Mar 10, 11:19 AM
Unknown Object (File)
Sun, Mar 10, 11:19 AM
Unknown Object (File)
Sun, Mar 10, 11:19 AM
Unknown Object (File)
Thu, Mar 7, 7:48 PM
Unknown Object (File)
Thu, Mar 7, 7:48 PM
Unknown Object (File)
Thu, Mar 7, 7:48 PM
Subscribers

Details

Reviewers
jansucan
Group Reviewers
manpages
Commits
rS368045: Merge ping6 to ping
Summary

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)

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/

This revision now requires changes to proceed.Aug 23 2019, 4:25 PM

Oh, and you need to update rescue/rescue/Makefile, too.

Update rescue/rescue/Makefile.
Fix typos in comments in ping.c and ping6.c.

sbin/ping/ping.c
313 ↗(On Diff #61157)

Fixed.

sbin/ping/ping6.c
359 ↗(On Diff #61157)

Fixed.

Add missing articles in the manual page.

sbin/ping/ping.8
118 ↗(On Diff #61157)

Fixed.

137 ↗(On Diff #61157)

Fixed.

170 ↗(On Diff #61157)

Fixed.

203 ↗(On Diff #61157)

Fixed.

209 ↗(On Diff #61157)

Fixed.

234 ↗(On Diff #61157)

Fixed.

239 ↗(On Diff #61157)

The article has been added.

Fix typos in the manual page.

sbin/ping/ping.8
200 ↗(On Diff #61157)

Done.

308 ↗(On Diff #61157)

Fixed.

408 ↗(On Diff #61157)

Fixed.

Use better wording in the manual page.
Keep the history in chronological order.

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.

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.

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.

In D21377#465322, @sucanjan_gmail.com wrote:

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.

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.

Sort options in the manual page alphabetically.

sbin/ping/ping.8
78 ↗(On Diff #61157)

Fixed.

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.

It's OK. This information could be helpful if I'll work with svn.

sbin/ping/ping.8
239 ↗(On Diff #61157)

Is the default hard-coded or defined by the net.inet6.ip6.hlim sysctl?

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.

No ping6 in the usage message.

asomers requested changes to this revision.Aug 24 2019, 3:13 PM

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.

This revision now requires changes to proceed.Aug 24 2019, 3:13 PM

Add entries to UPDATING and ObsoleteFiles.inc.
Remove ping6 from tools/build/mk/OptionalObsoleteFiles.inc.

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.

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.

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.

This is friendly reminder! :)

asomers edited reviewers, added: jansucan; removed: asomers.

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.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 8 2020, 6:43 PM
Closed by commit rS368045: Merge ping6 to ping (authored by asomers). · Explain Why
This revision was automatically updated to reflect the committed changes.