Page MenuHomeFreeBSD

ping: merge ping6 to ping
Needs ReviewPublic

Authored by sucanjan_gmail.com on Fri, Aug 23, 2:09 PM.

Details

Reviewers
asomers
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 26087
Build 24621: arc lint + arc unit

Event Timeline

Delete ping6 directory with its remaining content.

asomers requested changes to this revision.Fri, Aug 23, 4:25 PM

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?

78

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

113

Was this line simply an error in the old version?

118

Needs an article here: "an IPv6 target".

132

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

137

Need another article: "the specific IP version".

170

Need another article: "an IPv4 target".

200

Ditch the comma.

203–213

"an IPv4 target"

209

"an IPv6 target"

234

"an IPv4 target"

239

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

308

s/target/targets/

311

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

404

"hostname or IPv4 address"

408

s/target/targets/

411

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

770

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

790

I would say "ignore the IPv4 RECORD_ROUTE option"

sbin/ping/ping.c
313

s/options/option/

sbin/ping/ping6.c
359

s/options/option/

This revision now requires changes to proceed.Fri, Aug 23, 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

Fixed.

sbin/ping/ping6.c
359

Fixed.

Add missing articles in the manual page.

sbin/ping/ping.8
118

Fixed.

137

Fixed.

170

Fixed.

203–213

Fixed.

209

Fixed.

234

Fixed.

239

The article has been added.

Fix typos in the manual page.

sbin/ping/ping.8
200

Done.

308

Fixed.

408

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?

132

Fixed.

311

Fixed.

404

Fixed.

411

Fixed.

770

Fixed.

790

Fixed.

asomers added inline comments.Fri, Aug 23, 5:46 PM
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.

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

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

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.

asomers added inline comments.Fri, Aug 23, 8:42 PM
sbin/ping/ping.8
239

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.Sat, Aug 24, 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.Sat, Aug 24, 3:13 PM

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