Skip to content

Commit

Permalink
*: simplify log message lookup
Browse files Browse the repository at this point in the history 
log.c provides functionality for associating a constant (typically a
protocol constant) with a string and finding the string given the
constant. However this is highly delicate code that is extremely prone
to stack overflows and off-by-one's due to requiring the developer to
always remember to update the array size constant and to do so correctly
which, as shown by example, is never a good idea.b

The original goal of this code was to try to implement lookups in O(1)
time without a linear search through the message array. Since this code
is used 99% of the time for debugs, it's worth the 5-6 additional cmp's
worst case if it means we avoid explitable bugs due to oversights...

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
  • Loading branch information
@qlyoung
qlyoung committed Jun 21, 2017
1 parent d368cd4 commit 56b4067
Show file tree
Hide file tree
Showing 33 changed files with 198 additions and 244 deletions.
25 changes: 13 additions & 12 deletions bgpd/bgp_attr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@ static const struct message attr_str [] =
{ BGP_ATTR_VNC, "VNC" },
#endif
{ BGP_ATTR_LARGE_COMMUNITIES, "LARGE_COMMUNITY" },
{ BGP_ATTR_PREFIX_SID, "PREFIX_SID" }
{ BGP_ATTR_PREFIX_SID, "PREFIX_SID" },
{ 0 }
};
static const int attr_str_max = array_size(attr_str);

static const struct message attr_flag_str[] =
{
Expand All @@ -90,6 +90,7 @@ static const struct message attr_flag_str[] =
{ BGP_ATTR_FLAG_PARTIAL, "Partial" },
/* bgp_attr_flags_diagnose() relies on this bit being last in this list */
{ BGP_ATTR_FLAG_EXTLEN, "Extended Length" },
{ 0 }
};

static struct hash *cluster_hash;
Expand Down Expand Up @@ -1239,7 +1240,7 @@ bgp_attr_flags_diagnose (struct bgp_attr_parser_args *args,
)
{
zlog_err ("%s attribute must%s be flagged as \"%s\"",
LOOKUP (attr_str, attr_code),
lookup_msg(attr_str, attr_code, NULL),
CHECK_FLAG (desired_flags, attr_flag_str[i].key) ? "" : " not",
attr_flag_str[i].str);
seen = 1;
Expand All @@ -1248,7 +1249,7 @@ bgp_attr_flags_diagnose (struct bgp_attr_parser_args *args,
{
zlog_debug ("Strange, %s called for attr %s, but no problem found with flags"
" (real flags 0x%x, desired 0x%x)",
__func__, LOOKUP (attr_str, attr_code),
__func__, lookup_msg(attr_str, attr_code, NULL),
real_flags, desired_flags);
}
}
Expand Down Expand Up @@ -1297,7 +1298,7 @@ bgp_attr_flag_invalid (struct bgp_attr_parser_args *args)
&& !CHECK_FLAG (BGP_ATTR_FLAG_TRANS, flags))
{
zlog_err ("%s well-known attributes must have transitive flag set (%x)",
LOOKUP (attr_str, attr_code), flags);
lookup_msg(attr_str, attr_code, NULL), flags);
return 1;
}

