bgpd: Move up flag-check calls, parcel up attr-parser args, and other cleanups

* bgp_attr.h: (struct bgp_attr_parser_args) Attribute parsing context,
  containing common arguments.
* bgp_attr.c: (general) Move the bgp_attr_flag_invalid flag-check calls up,
  out of each individual attr parser function, to be done once in attr_parse.

  Similarly move the calculation of the 'total' attribute length field up
  to attr_parse.

  Bundle together common arguments to attr-parsing functions and helpers
  into (struct bgp_attr_parser_args), so it can be passed by reference down
  the stack & also de-clutter the argument lists & make it easier to
  add/modify the context for attr-parsing - add local const aliases to avoid
  modifying body of code too much. This also should help avoid cut & paste
  errors, where calls to helpers with hard-coded attribute types are pasted
  to other functions but the code isn't changed.

  (bgp_attr_flags_diagnose) as above.
  (bgp_attr_flag_invalid) as above.
  (bgp_attr_{origin,aspath,as4_path,nexthop,med,local_pref,atomic}) as above.
  (bgp_attr_{aggregator,as4_aggregator,community,originator_id}) as above
  (bgp_attr_{cluster_list,ext_communities},bgp_mp_{un,}reach_parse) as above
  (bgp_attr_unknown) as above.

  (bgp_attr_malformed) as above. Also, startp and length have to be
  special-cased, because whether or not to send attribute data depends
  on the particular error - a separate length argument, distinct from
  args->length, indicates whether or not the attribute data should be sent
  in the NOTIFY.
  (bgp_attr_aspath_check) Call to bgp_attr_malformed is wrong here, there is
  no attribute parsing context - e.g. the 'flag' argument is unlikely to be
  right, remove it. Explicitly handle the error instead.
  (bgp_attr_munge_as4_attrs) Flag argument is pointless.

  As the comment notes, the check here is pointless as AS_PATH presence
  already checked elsewhere.

  (bgp_attr_parse) Do bgp_attr_flag_invalid call here.
  Use (struct bgp_attr_parser_args) for args to attr parser functions.
  Remove out-of-context 'flag' argument to as4 checking functions.

2 files changed, 208 insertions, 278 deletions

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index d8ca831a..b02cfee3 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -710,9 +710,17 @@ bgp_attr_flush (struct attr *attr)
  * introduced by the sending neighbour.
  */
 static bgp_attr_parse_ret_t
-bgp_attr_malformed (struct peer *peer, u_char type, u_char flag,
-                    u_char subcode, u_char *startp, bgp_size_t length)
-{
+bgp_attr_malformed (struct bgp_attr_parser_args *args, u_char subcode,
+                    bgp_size_t length)
+{
+  struct peer *const peer = args->peer; 
+  const u_int8_t flags = args->flags;
+  /* startp and length must be special-cased, as whether or not to
+   * send the attribute data with the NOTIFY depends on the error,
+   * the caller therefore signals this with the seperate length argument
+   */
+  u_char *startp = (length > 0 ? args->startp : NULL);
+  
   /* Only relax error handling for eBGP peers */
   if (peer_sort (peer) != BGP_PEER_EBGP)
     {
@@ -722,7 +730,7 @@ bgp_attr_malformed (struct peer *peer, u_char type, u_char flag,
 
     }
   
-  switch (type) {
+  switch (args->type) {
     /* where an optional attribute is inconsequential, e.g. it does not affect
      * route selection, and can be safely ignored then any such attributes
      * which are malformed should just be ignored and the route processed as
@@ -756,9 +764,9 @@ bgp_attr_malformed (struct peer *peer, u_char type, u_char flag,
    * the whole session to be reset. Instead treat it as a withdrawal
    * of the routes, if possible.
    */
-  if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)
-      && CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)
-      && CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL))
+  if (CHECK_FLAG (flags, BGP_ATTR_FLAG_TRANS)
+      && CHECK_FLAG (flags, BGP_ATTR_FLAG_OPTIONAL)
+      && CHECK_FLAG (flags, BGP_ATTR_FLAG_PARTIAL))
     return BGP_ATTR_PARSE_WITHDRAW;
   
   /* default to reset */
@@ -771,16 +779,14 @@ bgp_attr_malformed (struct peer *peer, u_char type, u_char flag,
    being diagnosed is defined by RFC as either a "well-known" or an "optional,
    non-transitive" attribute. */
 static void
-bgp_attr_flags_diagnose
-(
-  struct peer * peer,
-  const u_int8_t attr_code,
-  u_int8_t desired_flags, /* how RFC says it must be */
-  u_int8_t real_flags     /* on wire                 */
+bgp_attr_flags_diagnose (struct bgp_attr_parser_args *args,
+                         u_int8_t desired_flags /* how RFC says it must be */
 )
 {
   u_char seen = 0, i;
-
+  u_char real_flags = args->flags;
+  const u_int8_t attr_code = args->type;
+  
   desired_flags &= ~BGP_ATTR_FLAG_EXTLEN;
   real_flags &= ~BGP_ATTR_FLAG_EXTLEN;
   for (i = 0; i <= 2; i++) /* O,T,P, but not E */
@@ -790,7 +796,7 @@ bgp_attr_flags_diagnose
       CHECK_FLAG (real_flags,    attr_flag_str[i].key)
     )
     {
-      zlog (peer->log, LOG_ERR, "%s attribute must%s be flagged as \"%s\"",
+      zlog (args->peer->log, LOG_ERR, "%s attribute must%s be flagged as \"%s\"",
             LOOKUP (attr_str, attr_code),
             CHECK_FLAG (desired_flags, attr_flag_str[i].key) ? "" : " not",
             attr_flag_str[i].str);
@@ -823,9 +829,12 @@ static const size_t attr_flags_values_max =
   sizeof (attr_flags_values) / sizeof (attr_flags_values[0]);
 
 static int
-bgp_attr_flag_invalid (struct peer *peer, u_int8_t attr_code, u_int8_t flags)
+bgp_attr_flag_invalid (struct bgp_attr_parser_args *args)
 {
   u_int8_t mask = BGP_ATTR_FLAG_EXTLEN;
+  const u_int8_t flags = args->flags;
+  const u_int8_t attr_code = args->type;
+  struct peer *const peer = args->peer; 
   
   /* there may be attributes we don't know about */
   if (attr_code > attr_flags_values_max)
@@ -880,31 +889,18 @@ bgp_attr_flag_invalid (struct peer *peer, u_int8_t attr_code, u_int8_t flags)
       == attr_flags_values[attr_code])
     return 0;
   
-  bgp_attr_flags_diagnose (peer, attr_code, attr_flags_values[attr_code],
-                           flags);
+  bgp_attr_flags_diagnose (args, attr_flags_values[attr_code]);
   return 1;
 }
 
 /* Get origin attribute of the update message. */
 static bgp_attr_parse_ret_t
-bgp_attr_origin (struct peer *peer, bgp_size_t length, 
-		 struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_origin (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total;
-
-  /* total is entire attribute length include Attribute Flags (1),
-     Attribute Type code (1) and Attribute length (1 or 2).  */
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-
-  /* If any recognized attribute has Attribute Flags that conflict
-     with the Attribute Type Code, then the Error Subcode is set to
-     Attribute Flags Error.  The Data field contains the erroneous
-     attribute (type, length and value). */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_ORIGIN, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
-
+  struct peer *const peer = args->peer;
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
+  
   /* If any recognized attribute has Attribute Length that conflicts
      with the expected length (based on the attribute type code), then
      the Error Subcode is set to Attribute Length Error.  The Data
@@ -914,9 +910,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length,
     {
       zlog (peer->log, LOG_ERR, "Origin attribute length is not one %d",
 	    length);
-      return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag,
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                                 startp, total);
+                                 args->total);
     }
 
   /* Fetch origin attribute. */
@@ -931,9 +927,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length,
     {
       zlog (peer->log, LOG_ERR, "Origin attribute value is invalid %d",
 	      attr->origin);
-      return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag,
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_INVAL_ORIGIN,
-                                 startp, total);
+                                 args->total);
     }
 
   /* Set oring attribute flag. */
@@ -945,17 +941,12 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length,
 /* Parse AS path information.  This function is wrapper of
    aspath_parse. */
 static int
-bgp_attr_aspath (struct peer *peer, bgp_size_t length, 
-		 struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_aspath (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
+  struct attr *const attr = args->attr;
+  struct peer *const peer = args->peer; 
+  const bgp_size_t length = args->length;
   
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS_PATH, flag))
-      return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag,
-				 BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-				 startp, total);
   /*
    * peer with AS4 => will get 4Byte ASnums
    * otherwise, will get 16 Bit
@@ -969,9 +960,7 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length,
       zlog (peer->log, LOG_ERR,
             "Malformed AS path from %s, length is %d",
             peer->host, length);
-      return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag,
-                                 BGP_NOTIFY_UPDATE_MAL_AS_PATH,
-                                 NULL, 0);
+      return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_MAL_AS_PATH, 0);
     }
 
   /* Set aspath attribute flag. */
@@ -981,7 +970,7 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length,
 }
 
 static bgp_attr_parse_ret_t
-bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag)
+bgp_attr_aspath_check (struct peer *const peer, struct attr *const attr)
 {
   /* These checks were part of bgp_attr_aspath, but with
    * as4 we should to check aspath things when
@@ -1000,9 +989,9 @@ bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag)
      (peer_sort (peer) == BGP_PEER_EBGP && aspath_confed_check (attr->aspath)))
     {
       zlog (peer->log, LOG_ERR, "Malformed AS path from %s", peer->host);
-      return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag,
-                                 BGP_NOTIFY_UPDATE_MAL_AS_PATH,
-                                 NULL, 0);
+      bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
+                       BGP_NOTIFY_UPDATE_MAL_AS_PATH);
+      return BGP_ATTR_PARSE_ERROR;
     }
 
   /* First AS check for EBGP. */
@@ -1013,9 +1002,9 @@ bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag)
  	{
  	  zlog (peer->log, LOG_ERR,
  		"%s incorrect first AS (must be %u)", peer->host, peer->as);
-          return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag,
-                                     BGP_NOTIFY_UPDATE_MAL_AS_PATH,
-                                     NULL, 0);
+          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
+                           BGP_NOTIFY_UPDATE_MAL_AS_PATH);
+          return BGP_ATTR_PARSE_ERROR;
  	}
     }
 
@@ -1035,19 +1024,11 @@ bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag)
 /* Parse AS4 path information.  This function is another wrapper of
    aspath_parse. */
 static int
-bgp_attr_as4_path (struct peer *peer, bgp_size_t length,
-		 struct attr *attr, u_char flag, u_char *startp,
-		 struct aspath **as4_path)
+bgp_attr_as4_path (struct bgp_attr_parser_args *args, struct aspath **as4_path)
 {
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-
-  /* Flag check. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS4_PATH, flag))
-      return bgp_attr_malformed (peer, BGP_ATTR_AS4_PATH, flag,
-                                 BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                                 startp, total);
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
   
   *as4_path = aspath_parse (peer->ibuf, length, 1);
 
@@ -1057,9 +1038,9 @@ bgp_attr_as4_path (struct peer *peer, bgp_size_t length,
       zlog (peer->log, LOG_ERR,
             "Malformed AS4 path from %s, length is %d",
             peer->host, length);
-      return bgp_attr_malformed (peer, BGP_ATTR_AS4_PATH, flag,
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_MAL_AS_PATH,
-                                 NULL, 0);
+                                 0);
     }
 
   /* Set aspath attribute flag. */
@@ -1071,29 +1052,23 @@ bgp_attr_as4_path (struct peer *peer, bgp_size_t length,
 
 /* Nexthop attribute. */
 static bgp_attr_parse_ret_t
-bgp_attr_nexthop (struct peer *peer, bgp_size_t length, 
-		  struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_nexthop (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total;
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
+  
   in_addr_t nexthop_h, nexthop_n;
 
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-
-  /* Flags check. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_NEXT_HOP, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
-
   /* Check nexthop attribute length. */
   if (length != 4)
     {
       zlog (peer->log, LOG_ERR, "Nexthop attribute length isn't four [%d]",
 	      length);
 
-      return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag,
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                                 startp, total);
+                                 args->total);
     }
 
   /* According to section 6.3 of RFC4271, syntactically incorrect NEXT_HOP
@@ -1108,9 +1083,9 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length,
       char buf[INET_ADDRSTRLEN];
       inet_ntop (AF_INET, &nexthop_h, buf, INET_ADDRSTRLEN);
       zlog (peer->log, LOG_ERR, "Martian nexthop %s", buf);
-      return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag,
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_INVAL_NEXT_HOP,
-                                 startp, total);
+                                 args->total);
     }
 
   attr->nexthop.s_addr = nexthop_n;
@@ -1121,28 +1096,21 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length,
 
 /* MED atrribute. */
 static bgp_attr_parse_ret_t
-bgp_attr_med (struct peer *peer, bgp_size_t length, 
-	      struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_med (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-
-  /* Flag checks. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_MULTI_EXIT_DISC, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
-
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
+  
   /* Length check. */
   if (length != 4)
     {
       zlog (peer->log, LOG_ERR, 
 	    "MED attribute length isn't four [%d]", length);
 
-      return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag,
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                                 startp, total);
+                                 args->total);
     }
 
   attr->med = stream_getl (peer->ibuf);
@@ -1154,25 +1122,20 @@ bgp_attr_med (struct peer *peer, bgp_size_t length,
 
 /* Local preference attribute. */
 static bgp_attr_parse_ret_t
-bgp_attr_local_pref (struct peer *peer, bgp_size_t length, 
-		     struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_local_pref (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-  /* Flag checks. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_LOCAL_PREF, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_LOCAL_PREF, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
   
   /* Length check. */
   if (length != 4)
   {
-    zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute length isn't 4 [%u]", length);
-    return bgp_attr_malformed (peer, BGP_ATTR_LOCAL_PREF, flag,
+    zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute length isn't 4 [%u]",
+          length);
+    return bgp_attr_malformed (args,
                                BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                               startp, total);
+                               args->total);
   }
 
   /* If it is contained in an UPDATE message that is received from an
@@ -1194,25 +1157,20 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length,
 
 /* Atomic aggregate. */
 static int
-bgp_attr_atomic (struct peer *peer, bgp_size_t length, 
-		 struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_atomic (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-  /* Flag checks. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
   
   /* Length check. */
   if (length != 0)
     {
-      zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute length isn't 0 [%u]", length);
-      return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag,
+      zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute length isn't 0 [%u]",
+            length);
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                                 startp, total);
+                                 args->total);
     }
 
   /* Set atomic aggregate flag. */
@@ -1223,19 +1181,14 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length,
 
 /* Aggregator attribute */
 static int
-bgp_attr_aggregator (struct peer *peer, bgp_size_t length,
-		     struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_aggregator (struct bgp_attr_parser_args *args)
 {
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
+  
   int wantedlen = 6;
   struct attr_extra *attre = bgp_attr_extra_get (attr);
-  bgp_size_t total;
-  
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-  /* Flags check. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_AGGREGATOR, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
   
   /* peer with AS4 will send 4 Byte AS, peer without will send 2 Byte */
   if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV))
@@ -1243,10 +1196,11 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length,
   
   if (length != wantedlen)
     {
-      zlog (peer->log, LOG_ERR, "AGGREGATOR attribute length isn't %u [%u]", wantedlen, length);
-      return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag,
+      zlog (peer->log, LOG_ERR, "AGGREGATOR attribute length isn't %u [%u]",
+            wantedlen, length);
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                                 startp, total);
+                                 args->total);
     }
   
   if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV ) )
@@ -1263,27 +1217,22 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length,
 
 /* New Aggregator attribute */
 static bgp_attr_parse_ret_t
-bgp_attr_as4_aggregator (struct peer *peer, bgp_size_t length,
-		     struct attr *attr, u_char flag, 
-		     as_t *as4_aggregator_as,
-		     struct in_addr *as4_aggregator_addr,
-		     u_char *startp)
+bgp_attr_as4_aggregator (struct bgp_attr_parser_args *args,
+		         as_t *as4_aggregator_as,
+		         struct in_addr *as4_aggregator_addr)
 {
-  bgp_size_t total;
-  
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
+      
   if (length != 8)
     {
-      zlog (peer->log, LOG_ERR, "New Aggregator length is not 8 [%d]", length);
-      return bgp_attr_malformed (peer, BGP_ATTR_AS4_AGGREGATOR, flag,
+      zlog (peer->log, LOG_ERR, "New Aggregator length is not 8 [%d]",
+            length);
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                                 NULL, 0);
+                                 0);
     }
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-  /* Flags check. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS4_AGGREGATOR, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_AS4_AGGREGATOR, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
   
   *as4_aggregator_as = stream_getl (peer->ibuf);
   as4_aggregator_addr->s_addr = stream_get_ipv4 (peer->ibuf);
@@ -1296,7 +1245,8 @@ bgp_attr_as4_aggregator (struct peer *peer, bgp_size_t length,
 /* Munge Aggregator and New-Aggregator, AS_PATH and NEW_AS_PATH.
  */
 static bgp_attr_parse_ret_t
-bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, u_char flag,
+bgp_attr_munge_as4_attrs (struct peer *const peer,
+                          struct attr *const attr,
                           struct aspath *as4_path, as_t as4_aggregator,
                           struct in_addr *as4_aggregator_addr)
 {
@@ -1325,24 +1275,6 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, u_char flag,
       return BGP_ATTR_PARSE_PROCEED;
     }
   
-  if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH))
-      && !(attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS_PATH))))
-    {
-      /* Hu? This is not supposed to happen at all!
-       * got as4_path and no aspath,
-       *   This should already
-       *   have been handled by 'well known attributes missing'
-       *   But... yeah, paranoia
-       * Take this as a "malformed attribute"
-       */
-      zlog (peer->log, LOG_ERR, 
-            "%s BGP not AS4 capable peer sent AS4_PATH but"
-            " no AS_PATH, cant do anything here", peer->host);
-      return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag,
-                                 BGP_NOTIFY_UPDATE_MAL_ATTR,
-                                 NULL, 0);
-    }
-
   /* We have a asn16 peer.  First, look for AS4_AGGREGATOR
    * because that may override AS4_PATH
    */
@@ -1410,11 +1342,11 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, u_char flag,
 
 /* Community attribute. */
 static bgp_attr_parse_ret_t
-bgp_attr_community (struct peer *peer, bgp_size_t length, 
-		    struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_community (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total
-    = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;  
+  const bgp_size_t length = args->length;
   
   if (length == 0)
     {
@@ -1422,12 +1354,6 @@ bgp_attr_community (struct peer *peer, bgp_size_t length,
       return BGP_ATTR_PARSE_PROCEED;
     }
   
-  /* Flags check. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_COMMUNITIES, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_COMMUNITIES, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
-  
   attr->community =
     community_parse ((u_int32_t *)stream_pnt (peer->ibuf), length);
   
@@ -1435,9 +1361,9 @@ bgp_attr_community (struct peer *peer, bgp_size_t length,
   stream_forward_getp (peer->ibuf, length);
 
   if (!attr->community)
-    return bgp_attr_malformed (peer, BGP_ATTR_COMMUNITIES, flag,
+    return bgp_attr_malformed (args,
                                BGP_NOTIFY_UPDATE_OPT_ATTR_ERR,
-                               startp, total);
+                               args->total);
   
   attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_COMMUNITIES);
 
@@ -1446,25 +1372,20 @@ bgp_attr_community (struct peer *peer, bgp_size_t length,
 
 /* Originator ID attribute. */
 static bgp_attr_parse_ret_t
-bgp_attr_originator_id (struct peer *peer, bgp_size_t length, 
-			struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_originator_id (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-  /* Flag checks. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_ORIGINATOR_ID, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
+  
   /* Length check. */
   if (length != 4)
     {
       zlog (peer->log, LOG_ERR, "Bad originator ID length %d", length);
 
-      return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag,
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                                 startp, total);
+                                 args->total);
     }
 
   (bgp_attr_extra_get (attr))->originator_id.s_addr 
@@ -1477,25 +1398,19 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length,
 
 /* Cluster list attribute. */
 static bgp_attr_parse_ret_t
-bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, 
-		       struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_cluster_list (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-  /* Flag checks. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_CLUSTER_LIST, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
+  
   /* Check length. */
   if (length % 4)
     {
       zlog (peer->log, LOG_ERR, "Bad cluster list length %d", length);
 
-      return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag,
-                                 BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
-                                 startp, total);
+      return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
+                                 args->total);
     }
 
   (bgp_attr_extra_get (attr))->cluster 
@@ -1511,8 +1426,8 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length,
 
 /* Multiprotocol reachability information parse. */
 int
-bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length,
-                    struct attr *attr, const u_char flag, u_char *startp, struct bgp_nlri *mp_update)
+bgp_mp_reach_parse (struct bgp_attr_parser_args *args,
+                    struct bgp_nlri *mp_update)
 {
   afi_t afi;
   safi_t safi;
@@ -1520,15 +1435,11 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length,
   size_t start;
   int ret;
   struct stream *s;
+  struct peer *const peer = args->peer;  
+  struct attr *const attr = args->attr;
+  const bgp_size_t length = args->length;
   struct attr_extra *attre = bgp_attr_extra_get(attr);
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-  /* Flag checks. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_MP_REACH_NLRI, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_MP_REACH_NLRI, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
+  
   /* Set end of packet. */
   s = BGP_INPUT(peer);
   start = stream_get_getp(s);
@@ -1647,8 +1558,7 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length,
 
 /* Multiprotocol unreachable parse */
 int
-bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length,
-                      const u_char flag, u_char *startp,
+bgp_mp_unreach_parse (struct bgp_attr_parser_args *args,
 		      struct bgp_nlri *mp_withdraw)
 {
   struct stream *s;
@@ -1656,14 +1566,8 @@ bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length,
   safi_t safi;
   u_int16_t withdraw_len;
   int ret;
-  bgp_size_t total;
-
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-  /* Flag checks. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_MP_UNREACH_NLRI, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_MP_UNREACH_NLRI, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
+  struct peer *const peer = args->peer;  
+  const bgp_size_t length = args->length;
 
   s = peer->ibuf;
   
@@ -1695,11 +1599,11 @@ bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length,
 
 /* Extended Community attribute. */
 static bgp_attr_parse_ret_t
-bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, 
-			  struct attr *attr, u_char flag, u_char *startp)
+bgp_attr_ext_communities (struct bgp_attr_parser_args *args)
 {
-  bgp_size_t total
-    = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
+  struct peer *const peer = args->peer;  
+  struct attr *const attr = args->attr;  
+  const bgp_size_t length = args->length;
   
   if (length == 0)
     {
@@ -1709,21 +1613,15 @@ bgp_attr_ext_communities (struct peer *peer, bgp_size_t length,
       return BGP_ATTR_PARSE_PROCEED;
     }
 
-  /* Flags check. */
-  if (bgp_attr_flag_invalid (peer, BGP_ATTR_EXT_COMMUNITIES, flag))
-    return bgp_attr_malformed (peer, BGP_ATTR_EXT_COMMUNITIES, flag,
-                               BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
-                               startp, total);
-  
   (bgp_attr_extra_get (attr))->ecommunity =
     ecommunity_parse ((u_int8_t *)stream_pnt (peer->ibuf), length);
   /* XXX: fix ecommunity_parse to use stream API */
   stream_forward_getp (peer->ibuf, length);
   
   if (!attr->extra->ecommunity)
-    return bgp_attr_malformed (peer, BGP_ATTR_EXT_COMMUNITIES,
-                               flag, BGP_NOTIFY_UPDATE_OPT_ATTR_ERR,
-                               startp, total);
+    return bgp_attr_malformed (args,
+                               BGP_NOTIFY_UPDATE_OPT_ATTR_ERR,
+                               args->total);
   
   attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES);
 
@@ -1732,12 +1630,18 @@ bgp_attr_ext_communities (struct peer *peer, bgp_size_t length,
 
 /* BGP unknown attribute treatment. */
 static bgp_attr_parse_ret_t
-bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag,
-		  u_char type, bgp_size_t length, u_char *startp)
+bgp_attr_unknown (struct bgp_attr_parser_args *args)
 {
   bgp_size_t total;
   struct transit *transit;
   struct attr_extra *attre;
+  struct peer *const peer = args->peer; 
+  struct attr *const attr = args->attr;
+  u_char *const startp = args->startp;
+  const u_char type = args->type;
+  const u_char flag = args->flags;  
+  const bgp_size_t length = args->length;
+  
 
   if (BGP_DEBUG (normal, NORMAL))
   zlog_debug ("%s Unknown attribute is received (type %d, length %d)",
@@ -1750,18 +1654,15 @@ bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag,
   /* Forward read pointer of input stream. */
   stream_forward_getp (peer->ibuf, length);
 
-  /* Adjest total length to include type and length. */
-  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-
   /* If any of the mandatory well-known attributes are not recognized,
      then the Error Subcode is set to Unrecognized Well-known
      Attribute.  The Data field contains the unrecognized attribute
      (type, length and value). */
   if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL))
     {
-      return bgp_attr_malformed (peer, type, flag,
+      return bgp_attr_malformed (args,
                                  BGP_NOTIFY_UPDATE_UNREC_ATTR,
-                                 startp, total);
+                                 args->total);
     }
 
   /* Unrecognized non-transitive optional attributes must be quietly
@@ -1858,7 +1759,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
 			   BGP_NOTIFY_UPDATE_ATTR_LENG_ERR);
 	  return BGP_ATTR_PARSE_ERROR;
 	}
-
+      
       /* Check extended attribue length bit. */
       if (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN))
 	length = stream_getw (BGP_INPUT (peer));
@@ -1898,59 +1799,79 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
 			   BGP_NOTIFY_UPDATE_ATTR_LENG_ERR);
 	  return BGP_ATTR_PARSE_ERROR;
 	}
+	
+        struct bgp_attr_parser_args attr_args = {
+          .peer = peer,
+          .length = length,
+          .attr = attr,
+          .type = type,
+          .flags = flag,
+          .startp = startp,
+          .total = attr_endp - startp,
+        };
+      
+	
+      /* If any recognized attribute has Attribute Flags that conflict
+         with the Attribute Type Code, then the Error Subcode is set to
+         Attribute Flags Error.  The Data field contains the erroneous
+         attribute (type, length and value). */
+      if (bgp_attr_flag_invalid (&attr_args))
+        return bgp_attr_malformed (&attr_args,
+                                   BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
+                                   attr_args.total);
 
       /* OK check attribute and store it's value. */
       switch (type)
 	{
 	case BGP_ATTR_ORIGIN:
-	  ret = bgp_attr_origin (peer, length, attr, flag, startp);
+	  ret = bgp_attr_origin (&attr_args);
 	  break;
 	case BGP_ATTR_AS_PATH:
-	  ret = bgp_attr_aspath (peer, length, attr, flag, startp);
+	  ret = bgp_attr_aspath (&attr_args);
 	  break;
 	case BGP_ATTR_AS4_PATH:
-	  ret = bgp_attr_as4_path (peer, length, attr, flag, startp, &as4_path);
+	  ret = bgp_attr_as4_path (&attr_args, &as4_path);
 	  break;
 	case BGP_ATTR_NEXT_HOP:	
-	  ret = bgp_attr_nexthop (peer, length, attr, flag, startp);
+	  ret = bgp_attr_nexthop (&attr_args);
 	  break;
 	case BGP_ATTR_MULTI_EXIT_DISC:
-	  ret = bgp_attr_med (peer, length, attr, flag, startp);
+	  ret = bgp_attr_med (&attr_args);
 	  break;
 	case BGP_ATTR_LOCAL_PREF:
-	  ret = bgp_attr_local_pref (peer, length, attr, flag, startp);
+	  ret = bgp_attr_local_pref (&attr_args);
 	  break;
 	case BGP_ATTR_ATOMIC_AGGREGATE:
-	  ret = bgp_attr_atomic (peer, length, attr, flag, startp);
+	  ret = bgp_attr_atomic (&attr_args);
 	  break;
 	case BGP_ATTR_AGGREGATOR:
-	  ret = bgp_attr_aggregator (peer, length, attr, flag, startp);
+	  ret = bgp_attr_aggregator (&attr_args);
 	  break;
 	case BGP_ATTR_AS4_AGGREGATOR:
-	  ret = bgp_attr_as4_aggregator (peer, length, attr, flag,
-	                                 &as4_aggregator, 
-	                                 &as4_aggregator_addr, startp);
+	  ret = bgp_attr_as4_aggregator (&attr_args,
+	                                 &as4_aggregator,
+	                                 &as4_aggregator_addr);
 	  break;
 	case BGP_ATTR_COMMUNITIES:
-	  ret = bgp_attr_community (peer, length, attr, flag, startp);
+	  ret = bgp_attr_community (&attr_args);
 	  break;
 	case BGP_ATTR_ORIGINATOR_ID:
-	  ret = bgp_attr_originator_id (peer, length, attr, flag, startp);
+	  ret = bgp_attr_originator_id (&attr_args);
 	  break;
 	case BGP_ATTR_CLUSTER_LIST:
-	  ret = bgp_attr_cluster_list (peer, length, attr, flag, startp);
+	  ret = bgp_attr_cluster_list (&attr_args);
 	  break;
 	case BGP_ATTR_MP_REACH_NLRI:
-	  ret = bgp_mp_reach_parse (peer, length, attr, flag, startp, mp_update);
+	  ret = bgp_mp_reach_parse (&attr_args, mp_update);
 	  break;
 	case BGP_ATTR_MP_UNREACH_NLRI:
-	  ret = bgp_mp_unreach_parse (peer, length, flag, startp, mp_withdraw);
+	  ret = bgp_mp_unreach_parse (&attr_args, mp_withdraw);
 	  break;
 	case BGP_ATTR_EXT_COMMUNITIES:
-	  ret = bgp_attr_ext_communities (peer, length, attr, flag, startp);
+	  ret = bgp_attr_ext_communities (&attr_args);
 	  break;
 	default:
-	  ret = bgp_attr_unknown (peer, attr, flag, type, length, startp);
+	  ret = bgp_attr_unknown (&attr_args);
 	  break;
 	}
       
@@ -2020,7 +1941,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
    * all attributes first, including these 32bit ones, and now,
    * afterwards, we look what and if something is to be done for as4.
    */
-  if (bgp_attr_munge_as4_attrs (peer, attr, flag, as4_path,
+  if (bgp_attr_munge_as4_attrs (peer, attr, as4_path,
                                 as4_aggregator, &as4_aggregator_addr))
     {
       if (as4_path)
@@ -2052,7 +1973,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
    */
   if (attr->flag & (ATTR_FLAG_BIT(BGP_ATTR_AS_PATH)))
     {
-      ret = bgp_attr_aspath_check (peer, attr, flag);
+      ret = bgp_attr_aspath_check (peer, attr);
       if (ret != BGP_ATTR_PARSE_PROCEED)
 	return ret;
     }
diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index e6300740..df87c863 100644
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -179,10 +179,19 @@ extern void cluster_unintern (struct cluster_list *);
 /* Transit attribute prototypes. */
 void transit_unintern (struct transit *);
 
-/* Exported for unit-test purposes only */
-extern int bgp_mp_reach_parse (struct peer *, const bgp_size_t, struct attr *,
-			       const u_char, u_char *, struct bgp_nlri *);
-extern int bgp_mp_unreach_parse (struct peer *, const bgp_size_t, const u_char,
-                                 u_char *, struct bgp_nlri *);
+/* Below exported for unit-test purposes only */
+struct bgp_attr_parser_args {
+  struct peer *peer;
+  bgp_size_t length; /* attribute data length; */
+  bgp_size_t total; /* total length, inc header */
+  struct attr *attr;
+  u_int8_t type;
+  u_int8_t flags;
+  u_char *startp;   
+};
+extern int bgp_mp_reach_parse (struct bgp_attr_parser_args *args, 
+			       struct bgp_nlri *);
+extern int bgp_mp_unreach_parse (struct bgp_attr_parser_args *args,
+                                 struct bgp_nlri *);
 
 #endif /* _QUAGGA_BGP_ATTR_H */