Just wanted to capture a discussion Paul and I had off list. The afi_t change in this patch breaks assumptions about data structures in bgpd/bgp_open.c and the handling of capability codes. I've fixed those in http://patchwork.quagga.net/patch/1748/ when I was looking at some zebra refactoring. I plan to merge this patch with Paul's afi_t typedef and it's fix of the clang issues with my bgp_open.c changes. I'll send something along here shortly.
This still leaves the change of the safi_t. Switching over to an enum for it doesn't work that well because bgp defines SAFI_MPLS_LABELED_VPN as 128. We would need to include this in the enum. The problem is that both bgp and zebra( maybe more ) create multidimensional arrays based upon SAFI_MAX and AFI_MAX. This mixing of indices and sizes causes issues. We need a generic methodology to map afi/safi to an indice to create one dimensional arrays that are appropriately sized. At this point that's a decent amount of work that would need some major testing to get right. I'm going to hold off on that at the moment, unless someone wants to tackle this issue separately donald On Tue, Dec 15, 2015 at 11:22 AM, Paul Jakma <[email protected]> wrote: > On Tue, 15 Dec 2015, Donald Sharp wrote: > > Martin - >> >> As discussed in the meeting today here is an example of a bogus issue: >> >> >> https://ci1.netdef.org/artifact/QUAGGA-QMASTER/shared/build-82/static_analysis/report-8a9e85.html#EndPath >> >> Basically the error message boils down to this code construct: >> >> struct prefix p; >> if (afi == v4) >> set p appropriately >> else if (afi == v6) >> set p appropriately >> >> if (p.prefixlen) <------- If afi is not v4 or v6 then prefix length is >> not going to be set. >> >> I spent a few minutes looking at clang and saw no good way to mark the >> code >> in plist.c as ok. >> > > To be fair, the compiler is technically correct - the best kind of > correct. :) > > It'd be nice if setting afi_t to an enum with just those 2 values was > sufficient, but it seems clang still does: > > if (afi == v4) > /* take false */ > if (afi == v6) > /* take false */ > > However, enum + a switch with just those 2 values and clang shuts up. See > below. > > A more compact diff would just be: > > if (!(afi == AFI_IP || afi == AFI_IP6)) > return CMD_WARNING; > > at the top of that func, but the enum could help compilers. > > diff --git a/lib/plist.c b/lib/plist.c > index f9e626d..6e00869 100644 > --- a/lib/plist.c > +++ b/lib/plist.c > @@ -698,44 +698,45 @@ vty_prefix_list_install (struct vty *vty, afi_t afi, > const char *name, > } > > /* "any" is special token for matching any IPv4 addresses. */ > - if (afi == AFI_IP) > - { > - if (strncmp ("any", prefix, strlen (prefix)) == 0) > - { > - ret = str2prefix_ipv4 ("0.0.0.0/0", (struct prefix_ipv4 *) &p); > - genum = 0; > - lenum = IPV4_MAX_BITLEN; > - any = 1; > - } > - else > - ret = str2prefix_ipv4 (prefix, (struct prefix_ipv4 *) &p); > - > - if (ret <= 0) > - { > - vty_out (vty, "%% Malformed IPv4 prefix%s", VTY_NEWLINE); > - return CMD_WARNING; > - } > - } > -#ifdef HAVE_IPV6 > - else if (afi == AFI_IP6) > + switch (afi) > { > - if (strncmp ("any", prefix, strlen (prefix)) == 0) > - { > - ret = str2prefix_ipv6 ("::/0", (struct prefix_ipv6 *) &p); > - genum = 0; > - lenum = IPV6_MAX_BITLEN; > - any = 1; > - } > - else > - ret = str2prefix_ipv6 (prefix, (struct prefix_ipv6 *) &p); > - > - if (ret <= 0) > - { > - vty_out (vty, "%% Malformed IPv6 prefix%s", VTY_NEWLINE); > - return CMD_WARNING; > - } > + case AFI_IP: > + { > + if (strncmp ("any", prefix, strlen (prefix)) == 0) > + { > + ret = str2prefix_ipv4 ("0.0.0.0/0", (struct prefix_ipv4 *) > &p); > + genum = 0; > + lenum = IPV4_MAX_BITLEN; > + any = 1; > + } > + else > + ret = str2prefix_ipv4 (prefix, (struct prefix_ipv4 *) &p); > + > + if (ret <= 0) > + { > + vty_out (vty, "%% Malformed IPv4 prefix%s", VTY_NEWLINE); > + return CMD_WARNING; > + } > + } break; > + case AFI_IP6: > + { > + if (strncmp ("any", prefix, strlen (prefix)) == 0) > + { > + ret = str2prefix_ipv6 ("::/0", (struct prefix_ipv6 *) &p); > + genum = 0; > + lenum = IPV6_MAX_BITLEN; > + any = 1; > + } > + else > + ret = str2prefix_ipv6 (prefix, (struct prefix_ipv6 *) &p); > + > + if (ret <= 0) > + { > + vty_out (vty, "%% Malformed IPv6 prefix%s", VTY_NEWLINE); > + return CMD_WARNING; > + } > + } break; > } > -#endif /* HAVE_IPV6 */ > > /* ge and le check. */ > if (genum && (genum <= p.prefixlen)) > diff --git a/lib/zebra.h b/lib/zebra.h > index a607437..97ed05c 100644 > --- a/lib/zebra.h > +++ b/lib/zebra.h > @@ -483,15 +483,19 @@ extern const char *zserv_command_string (unsigned > int command); > #endif > > /* Address family numbers from RFC1700. */ > -#define AFI_IP 1 > -#define AFI_IP6 2 > +typedef enum { > + AFI_IP = 1, > + AFI_IP6 = 2, > +} afi_t; > #define AFI_MAX 3 > > /* Subsequent Address Family Identifier. */ > -#define SAFI_UNICAST 1 > -#define SAFI_MULTICAST 2 > -#define SAFI_RESERVED_3 3 > -#define SAFI_MPLS_VPN 4 > +typedef enum { > + SAFI_UNICAST = 1, > + SAFI_MULTICAST = 2, > + SAFI_RESERVED_3 = 3, > + SAFI_MPLS_VPN = 4, > +} safi_t; > #define SAFI_MAX 5 > > /* Filter direction. */ > @@ -516,10 +520,6 @@ extern const char *zserv_command_string (unsigned int > command); > #define SET_FLAG(V,F) (V) |= (F) > #define UNSET_FLAG(V,F) (V) &= ~(F) > > -/* AFI and SAFI type. */ > -typedef u_int16_t afi_t; > -typedef u_int8_t safi_t; > - > /* Zebra types. Used in Zserv message header. */ > typedef u_int16_t zebra_size_t; > typedef u_int16_t zebra_command_t; > > regards, > -- > Paul Jakma [email protected] @pjakma Key ID: 64A2FF6A > Fortune: > Going the speed of light is bad for your age. >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
