Nest: Update handling of temporary attributes

The temporary atttributes are no longer removed by ea_do_prune(), but
they are undefined by store_tmp_attrs() protocol hooks. This fixes
several bugs where temporary attributes were removed when they should
not or not removed when they should be. The flag EAF_TEMP is no longer
needed and was removed.

Update all protocol make_tmp_attrs() / store_tmp_attrs() hooks to use
helper functions and to handle unset attributes properly.

Also fix some related bugs like improper handling of empty eattr list.

5 files changed, 182 insertions, 57 deletions

diff --git a/nest/protocol.h b/nest/protocol.h
index 6c04071b..56d66ed5 100644
--- a/nest/protocol.h
+++ b/nest/protocol.h
@@ -194,8 +194,8 @@ struct proto {
    *	   ifa_notify	Notify protocol about interface address changes.
    *	   rt_notify	Notify protocol about routing table updates.
    *	   neigh_notify	Notify protocol about neighbor cache events.
-   *	   make_tmp_attrs  Construct ea_list from private attrs stored in rta.
-   *	   store_tmp_attrs Store private attrs back to rta. The route MUST NOT be cached.
+   *	   make_tmp_attrs  Add attributes to rta from from private attrs stored in rte. The route and rta MUST NOT be cached.
+   *	   store_tmp_attrs Store private attrs back to rte and undef added attributes. The route and rta MUST NOT be cached.
    *	   preexport  Called as the first step of the route exporting process.
    *			It can construct a new rte, add private attributes and
    *			decide whether the route shall be exported: 1=yes, -1=no,
@@ -211,8 +211,8 @@ struct proto {
   void (*ifa_notify)(struct proto *, unsigned flags, struct ifa *a);
   void (*rt_notify)(struct proto *, struct channel *, struct network *net, struct rte *new, struct rte *old);
   void (*neigh_notify)(struct neighbor *neigh);
-  struct ea_list *(*make_tmp_attrs)(struct rte *rt, struct linpool *pool);
-  void (*store_tmp_attrs)(struct rte *rt);
+  void (*make_tmp_attrs)(struct rte *rt, struct linpool *pool);
+  void (*store_tmp_attrs)(struct rte *rt, struct linpool *pool);
   int (*preexport)(struct proto *, struct rte **rt, struct linpool *pool);
   void (*reload_routes)(struct channel *);
   void (*feed_begin)(struct channel *, int initial);
@@ -297,18 +297,6 @@ proto_get_router_id(struct proto_config *pc)
   return pc->router_id ? pc->router_id : pc->global->router_id;
 }
 
-static inline void
-rte_make_tmp_attrs(struct rte **rt, struct linpool *pool)
-{
-  struct ea_list *(*mta)(struct rte *rt, struct linpool *pool);
-  mta = (*rt)->attrs->src->proto->make_tmp_attrs;
-  if (!mta) return;
-  *rt = rte_cow_rta(*rt, pool);
-  struct ea_list *ea = mta(*rt, pool);
-  ea->next = (*rt)->attrs->eattrs;
-  (*rt)->attrs->eattrs = ea;
-}
-
 
 extern pool *proto_pool;
 extern list proto_list;
diff --git a/nest/route.h b/nest/route.h
index 3e835bb1..15aac8f6 100644
--- a/nest/route.h
+++ b/nest/route.h
@@ -308,8 +308,10 @@ void rte_free(rte *);
 rte *rte_do_cow(rte *);
 static inline rte * rte_cow(rte *r) { return (r->flags & REF_COW) ? rte_do_cow(r) : r; }
 rte *rte_cow_rta(rte *r, linpool *lp);
-void rte_make_tmp_attr(struct rte *r, struct ea_list *e, uint id, uint type, u32 val);
-uint rte_store_tmp_attr(struct rte *r, uint id);
+void rte_init_tmp_attrs(struct rte *r, linpool *lp, uint max);
+void rte_make_tmp_attr(struct rte *r, uint id, uint type, uintptr_t val);
+void rte_make_tmp_attrs(struct rte **r, struct linpool *pool, struct rta **old_attrs);
+uintptr_t rte_store_tmp_attr(struct rte *r, uint id);
 void rt_dump(rtable *);
 void rt_dump_all(void);
 int rt_feed_channel(struct channel *c);
@@ -512,7 +514,6 @@ const char *ea_custom_name(uint ea);
 #define EAF_VAR_LENGTH 0x02		/* Attribute length is variable (part of type spec) */
 #define EAF_ORIGINATED 0x20		/* The attribute has originated locally */
 #define EAF_FRESH 0x40			/* An uncached attribute (e.g. modified in export filter) */
-#define EAF_TEMP 0x80			/* A temporary attribute (the one stored in the tmp attr list) */
 
 typedef struct adata {
   uint length;				/* Length of data */
@@ -542,6 +543,7 @@ typedef struct ea_list {
 #define EALF_SORTED 1			/* Attributes are sorted by code */
 #define EALF_BISECT 2			/* Use interval bisection for searching */
 #define EALF_CACHED 4			/* Attributes belonging to cached rta */
+#define EALF_TEMP 8			/* Temporary ea_list added by make_tmp_attrs hooks */
 
 struct rte_src *rt_find_source(struct proto *p, u32 id);
 struct rte_src *rt_get_source(struct proto *p, u32 id);
@@ -574,6 +576,8 @@ void ea_format_bitfield(struct eattr *a, byte *buf, int bufsize, const char **na
     ea = t; \
   } \
   ea_sort(ea); \
+  if (ea->count == 0) \
+    ea = NULL; \
 } while(0) \
 
 static inline eattr *
diff --git a/nest/rt-attr.c b/nest/rt-attr.c
index 3e4578b0..cc362cab 100644
--- a/nest/rt-attr.c
+++ b/nest/rt-attr.c
@@ -571,8 +571,8 @@ ea_do_sort(ea_list *e)
 }
 
 /**
- * In place discard duplicates, undefs and temporary attributes in sorted
- * ea_list. We use stable sort for this reason.
+ * In place discard duplicates and undefs in sorted ea_list. We use stable sort
+ * for this reason.
  **/
 static inline void
 ea_do_prune(ea_list *e)
@@ -596,10 +596,6 @@ ea_do_prune(ea_list *e)
       if ((s0->type & EAF_TYPE_MASK) == EAF_TYPE_UNDEF)
 	continue;
 
-      /* Drop temporary attributes */
-      if (s0->type & EAF_TEMP)
-	continue;
-
       /* Copy the newest version to destination */
       *d = *s0;
 
@@ -979,8 +975,6 @@ ea_dump(ea_list *e)
 	{
 	  eattr *a = &e->attrs[i];
 	  debug(" %02x:%02x.%02x", EA_PROTO(a->id), EA_ID(a->id), a->flags);
-	  if (a->type & EAF_TEMP)
-	    debug("T");
 	  debug("=%c", "?iO?I?P???S?????" [a->type & EAF_TYPE_MASK]);
 	  if (a->type & EAF_ORIGINATED)
 	    debug("o");
diff --git a/nest/rt-show.c b/nest/rt-show.c
index c7bcdf2f..002a6039 100644
--- a/nest/rt-show.c
+++ b/nest/rt-show.c
@@ -45,10 +45,11 @@ rt_show_rte(struct cli *c, byte *ia, rte *e, struct rt_show_data *d)
   else
     from[0] = 0;
 
-  get_route_info = a->src->proto->proto->get_route_info;
   /* Need to normalize the extended attributes */
-  if ((get_route_info || d->verbose) && !rta_is_cached(a))
+  if (d->verbose && !rta_is_cached(a) && a->eattrs)
     ea_normalize(a->eattrs);
+
+  get_route_info = a->src->proto->proto->get_route_info;
   if (get_route_info)
     get_route_info(e, info);
   else
@@ -114,7 +115,7 @@ rt_show_net(struct cli *c, net *n, struct rt_show_data *d)
 	continue;
 
       ee = e;
-      rte_make_tmp_attrs(&e, c->show_pool);
+      rte_make_tmp_attrs(&e, c->show_pool, NULL);
 
       /* Export channel is down, do not try to export routes to it */
       if (ec && (ec->export_state == ES_DOWN))
diff --git a/nest/rt-table.c b/nest/rt-table.c
index 7990fca3..0b796228 100644
--- a/nest/rt-table.c
+++ b/nest/rt-table.c
@@ -326,29 +326,90 @@ rte_cow_rta(rte *r, linpool *lp)
 }
 
 
-/* Note that rte_make_tmp_attr() requires free eattr in ea_list */
+/**
+ * rte_init_tmp_attrs - initialize temporary ea_list for route
+ * @r: route entry to be modified
+ * @lp: linpool from which to allocate attributes
+ * @max: maximum number of added temporary attribus
+ *
+ * This function is supposed to be called from make_tmp_attrs() and
+ * store_tmp_attrs() hooks before rte_make_tmp_attr() / rte_store_tmp_attr()
+ * functions. It allocates &ea_list with length for @max items for temporary
+ * attributes and puts it on top of eattrs stack.
+ */
 void
-rte_make_tmp_attr(rte *r, ea_list *e, uint id, uint type, u32 val)
+rte_init_tmp_attrs(rte *r, linpool *lp, uint max)
+{
+  struct ea_list *e = lp_alloc(lp, sizeof(struct ea_list) + max * sizeof(eattr));
+
+  e->next = r->attrs->eattrs;
+  e->flags = EALF_SORTED | EALF_TEMP;
+  e->count = 0;
+
+  r->attrs->eattrs = e;
+}
+
+/**
+ * rte_make_tmp_attr - make temporary eattr from private route fields
+ * @r: route entry to be modified
+ * @id: attribute ID
+ * @type: attribute type
+ * @val: attribute value (u32 or adata ptr)
+ *
+ * This function is supposed to be called from make_tmp_attrs() hook for
+ * each temporary attribute, after temporary &ea_list was initialized by
+ * rte_init_tmp_attrs(). It checks whether temporary attribute is supposed to
+ * be defined (based on route pflags) and if so then it fills &eattr field in
+ * preallocated temporary &ea_list on top of route @r eattrs stack.
+ *
+ * Note that it may require free &eattr in temporary &ea_list, so it must not be
+ * called more times than @max argument of rte_init_tmp_attrs().
+ */
+void
+rte_make_tmp_attr(rte *r, uint id, uint type, uintptr_t val)
 {
   if (r->pflags & EA_ID_FLAG(id))
   {
+    ea_list *e = r->attrs->eattrs;
     eattr *a = &e->attrs[e->count++];
     a->id = id;
-    a->type = type | EAF_TEMP;
+    a->type = type;
     a->flags = 0;
-    a->u.data = val;
+
+    if (type & EAF_EMBEDDED)
+      a->u.data = (u32) val;
+    else
+      a->u.ptr = (struct adata *) val;
   }
 }
 
-/* Note that rte has to be writable */
-uint
+/**
+ * rte_store_tmp_attr - store temporary eattr to private route fields
+ * @r: route entry to be modified
+ * @id: attribute ID
+ *
+ * This function is supposed to be called from store_tmp_attrs() hook for
+ * each temporary attribute, after temporary &ea_list was initialized by
+ * rte_init_tmp_attrs(). It checks whether temporary attribute is defined in
+ * route @r eattrs stack, updates route pflags accordingly, undefines it by
+ * filling &eattr field in preallocated temporary &ea_list on top of the eattrs
+ * stack, and returns the value. Caller is supposed to store it in the
+ * appropriate private field.
+ *
+ * Note that it may require free &eattr in temporary &ea_list, so it must not be
+ * called more times than @max argument of rte_init_tmp_attrs()
+ */
+uintptr_t
 rte_store_tmp_attr(rte *r, uint id)
 {
-  eattr *a;
-  if (a = ea_find(r->attrs->eattrs, id))
+  ea_list *e = r->attrs->eattrs;
+  eattr *a = ea_find(e->next, id);
+
+  if (a)
   {
+    e->attrs[e->count++] = (struct eattr) { .id = id, .type = EAF_TYPE_UNDEF };
     r->pflags |= EA_ID_FLAG(id);
-    return a->u.data;
+    return (a->type & EAF_EMBEDDED) ? a->u.data : (uintptr_t) a->u.ptr;
   }
   else
   {
@@ -357,6 +418,82 @@ rte_store_tmp_attr(rte *r, uint id)
   }
 }
 
+/**
+ * rte_make_tmp_attrs - prepare route by adding all relevant temporary route attributes
+ * @r: route entry to be modified (may be replaced if COW)
+ * @lp: linpool from which to allocate attributes
+ * @old_attrs: temporary ref to old &rta (may be NULL)
+ *
+ * This function expands privately stored protocol-dependent route attributes
+ * to a uniform &eattr / &ea_list representation. It is essentially a wrapper
+ * around protocol make_tmp_attrs() hook, which does some additional work like
+ * ensuring that route @r is writable.
+ *
+ * The route @r may be read-only (with %REF_COW flag), in that case rw copy is
+ * obtained by rte_cow() and @r is replaced. If @rte is originally rw, it may be
+ * directly modified (and it is never copied).
+ *
+ * If the @old_attrs ptr is supplied, the function obtains another reference of
+ * old cached &rta, that is necessary in some cases (see rte_cow_rta() for
+ * details). It is freed by rte_store_tmp_attrs(), or manually by rta_free().
+ *
+ * Generally, if caller ensures that @r is read-only (e.g. in route export) then
+ * it may ignore @old_attrs (and set it to NULL), but must handle replacement of
+ * @r. If caller ensures that @r is writable (e.g. in route import) then it may
+ * ignore replacement of @r, but it must handle @old_attrs.
+ */
+void
+rte_make_tmp_attrs(rte **r, linpool *lp, rta **old_attrs)
+{
+  void (*make_tmp_attrs)(rte *r, linpool *lp);
+  make_tmp_attrs = (*r)->attrs->src->proto->make_tmp_attrs;
+
+  if (!make_tmp_attrs)
+    return;
+
+  /* We may need to keep ref to old attributes, will be freed in rte_store_tmp_attrs() */
+  if (old_attrs)
+    *old_attrs = rta_is_cached((*r)->attrs) ? rta_clone((*r)->attrs) : NULL;
+
+  *r = rte_cow_rta(*r, lp);
+  make_tmp_attrs(*r, lp);
+}
+
+/**
+ * rte_store_tmp_attrs - store temporary route attributes back to private route fields
+ * @r: route entry to be modified
+ * @lp: linpool from which to allocate attributes
+ * @old_attrs: temporary ref to old &rta
+ *
+ * This function stores temporary route attributes that were expanded by
+ * rte_make_tmp_attrs() back to private route fields and also undefines them.
+ * It is essentially a wrapper around protocol store_tmp_attrs() hook, which
+ * does some additional work like shortcut if there is no change and cleanup
+ * of @old_attrs reference obtained by rte_make_tmp_attrs().
+ */
+static void
+rte_store_tmp_attrs(rte *r, linpool *lp, rta *old_attrs)
+{
+  void (*store_tmp_attrs)(rte *rt, linpool *lp);
+  store_tmp_attrs = r->attrs->src->proto->store_tmp_attrs;
+
+  if (!store_tmp_attrs)
+    return;
+
+  ASSERT(!rta_is_cached(r->attrs));
+
+  /* If there is no new ea_list, we just skip the temporary ea_list */
+  ea_list *ea = r->attrs->eattrs;
+  if (ea && (ea->flags & EALF_TEMP))
+    r->attrs->eattrs = ea->next;
+  else
+    store_tmp_attrs(r, lp);
+
+  /* Free ref we got in rte_make_tmp_attrs(), have to do rta_lookup() first */
+  r->attrs = rta_lookup(r->attrs);
+  rta_free(old_attrs);
+}
+
 
 static int				/* Actually better or at least as good as */
 rte_better(rte *new, rte *old)
@@ -456,7 +593,7 @@ export_filter_(struct channel *c, rte *rt0, rte **rt_free, linpool *pool, int si
       goto accept;
     }
 
-  rte_make_tmp_attrs(&rt, pool);
+  rte_make_tmp_attrs(&rt, pool, NULL);
 
   v = filter && ((filter == FILTER_REJECT) ||
 		 (f_run(filter, &rt, pool,
@@ -1436,26 +1573,27 @@ rte_update2(struct channel *c, const net_addr *n, rte *new, struct rte_src *src)
 	  /* new is a private copy, i could modify it */
 	  new->flags |= REF_FILTERED;
 	}
-      else
+      else if (filter)
 	{
-	  rte_make_tmp_attrs(&new, rte_update_pool);
-	  if (filter && (filter != FILTER_REJECT))
-	    {
-	      ea_list *oldea = new->attrs->eattrs;
-	      int fr = f_run(filter, &new, rte_update_pool, 0);
-	      if (fr > F_ACCEPT)
-		{
-		  stats->imp_updates_filtered++;
-		  rte_trace_in(D_FILTERS, p, new, "filtered out");
+	  rta *old_attrs;
+	  rte_make_tmp_attrs(&new, rte_update_pool, &old_attrs);
 
-		  if (! c->in_keep_filtered)
-		    goto drop;
+	  int fr = f_run(filter, &new, rte_update_pool, 0);
+	  if (fr > F_ACCEPT)
+	  {
+	    stats->imp_updates_filtered++;
+	    rte_trace_in(D_FILTERS, p, new, "filtered out");
 
-		  new->flags |= REF_FILTERED;
-		}
-	      if (new->attrs->eattrs != oldea && src->proto->store_tmp_attrs)
-		src->proto->store_tmp_attrs(new);
+	    if (! c->in_keep_filtered)
+	    {
+	      rta_free(old_attrs);
+	      goto drop;
 	    }
+
+	    new->flags |= REF_FILTERED;
+	  }
+
+	  rte_store_tmp_attrs(new, rte_update_pool, old_attrs);
 	}
       if (!rta_is_cached(new->attrs)) /* Need to copy attributes */
 	new->attrs = rta_lookup(new->attrs);
@@ -1552,7 +1690,7 @@ rt_examine(rtable *t, net_addr *a, struct proto *p, struct filter *filter)
   int v = p->preexport ? p->preexport(p, &rt, rte_update_pool) : 0;
   if (v == RIC_PROCESS)
   {
-    rte_make_tmp_attrs(&rt, rte_update_pool);
+    rte_make_tmp_attrs(&rt, rte_update_pool, NULL);
     v = (f_run(filter, &rt, rte_update_pool, FF_SILENT) <= F_ACCEPT);
   }