Page MenuHomeFreeBSD

ping: merge ping6 to ping
ClosedPublic

Authored by asomers on Aug 23 2019, 2:09 PM.

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

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 26042
Build 24587: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Don't forget to add an entry to UPDATING and to ObsoleteFiles.inc

sbin/ping/ping.8
63

It's dumb to call this argument iface when it must be an address. What's the difference between -I and -S anyway?

77

When sorting alphabetically, BIG letters like A come before small ones like a.

113

Was this line simply an error in the old version?

117

Needs an article here: "an IPv6 target".

131

Instead of "target of unknown IP version", couldn't you simply say "hostname"?

136

Need another article: "the specific IP version".

169

Need another article: "an IPv4 target".

200

Ditch the comma.

203–213

"an IPv4 target"

208

"an IPv6 target"

234

"an IPv4 target"

238

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

307

s/target/targets/

310

I would simply say "Use IPv4 regardless of how the target is resolved"

403

"hostname or IPv4 address"

407

s/target/targets/

410

As above, I would say "Use IPv6 regardless of how the target is resolved"

769

Let's keep things in chronological order. Can you move the GSoC 2019 sentence to below "FreeBSD 4.0"?

786

I would say "ignore the IPv4 RECORD_ROUTE option"

sbin/ping/ping.c
312

s/options/option/

sbin/ping/ping6.c
358

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
312

Fixed.

sbin/ping/ping6.c
358

Fixed.

Add missing articles in the manual page.

sbin/ping/ping.8
117

Fixed.

136

Fixed.

169

Fixed.

203–213

Fixed.

208

Fixed.

234

Fixed.

238

The article has been added.

Fix typos in the manual page.

sbin/ping/ping.8
200

Done.

307

Fixed.

407

Fixed.

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

sbin/ping/ping.8
113

Yes. More precisely, the bell is included only for a non-flood ping. Should I try to mention this in the manual page?

131

Fixed.

310

Fixed.

403

Fixed.

410

Fixed.

769

Fixed.

786

Fixed.

sbin/ping/ping.8
113

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
77

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
238

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
238

Don't worry about it. It isn't ping.8's job to document the kernel.

sbin/ping/ping.8
63

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

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.