ping6: Fix alignment errors

[jansucan]

Summary

Fix alignment errors

This fixes -Wcast-align errors when compiled with WARNS=6.

Submitted by: Ján Sučan <[e-mail]>
Sponsored by: Google, inc. (Google Summer of Code 2019)

[asomers]

sbin/ping6/ping6.c

1334 ↗

(On Diff #60650)

Yes, it should be htons. The existing code works, though, because htons and ntohs are identical. My bigger worry is about the type of seq. Shouldn't it be short instead of int?

1400 ↗

(On Diff #60650)

This is a bug. ping6 making two calls to gettimeofday and comparing the (usually subsecond) output to compute a duration. That will produce incorrect results if the system clock changes in the meantime. The correct way to measure durations is with clock_gettime and CLOCK_MONOTONIC. However, this bug is wholly unrelated to your task at hand, so you should fix it separately.

1401 ↗

(On Diff #60650)

This line silently truncates tv.tv_sec down to 32 bits. It's ok given how it's used, but it looks like a bug. Better to annotate with a comment, and/or explicitly cast tv.tv_sec to a uint32_t.

[jansucan]

sbin/ping6/ping6.c

1334 ↗

(On Diff #60650)

Yes. It looks like the author wanted to use short when he or she used ntohs and type casting to u_int16_t. Would it be better to use short or uint16_t?

[asomers]

sbin/ping6/ping6.c

1334 ↗

(On Diff #60650)

The definition of htons says uint16_t, so that would be the best.

[jansucan]

sbin/ping6/ping6.c

1334 ↗

(On Diff #60650)

seq variable in other parts of the code uses u_int16_t. What's the difference between u_int16_t and uint16_t? Shouldn't only one "family" of fixed-width types be used in the code?

[asomers]

sbin/ping6/ping6.c

1334 ↗

(On Diff #60650)

Types like uint16_t were standardized by C99. Code that predates C99 used a plethora of nonstandard alternatives. New code should use the standard types.

[jansucan]

sbin/ping6/ping6.c

1334 ↗

(On Diff #60650)

So I will keep u_int16_t for the other seq variables and use uint16_t only for the single one.

[jansucan]

sbin/ping6/ping6.c

1334 ↗

(On Diff #60650)

Fixed in D21218 and D21219.

1400 ↗

(On Diff #60650)

Thanks. I looked at the gettimeofday calls in ping6. There are 5 of them. Three are used for getting timeout value for select and two are used for measuring a packet trip time. I think all of them should be replaced by clock_gettime with CLOCK_MONOTONIC. I'm going to do that.

1401 ↗

(On Diff #60650)

Fixed in D21220.

[jansucan]

sbin/ping6/ping6.c

1400 ↗

(On Diff #60650)

It's done D21226. But it's based on master so I will have to rebase it after D21220 is applied.

[jansucan]

Rebase to the master branch.

[asomers]

I combined this revision with D21260 and ran a "make tinderbox". The build failed with GCC.
Here's with GCC 4.2

cc1: warnings being treated as errors
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c: In function 'pr_nodeaddr':
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:2105: warning: 'ttl' may be used uninitialized in this function

And here's with GCC 8:

/home/asomers/freebsd/base/head/sbin/ping6/ping6.c: In function 'main':
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:963:15: warning: variable 'types' set but not used [-Wunused-but-set-variable]
   const char *types[1];
               ^~~~~
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:1171:4: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
    memset(cm, 0, CONTROLLEN);
    ^~~~~~
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c: In function 'capdns_setup':
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:2816:6: warning: variable 'families' set but not used [-Wunused-but-set-variable]
  int families[1];
      ^~~~~~~~
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:2815:14: warning: variable 'types' set but not used [-Wunused-but-set-variable]
  const char *types[2];
              ^~~~~
At top level:
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:69:19: warning: 'copyright' defined but not used [-Wunused-const-variable=]
 static const char copyright[] =
                   ^~~~~~~~~
In file included from /home/asomers/freebsd/base/head/sys/net/if.h:47,
                 from /home/asomers/freebsd/base/head/sbin/ping6/ping6.c:111:
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c: In function 'main':
/home/asomers/freebsd/base/head/sys/sys/time.h:380:35: error: 'intvl.tv_nsec' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   (vsp)->tv_nsec = (tsp)->tv_nsec + (usp)->tv_nsec; \
                                   ^
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:298:24: note: 'intvl.tv_nsec' was declared here
  struct timespec last, intvl;
                        ^~~~~
In file included from /home/asomers/freebsd/base/head/sys/net/if.h:47,
                 from /home/asomers/freebsd/base/head/sbin/ping6/ping6.c:111:
/home/asomers/freebsd/base/head/sys/sys/time.h:379:33: error: 'intvl.tv_sec' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   (vsp)->tv_sec = (tsp)->tv_sec + (usp)->tv_sec;  \
                                 ^
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:298:24: note: 'intvl.tv_sec' was declared here
  struct timespec last, intvl;
                        ^~~~~
cc1: all warnings being treated as errors
--- ping6.o ---
*** [ping6.o] Error code 1

The easiest way to reproduce the full failure with GCC8 (easy, but not fast), is to install the riscv64-xtoolchain-gcc package and then do env CROSS_TOOLCHAIN=riscv64-gcc TARGET=riscv TARGET_ARCH=riscv64 make -jwhatever buildworld.

[jansucan]

All of the warnings should be fixed (D21269, D21270, D21271, D21273, D21275).