[Toybox] [PATCH] ping: fix print error and delete redundant checksum.

[Rob Landley]

On 2/25/20 12:02 PM, anatasluo wrote:
> Signed-off-by: anatasluo <luolongjuna at gmail.com>
> ---
>  toys/net/ping.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/toys/net/ping.c b/toys/net/ping.c
> index 9ae7c856..e27d6c6c 100644
> --- a/toys/net/ping.c
> +++ b/toys/net/ping.c
> @@ -62,28 +62,12 @@ static void summary(int sig)
>      printf("\n--- %s ping statistics ---\n", ntop(TT.sa));
>      printf("%lu packets transmitted, %lu received, %ld%% packet loss\n",
>        TT.sent, TT.recv, ((TT.sent-TT.recv)*100)/(TT.sent?TT.sent:1));
> -    printf("round-trip min/avg/max = %lu/%lu/%lu ms\n",
> -      TT.min, TT.max, TT.fugit/(TT.recv?TT.recv:1));
> +    if (TT.recv) printf("round-trip min/avg/max = %lu/%lu/%lu ms\n",
> +      TT.min, TT.fugit/(TT.recv?TT.recv:1), TT.max);

Oops, yes the order is wrong. (All my scripts just use ping -c 3 for "is this
connection up y/n".)

If you're already checking for TT.recv on the first line you don't need the
TT.recv ? TT.recv : 1 on the second line.

> @@ -146,9 +130,9 @@ void ping_main(void)
>    // Open DGRAM socket
>    sa->sa_family = ai->ai_family;
>    TT.sock = socket(ai->ai_family, SOCK_DGRAM,
> -    len = (ai->ai_family == AF_INET) ? IPPROTO_ICMP : IPPROTO_ICMPV6);
> +    (ai->ai_family == AF_INET) ? IPPROTO_ICMP : IPPROTO_ICMPV6);
>    if (TT.sock == -1) {
> -    perror_msg("socket SOCK_DGRAM %x", len);
> +    perror_msg("socket SOCK_DGRAM %x", ai->ai_family);

That's not what we fed to the socket() call. You're discarding information that
helps debug what it tried to do (and thus why it failed).

I suppose you could get the same from strace, but... why?

>      if (errno == EACCES) {
>        fprintf(stderr, "Kernel bug workaround (as root):\n");
>        fprintf(stderr, "echo 0 9999999 > /proc/sys/net/ipv4/ping_group_range\n");
> @@ -180,6 +164,7 @@ void ping_main(void)
>      // 20 byte TCP header, 8 byte ICMP header, plus data payload
>      printf(": %ld(%ld) bytes.\n", TT.s, TT.s+28);
>    }
> +  TT.min = ULLONG_MAX;
>    toys.exitval = 1;
>  
>    tW = tw = 0;
> @@ -215,7 +200,7 @@ void ping_main(void)
>        if (TT.s >= 4) *(unsigned *)(ih+1) = tnow;
>  
>        ih->checksum = 0;
> -      ih->checksum = pingchksum((void *)toybuf, TT.s+sizeof(*ih));
> +

Back when I wrote this, I was having outgoing ping packets with improper
checksum discarded by some corporate firewall. The = 0 is leftover from
debugging and should be removed, but setting the checksum is a thing that
matters sometimes.

>        xsendto(TT.sock, toybuf, TT.s+sizeof(*ih), TT.sa);
>        TT.sent++;
>        if ((toys.optflags&(FLAG_f|FLAG_q)) == FLAG_f) xputc('.');
> @@ -238,6 +223,8 @@ void ping_main(void)
>  
>      TT.recv++;
>      TT.fugit += (pkttime = millitime()-*(unsigned *)(ih+1));
> +    if (pkttime < TT.min) TT.min = pkttime;
> +    if (pkttime > TT.max) TT.max = pkttime;

Ok, that's a thing that it should have been doing, yes. :)

I applied parts of your patch and switched to FLAG() macros while I was there.

Thanks,

Rob