Bogus check for negative values on an unsigned number.

This causes compiler warnings when using Clang instead of default GCC.

---
 mroute.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

index 285b151..0213a3e 100644
@@ -516,29 +516,23 @@ void
 mroute_helper_add_iroute6 (struct mroute_helper *mh, 
                            const struct iroute_ipv6 *ir6)
 {
-  if (ir6->netbits >= 0)
-    {
-      ASSERT (ir6->netbits < MR_HELPER_NET_LEN);
-      ++mh->cache_generation;
-      ++mh->net_len_refcount[ir6->netbits];
-      if (mh->net_len_refcount[ir6->netbits] == 1)
+  ASSERT (ir6->netbits < MR_HELPER_NET_LEN);
+  ++mh->cache_generation;
+  ++mh->net_len_refcount[ir6->netbits];
+  if (mh->net_len_refcount[ir6->netbits] == 1)
 	mroute_helper_regenerate (mh);
-    }
 }
 
 void
 mroute_helper_del_iroute6 (struct mroute_helper *mh, 
 			   const struct iroute_ipv6 *ir6)
 {
-  if (ir6->netbits >= 0)
-    {
-      ASSERT (ir6->netbits < MR_HELPER_NET_LEN);
-      ++mh->cache_generation;
-      --mh->net_len_refcount[ir6->netbits];
-      ASSERT (mh->net_len_refcount[ir6->netbits] >= 0);
-      if (!mh->net_len_refcount[ir6->netbits])
+  ASSERT (ir6->netbits < MR_HELPER_NET_LEN);
+  ++mh->cache_generation;
+  --mh->net_len_refcount[ir6->netbits];
+  ASSERT (mh->net_len_refcount[ir6->netbits] >= 0);
+  if (!mh->net_len_refcount[ir6->netbits])
 	mroute_helper_regenerate (mh);
-    }
 }
 
 void

1.7.5.4

Hi,

I'm not particularily interested in source code fixes "just to silence a compiler warning" - but Clang is right, of course, that this variable cannot ever be negative, and thus it should be cleaned up :-)

I'm not ACKing this patch anyway, because I think the issue is bigger - I want to look at the IPv4 code paths and see whether the IPv4 "netbits" structure element can ever be negative - and if yes, figure out whether this should be possible for IPv6, and if not, convert the IPv4 netbits to "unsigned int" as well, and get rid of the if() statement there as well.

Thanks for reporting this. I'll look into it.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

I totally agree on not ACKing the patch and your solution description ;-) I tried to start a discussion on IRC about the preferred solution before sending this patch, but that didn't get very far.

Op 17 februari 2012 22:58 heeft Gert Doering <[email protected]...> het volgende geschreven:

Hi,

I've dug a bit further into the code, and the IPv4 and IPv6 code differ here - IPv4 flags "a host iroute" with ir->netbits = -1, while for IPv6, a "host iroute" is just a /128.

The difference shows up in

- options.c, option_iroute() vs. option_iroute_ipv6()
- push.c in checking "if < 0 then make it a /32":

    if (network == ir->network && netmask == netbits_to_netmask (ir->netbits >= 0 ? ir->netbits : 32))

- multi.c, in pretty-printing host routes "without the /32":

          if (ir->netbits >= 0)
            msg (D_MULTI_LOW, "MULTI: internal route %s/%d -> %s",
...
          else
            msg (D_MULTI_LOW, "MULTI: internal route %s -> %s",

(which I blindly copied for IPv6, not noticing that ir6->netbits cannot
be negative anyway)

- mroute.c, in the mroute_helper_{add,del}_iroute[6]() helpers that you've found

Now I'm wondering which way forward is cleaner - make ip->netbits an "unsigned" as well, changing all the places where "<0 == host route" to be "/32 = host" (mostly prettyprinting I think, but need to understand the mroute stuff better), or live with artificial IPv4/IPv6 asymmetry here...

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...