Shouldn't the BGP_NOTIFY_OPEN_ERR sub codes be an enum? This way the
compiler can help make sure we don't mess up passing in a wrong value.
Probably would be ok to do for a future patch as well. I'm not going to
hold this change up for this :)
donald
On Wed, Nov 25, 2015 at 12:14 PM, Paul Jakma <paul.ja...@hpe.com> wrote:
> CEASE NOTIFICATION for OPEN parsing errors seems, to my reading of RFC4271
> ยง6.2 to be incorrect.
>
> * bgp_packet.c: (bgp_open_receive) OPEN/UNACEP_HOLDTIME is not an
> appropriate error subcode if bgp_open_option_parse returns an error. Set
> it to "Unspecific". Where a more specific subcode is appropriate, then
> lower
> level should send that.
> * bgp_open.c: (bgp_open_option_parse) Malformed OPENs should result in
> NOTIFICATION with OPEN error, and OPEN/UNSPECIFIC sub-code - not CEASE.
> (bgp_capability_{parse,orf_entry}) ditto.
> * bgpd.h: Add BGP_NOTIFY_OPEN_UNSPECIFIC for 0. Note that IANA lists 0 as
> reserved in the OPEN error sub-code registry, but RFC4271 page 32 says 0
> is the "Unspecific" OPEN error subcode.
>
> Have emailed IANA, they says it's a known errate to 4271 under review.
>
> Some inspiration from Cumulus' bgpd-capability-cleanup.patch, though
> v different result.
> ---
> bgpd/bgp_open.c | 15 +++++++++------
> bgpd/bgp_packet.c | 2 +-
> bgpd/bgpd.h | 1 +
> 3 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c
> index b9d6e93..4709871 100644
> --- a/bgpd/bgp_open.c
> +++ b/bgpd/bgp_open.c
> @@ -235,7 +235,7 @@ bgp_capability_orf_entry (struct peer *peer, struct
> capability_header *hdr)
> zlog_info ("%s ORF Capability entry length error,"
> " Cap length %u, num %u",
> peer->host, hdr->length, entry.num);
> - bgp_notify_send (peer, BGP_NOTIFY_CEASE, 0);
> + bgp_notify_send (peer, BGP_NOTIFY_OPEN_ERR,
> BGP_NOTIFY_OPEN_UNSPECIFIC);
> return -1;
> }
>
> @@ -469,7 +469,7 @@ bgp_capability_parse (struct peer *peer, size_t
> length, int *mp_capability,
> if (stream_get_getp(s) + 2 > end)
> {
> zlog_info ("%s Capability length error (< header)", peer->host);
> - bgp_notify_send (peer, BGP_NOTIFY_CEASE, 0);
> + bgp_notify_send (peer, BGP_NOTIFY_OPEN_ERR,
> BGP_NOTIFY_OPEN_UNSPECIFIC);
> return -1;
> }
>
> @@ -481,7 +481,7 @@ bgp_capability_parse (struct peer *peer, size_t
> length, int *mp_capability,
> if (start + caphdr.length > end)
> {
> zlog_info ("%s Capability length error (< length)", peer->host);
> - bgp_notify_send (peer, BGP_NOTIFY_CEASE, 0);
> + bgp_notify_send (peer, BGP_NOTIFY_OPEN_ERR,
> BGP_NOTIFY_OPEN_UNSPECIFIC);
> return -1;
> }
>
> @@ -511,7 +511,8 @@ bgp_capability_parse (struct peer *peer, size_t
> length, int *mp_capability,
> LOOKUP (capcode_str, caphdr.code),
> caphdr.length,
> (unsigned) cap_minsizes[caphdr.code]);
> - bgp_notify_send (peer, BGP_NOTIFY_CEASE, 0);
> + bgp_notify_send (peer, BGP_NOTIFY_OPEN_ERR,
> + BGP_NOTIFY_OPEN_UNSPECIFIC);
> return -1;
> }
> /* we deliberately ignore unknown codes, see below */
> @@ -727,7 +728,8 @@ bgp_open_option_parse (struct peer *peer, u_char
> length, int *mp_capability)
> if (STREAM_READABLE(s) < 2)
> {
> zlog_info ("%s Option length error", peer->host);
> - bgp_notify_send (peer, BGP_NOTIFY_CEASE, 0);
> + bgp_notify_send (peer, BGP_NOTIFY_OPEN_ERR,
> + BGP_NOTIFY_OPEN_UNSPECIFIC);
> return -1;
> }
>
> @@ -739,7 +741,8 @@ bgp_open_option_parse (struct peer *peer, u_char
> length, int *mp_capability)
> if (STREAM_READABLE (s) < opt_length)
> {
> zlog_info ("%s Option length error", peer->host);
> - bgp_notify_send (peer, BGP_NOTIFY_CEASE, 0);
> + bgp_notify_send (peer, BGP_NOTIFY_OPEN_ERR,
> + BGP_NOTIFY_OPEN_UNSPECIFIC);
> return -1;
> }
>
> diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
> index e1ae494..032808f 100644
> --- a/bgpd/bgp_packet.c
> +++ b/bgpd/bgp_packet.c
> @@ -1540,7 +1540,7 @@ bgp_open_receive (struct peer *peer, bgp_size_t size)
> {
> bgp_notify_send (peer,
> BGP_NOTIFY_OPEN_ERR,
> - BGP_NOTIFY_OPEN_UNACEP_HOLDTIME);
> + BGP_NOTIFY_OPEN_UNSPECIFIC);
> return ret;
> }
> }
> diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
> index 95c16de..51501f1 100644
> --- a/bgpd/bgpd.h
> +++ b/bgpd/bgpd.h
> @@ -674,6 +674,7 @@ struct bgp_nlri
> #define BGP_NOTIFY_HEADER_MAX 4
>
> /* BGP_NOTIFY_OPEN_ERR sub codes. */
> +#define BGP_NOTIFY_OPEN_UNSPECIFIC 0
> #define BGP_NOTIFY_OPEN_UNSUP_VERSION 1
> #define BGP_NOTIFY_OPEN_BAD_PEER_AS 2
> #define BGP_NOTIFY_OPEN_BAD_BGP_IDENT 3
> --
> 2.5.0
>
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev@lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev
