On Tue, Jul 04, 2017 at 11:24:46PM +0200, Klemens Nanni wrote:
> On Tue, Jul 04, 2017 at 03:58:13PM +0000, Florian Obser wrote:
> >I like most (all? of the correct ones), can you have another go
> >at trying to fix the pointed out changes in behaviour?
>
> Thanks natano for pointing out these stupid mistakes.
>
> Unify option checking, check for F_RROUTE only if F_HDRINCL is not set
> already (they're incompatible and checked together already beforehand).
>
> Feedback/OK?
>
> Index: ping.c
> ===================================================================
> RCS file: /cvs/src/sbin/ping/ping.c,v
> retrieving revision 1.220
> diff -u -p -r1.220 ping.c
> --- ping.c 4 Jul 2017 15:55:22 -0000 1.220
> +++ ping.c 4 Jul 2017 21:18:36 -0000
> @@ -562,10 +562,10 @@ main(int argc, char *argv[])
> (void)setsockopt(s, SOL_SOCKET, SO_DEBUG, &optval,
> sizeof(optval));
>
> - if ((options & F_FLOOD) && (options & F_INTERVAL))
> + if (options & F_FLOOD && options & F_INTERVAL)
I went in the oposite direction I think the former is easier on the
eyes. Both forms are equivalent though.
> errx(1, "-f and -i options are incompatible");
>
> - if ((options & F_FLOOD) && (options & (F_AUD_RECV | F_AUD_MISS)))
> + if (options & F_FLOOD && options & (F_AUD_RECV | F_AUD_MISS))
> warnx("No audible output for flood pings");
>
> if (datalen >= sizeof(struct payload)) /* can we time transfer */
> @@ -621,7 +621,7 @@ main(int argc, char *argv[])
> * let the kernel pass extension headers of incoming packets,
> * for privileged socket options
> */
> - if ((options & F_VERBOSE) != 0) {
> + if (options & F_VERBOSE) {
yes
> int opton = 1;
>
> if (setsockopt(s, IPPROTO_IPV6, IPV6_RECVHOPOPTS,
> @@ -717,10 +717,8 @@ main(int argc, char *argv[])
> else
> ip->ip_src.s_addr = INADDR_ANY;
> ip->ip_dst = dst4.sin_addr;
> - }
> -
> /* record route option */
> - if (options & F_RROUTE) {
> + } else if (options & F_RROUTE) {
no
Here is why:
First we check that F_RROUTE and F_HDRINCL are not set at the same
time. We need to do this because of the way the following code is
written. I think we might be able to allow both at the same time.
After that there is code that does things if F_RROUTE and there is
code that does things if F_HDRINCL is set.
But both pieces of code are independent. The code blocks are not
of the form if this do that, otherwise do the other thing.
Adding else if() makes it harder to figure out what is going on.
> if (IN_MULTICAST(ntohl(dst4.sin_addr.s_addr)))
> errx(1, "record route not valid to multicast"
> " destinations");
> @@ -771,7 +769,7 @@ main(int argc, char *argv[])
> (void)signal(SIGINT, onsignal);
> (void)signal(SIGINFO, onsignal);
>
> - if ((options & F_FLOOD) == 0) {
> + if (!(options & F_FLOOD)) {
yes
> (void)signal(SIGALRM, onsignal);
> itimer.it_interval = interval;
> itimer.it_value = interval;
> @@ -805,7 +803,7 @@ main(int argc, char *argv[])
> if (ntransmitted - nreceived - 1 > nmissedmax) {
> nmissedmax = ntransmitted - nreceived - 1;
> if (!(options & F_FLOOD) &&
> - (options & F_AUD_MISS))
> + options & F_AUD_MISS)
> (void)fputc('\a', stderr);
> }
> continue;
> @@ -859,7 +857,7 @@ main(int argc, char *argv[])
> * a path MTU notification.)
> */
> if ((mtu = get_pathmtu(&m, &dst6)) > 0) {
> - if ((options & F_VERBOSE) != 0) {
> + if (options & F_VERBOSE) {
yes
> printf("new path MTU (%d) is "
> "notified\n", mtu);
> }
> @@ -959,7 +957,7 @@ pr_addr(struct sockaddr *addr, socklen_t
> static char buf[NI_MAXHOST];
> int flag = 0;
>
> - if ((options & F_HOSTNAME) == 0)
> + if (!(options & F_HOSTNAME))
yes
> flag |= NI_NUMERICHOST;
>
> if (getnameinfo(addr, addrlen, buf, sizeof(buf), NULL, 0, flag) == 0)
> @@ -1890,7 +1888,7 @@ get_pathmtu(struct msghdr *mhdr, struct
>
> dst->sin6_scope_id &&
> mtuctl->ip6m_addr.sin6_scope_id !=
> dst->sin6_scope_id)) {
> - if ((options & F_VERBOSE) != 0) {
> + if (options & F_VERBOSE) {
yes
> printf("path MTU for %s is notified. "
> "(ignored)\n",
> pr_addr((struct sockaddr *)
>
So I commited most things. Thanks for the effort!
--
I'm not entirely sure you are real.