Expand All @@ -1310,15 +1311,15 @@ bgp_attr_flag_invalid (struct bgp_attr_parser_args *args)
{
zlog_err ("%s well-known attribute "
"must NOT have the partial flag set (%x)",
LOOKUP (attr_str, attr_code), flags);
lookup_msg(attr_str, attr_code, NULL), flags);
return 1;
}
if (CHECK_FLAG (flags, BGP_ATTR_FLAG_OPTIONAL)
&& !CHECK_FLAG (flags, BGP_ATTR_FLAG_TRANS))
{
zlog_err ("%s optional + transitive attribute "
"must NOT have the partial flag set (%x)",
LOOKUP (attr_str, attr_code), flags);
lookup_msg(attr_str, attr_code, NULL), flags);
return 1;
}
}
Expand Down Expand Up @@ -2460,7 +2461,7 @@ bgp_attr_check (struct peer *peer, struct attr *attr)
if (type)
{
zlog_warn ("%s Missing well-known attribute %s.", peer->host,
LOOKUP (attr_str, type));
lookup_msg(attr_str, type, NULL));
bgp_notify_send_with_data (peer,
BGP_NOTIFY_UPDATE_ERR,
BGP_NOTIFY_UPDATE_MISS_ATTR,
Expand Down Expand Up @@ -2679,7 +2680,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
{
zlog_warn ("%s: Attribute %s, parse error",
peer->host,
LOOKUP (attr_str, type));
lookup_msg(attr_str, type, NULL));
if (as4_path)
aspath_unintern (&as4_path);
return ret;
Expand All @@ -2689,7 +2690,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,

zlog_warn ("%s: Attribute %s, parse error - treating as withdrawal",
peer->host,
LOOKUP (attr_str, type));
lookup_msg(attr_str, type, NULL));
if (as4_path)
aspath_unintern (&as4_path);
return ret;
Expand All @@ -2699,7 +2700,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
if (BGP_INPUT_PNT (peer) != attr_endp)
{
zlog_warn ("%s: BGP attribute %s, fetch error",
peer->host, LOOKUP (attr_str, type));
peer->host, lookup_msg(attr_str, type, NULL));
bgp_notify_send (peer,
BGP_NOTIFY_UPDATE_ERR,
BGP_NOTIFY_UPDATE_ATTR_LENG_ERR);
Expand All @@ -2713,7 +2714,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
if (BGP_INPUT_PNT (peer) != endp)
{
zlog_warn ("%s: BGP attribute %s, length mismatch",
peer->host, LOOKUP (attr_str, type));
peer->host, lookup_msg(attr_str, type, NULL));
bgp_notify_send (peer,
BGP_NOTIFY_UPDATE_ERR,
BGP_NOTIFY_UPDATE_ATTR_LENG_ERR);
Expand Down
28 changes: 14 additions & 14 deletions bgpd/bgp_debug.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ const struct message bgp_status_msg[] =
{ Established, "Established" },
{ Clearing, "Clearing" },
{ Deleted, "Deleted" },
{ 0 }
};
const int bgp_status_msg_max = BGP_STATUS_MAX;

/* BGP message type string. */
const char *bgp_type_str[] =
Expand All @@ -110,16 +110,16 @@ static const struct message bgp_notify_msg[] =
{ BGP_NOTIFY_FSM_ERR, "Neighbor Events Error"},
{ BGP_NOTIFY_CEASE, "Cease"},
{ BGP_NOTIFY_CAPABILITY_ERR, "CAPABILITY Message Error"},
{ 0 }
};
static const int bgp_notify_msg_max = BGP_NOTIFY_MAX;

static const struct message bgp_notify_head_msg[] =
{
{ BGP_NOTIFY_HEADER_NOT_SYNC, "/Connection Not Synchronized"},
{ BGP_NOTIFY_HEADER_BAD_MESLEN, "/Bad Message Length"},
{ BGP_NOTIFY_HEADER_BAD_MESTYPE, "/Bad Message Type"}
{ BGP_NOTIFY_HEADER_BAD_MESTYPE, "/Bad Message Type"},
{ 0 }
};
static const int bgp_notify_head_msg_max = BGP_NOTIFY_HEADER_MAX;

static const struct message bgp_notify_open_msg[] =
{
Expand All @@ -131,8 +131,8 @@ static const struct message bgp_notify_open_msg[] =
{ BGP_NOTIFY_OPEN_AUTH_FAILURE, "/Authentication Failure"},
{ BGP_NOTIFY_OPEN_UNACEP_HOLDTIME, "/Unacceptable Hold Time"},
{ BGP_NOTIFY_OPEN_UNSUP_CAPBL, "/Unsupported Capability"},
{ 0 }
};
static const int bgp_notify_open_msg_max = BGP_NOTIFY_OPEN_MAX;

static const struct message bgp_notify_update_msg[] =
{
Expand All @@ -148,8 +148,8 @@ static const struct message bgp_notify_update_msg[] =
{ BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, "/Optional Attribute Error"},
{ BGP_NOTIFY_UPDATE_INVAL_NETWORK, "/Invalid Network Field"},
{ BGP_NOTIFY_UPDATE_MAL_AS_PATH, "/Malformed AS_PATH"},
{ 0 }
};
static const int bgp_notify_update_msg_max = BGP_NOTIFY_UPDATE_MAX;

static const struct message bgp_notify_cease_msg[] =
{
Expand All @@ -162,17 +162,17 @@ static const struct message bgp_notify_cease_msg[] =
{ BGP_NOTIFY_CEASE_CONFIG_CHANGE, "/Other Configuration Change"},
{ BGP_NOTIFY_CEASE_COLLISION_RESOLUTION, "/Connection collision resolution"},
{ BGP_NOTIFY_CEASE_OUT_OF_RESOURCE, "/Out of Resource"},
{ 0 }
};
static const int bgp_notify_cease_msg_max = BGP_NOTIFY_CEASE_MAX;

static const struct message bgp_notify_capability_msg[] =
{
{ BGP_NOTIFY_SUBCODE_UNSPECIFIC, "/Unspecific"},
{ BGP_NOTIFY_CAPABILITY_INVALID_ACTION, "/Invalid Action Value" },
{ BGP_NOTIFY_CAPABILITY_INVALID_LENGTH, "/Invalid Capability Length"},
{ BGP_NOTIFY_CAPABILITY_MALFORMED_CODE, "/Malformed Capability Value"},
{ 0 }
};
static const int bgp_notify_capability_msg_max = BGP_NOTIFY_CAPABILITY_MAX;

/* Origin strings. */
const char *bgp_origin_str[] = {"i","e","?"};
Expand Down Expand Up @@ -464,7 +464,7 @@ bgp_dump_attr (struct attr *attr, char *buf, size_t size)
const char *
bgp_notify_code_str (char code)
{
return LOOKUP_DEF (bgp_notify_msg, code, "Unrecognized Error Code");
return lookup_msg (bgp_notify_msg, code, "Unrecognized Error Code");
}

const char *
Expand All @@ -474,23 +474,23 @@ bgp_notify_subcode_str (char code, char subcode)
switch (code)
{
case BGP_NOTIFY_HEADER_ERR:
return LOOKUP_DEF (bgp_notify_head_msg, subcode,
return lookup_msg (bgp_notify_head_msg, subcode,
"Unrecognized Error Subcode");
case BGP_NOTIFY_OPEN_ERR:
return LOOKUP_DEF (bgp_notify_open_msg, subcode,
return lookup_msg (bgp_notify_open_msg, subcode,
"Unrecognized Error Subcode");
case BGP_NOTIFY_UPDATE_ERR:
return LOOKUP_DEF (bgp_notify_update_msg, subcode,
return lookup_msg (bgp_notify_update_msg, subcode,
"Unrecognized Error Subcode");
case BGP_NOTIFY_HOLD_ERR:
break;
case BGP_NOTIFY_FSM_ERR:
break;
case BGP_NOTIFY_CEASE:
return LOOKUP_DEF (bgp_notify_cease_msg, subcode,
return lookup_msg (bgp_notify_cease_msg, subcode,
"Unrecognized Error Subcode");
case BGP_NOTIFY_CAPABILITY_ERR:
return LOOKUP_DEF (bgp_notify_capability_msg, subcode,
return lookup_msg (bgp_notify_capability_msg, subcode,
"Unrecognized Error Subcode");
}
return "";
Expand Down
1 change: 0 additions & 1 deletion bgpd/bgp_debug.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ extern const char *bgp_notify_subcode_str(char, char);
extern void bgp_notify_print (struct peer *, struct bgp_notify *, const char *);

extern const struct message bgp_status_msg[];
extern const int bgp_status_msg_max;
extern int bgp_debug_neighbor_events(struct peer *peer);
extern int bgp_debug_keepalive(struct peer *peer);
extern int bgp_debug_update(struct peer *peer, struct prefix *p,
Expand Down
16 changes: 8 additions & 8 deletions bgpd/bgp_fsm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,8 +971,8 @@ bgp_fsm_change_status (struct peer *peer, int status)
if (bgp_debug_neighbor_events(peer))
zlog_debug ("%s went from %s to %s",
peer->host,
LOOKUP (bgp_status_msg, peer->ostatus),
LOOKUP (bgp_status_msg, peer->status));
lookup_msg(bgp_status_msg, peer->ostatus, NULL),
lookup_msg(bgp_status_msg, peer->status, NULL));
}

/* Flush the event queue and ensure the peer is shut down */
Expand Down Expand Up @@ -1413,7 +1413,7 @@ static int
bgp_fsm_event_error (struct peer *peer)
{
zlog_err ("%s [FSM] unexpected packet received in state %s",
peer->host, LOOKUP (bgp_status_msg, peer->status));
peer->host, lookup_msg(bgp_status_msg, peer->status, NULL));

return bgp_stop_with_notify (peer, BGP_NOTIFY_FSM_ERR, 0);
}
Expand Down Expand Up @@ -1599,7 +1599,7 @@ bgp_ignore (struct peer *peer)
{
zlog_err ("%s [FSM] Ignoring event %s in state %s, prior events %s, %s, fd %d",
peer->host, bgp_event_str[peer->cur_event],
LOOKUP (bgp_status_msg, peer->status),
lookup_msg(bgp_status_msg, peer->status, NULL),
bgp_event_str[peer->last_event],
bgp_event_str[peer->last_major_event], peer->fd);
return 0;
Expand All @@ -1611,7 +1611,7 @@ bgp_fsm_exeption (struct peer *peer)
{
zlog_err ("%s [FSM] Unexpected event %s in state %s, prior events %s, %s, fd %d",
peer->host, bgp_event_str[peer->cur_event],
LOOKUP (bgp_status_msg, peer->status),
lookup_msg(bgp_status_msg, peer->status, NULL),
bgp_event_str[peer->last_event],
bgp_event_str[peer->last_major_event], peer->fd);
return(bgp_stop (peer));
Expand Down Expand Up @@ -1837,8 +1837,8 @@ bgp_event_update (struct peer *peer, int event)
if (bgp_debug_neighbor_events(peer) && peer->status != next)
zlog_debug ("%s [FSM] %s (%s->%s), fd %d", peer->host,
bgp_event_str[event],
LOOKUP (bgp_status_msg, peer->status),
LOOKUP (bgp_status_msg, next), peer->fd);
lookup_msg(bgp_status_msg, peer->status, NULL),
lookup_msg(bgp_status_msg, next, NULL), peer->fd);

peer->last_event = peer->cur_event;
peer->cur_event = event;
Expand Down Expand Up @@ -1873,7 +1873,7 @@ bgp_event_update (struct peer *peer, int event)
zlog_err ("%s [FSM] Failure handling event %s in state %s, "
"prior events %s, %s, fd %d",
peer->host, bgp_event_str[peer->cur_event],
LOOKUP (bgp_status_msg, peer->status),
lookup_msg(bgp_status_msg, peer->status, NULL),
bgp_event_str[peer->last_event],
bgp_event_str[peer->last_major_event], peer->fd);
bgp_stop (peer);
Expand Down
20 changes: 10 additions & 10 deletions bgpd/bgp_open.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,16 +251,16 @@ static const struct message orf_type_str[] =
{
{ ORF_TYPE_PREFIX, "Prefixlist" },
{ ORF_TYPE_PREFIX_OLD, "Prefixlist (old)" },
{ 0 }
};
static const int orf_type_str_max = array_size(orf_type_str);

static const struct message orf_mode_str[] =
{
{ ORF_MODE_RECEIVE, "Receive" },
{ ORF_MODE_SEND, "Send" },
{ ORF_MODE_BOTH, "Both" },
{ 0 }
};
static const int orf_mode_str_max = array_size(orf_mode_str);

static int
bgp_capability_orf_entry (struct peer *peer, struct capability_header *hdr)
Expand Down Expand Up @@ -366,8 +366,8 @@ bgp_capability_orf_entry (struct peer *peer, struct capability_header *hdr)
if (bgp_debug_neighbor_events(peer))
zlog_debug ("%s OPEN has %s ORF capability"
" as %s for afi/safi: %d/%d",
peer->host, LOOKUP (orf_type_str, type),
LOOKUP (orf_mode_str, mode),
peer->host, lookup_msg(orf_type_str, type, NULL),
lookup_msg(orf_mode_str, mode, NULL),
pkt_afi, pkt_safi);

if (hdr->code == CAPABILITY_CODE_ORF)
Expand Down Expand Up @@ -716,9 +716,9 @@ bgp_capability_hostname (struct peer *peer, struct capability_header *hdr)
{ CAPABILITY_CODE_DYNAMIC_OLD, "Dynamic (Old)" },
{ CAPABILITY_CODE_REFRESH_OLD, "Route Refresh (Old)" },
{ CAPABILITY_CODE_ORF_OLD, "ORF (Old)" },
{ CAPABILITY_CODE_FQDN, "FQDN" },
{ CAPABILITY_CODE_FQDN, "FQDN" },
{ 0 }
};
static const int capcode_str_max = array_size(capcode_str);

/* Minimum sizes for length field of each cap (so not inc. the header) */
static const size_t cap_minsizes[] =
Expand Down Expand Up @@ -805,7 +805,7 @@ bgp_capability_parse (struct peer *peer, size_t length, int *mp_capability,
if (bgp_debug_neighbor_events(peer))
zlog_debug ("%s OPEN has %s capability (%u), length %u",
peer->host,
LOOKUP (capcode_str, caphdr.code),
lookup_msg(capcode_str, caphdr.code, NULL),
caphdr.code, caphdr.length);

/* Length sanity check, type-specific, for known capabilities */
Expand All @@ -829,7 +829,7 @@ bgp_capability_parse (struct peer *peer, size_t length, int *mp_capability,
zlog_info ("%s %s Capability length error: got %u,"
" expected at least %u",
peer->host,
LOOKUP (capcode_str, caphdr.code),
lookup_msg(capcode_str, caphdr.code, NULL),
caphdr.length,
(unsigned) cap_minsizes[caphdr.code]);
bgp_notify_send (peer, BGP_NOTIFY_OPEN_ERR, BGP_NOTIFY_OPEN_MALFORMED_ATTR);
Expand All @@ -841,7 +841,7 @@ bgp_capability_parse (struct peer *peer, size_t length, int *mp_capability,
zlog_info ("%s %s Capability length error: got %u,"
" expected a multiple of %u",
peer->host,
LOOKUP (capcode_str, caphdr.code),
lookup_msg(capcode_str, caphdr.code, NULL),
caphdr.length,
(unsigned) cap_modsizes[caphdr.code]);
bgp_notify_send (peer, BGP_NOTIFY_OPEN_ERR,
Expand Down Expand Up @@ -940,7 +940,7 @@ bgp_capability_parse (struct peer *peer, size_t length, int *mp_capability,
{
if (stream_get_getp(s) > (start + caphdr.length))
zlog_warn ("%s Cap-parser for %s read past cap-length, %u!",
peer->host, LOOKUP (capcode_str, caphdr.code),
peer->host, lookup_msg(capcode_str, caphdr.code, NULL),
caphdr.length);
stream_set_getp (s, start + caphdr.length);
}
Expand Down
6 changes: 3 additions & 3 deletions bgpd/bgp_packet.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1382,7 +1382,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
if (peer->status != Established)
{
zlog_err ("%s [FSM] Update packet received under status %s",
peer->host, LOOKUP (bgp_status_msg, peer->status));
peer->host, lookup_msg(bgp_status_msg, peer->status, NULL));
bgp_notify_send (peer, BGP_NOTIFY_FSM_ERR, 0);
return -1;
}
Expand Down Expand Up @@ -1749,7 +1749,7 @@ bgp_route_refresh_receive (struct peer *peer, bgp_size_t size)
if (peer->status != Established)
{
zlog_err ("%s [Error] Route refresh packet received under status %s",
peer->host, LOOKUP (bgp_status_msg, peer->status));
peer->host, lookup_msg(bgp_status_msg, peer->status, NULL));
bgp_notify_send (peer, BGP_NOTIFY_FSM_ERR, 0);
return;
}
Expand Down Expand Up @@ -2078,7 +2078,7 @@ bgp_capability_receive (struct peer *peer, bgp_size_t size)
if (peer->status != Established)
{
zlog_err ("%s [Error] Dynamic capability packet received under status %s",
peer->host, LOOKUP (bgp_status_msg, peer->status));
peer->host, lookup_msg(bgp_status_msg, peer->status, NULL));
bgp_notify_send (peer, BGP_NOTIFY_FSM_ERR, 0);
return -1;
}
Expand Down
Loading

0 comments on commit 56b4067

Please sign in to comment.