cache: don't decide cache operation based on NL_OBJ_DUMP in ce_flags

Objects used to be marked as NL_OBJ_DUMP in ce_flags. Then,
later, the flag was evaluated to decide whether:

  - append/prepend the object in the hash-table (relevant for for routes)
  - ignore duplicate nexthops for routes during oo_update().

That becomes for example rather confusing for pickup_checkdup_cb()
which marks the object as NL_OBJ_DUMP for nl_cache_add(), but not during
nl_object_update() (why?).

Another example where that is confusing is nl_cache_move(), which would
append/prepend the object based on (obj->ce_msgflags & NLM_F_APPEND).
Is that correct behavior? Maybe, maybe it doesn't matter. Either way
it is confusing to have the decision whether to append/prepend the
object in the hash-table 3 functions down the call stack, instead
as explict argument to __cache_add().

Instead, callers must tell nl_cache_add() whether to append the flag,
and they must tell nl_object_update() whether the object was received
during a dump.

Signed-off-by: Thomas Haller <thaller@redhat.com>

5 files changed, 56 insertions, 31 deletions

diff --git a/include/netlink-private/object-api.h b/include/netlink-private/object-api.h
index 6f53d27..e94b67c 100644
--- a/include/netlink-private/object-api.h
+++ b/include/netlink-private/object-api.h
@@ -20,6 +20,13 @@
 extern "C" {
 #endif
 
+enum oo_update_flags {
+	OO_UPDATE_FLAGS_NONE = 0,
+
+	/* object received due to a dump */
+	OO_UPDATE_FLAGS_IS_DUMP = 1,
+};
+
 /**
  * @ingroup object
  * @defgroup object_api Object API
@@ -350,7 +357,8 @@ struct nl_object_ops
 	 * to update. In case of failure its assumed that the original
 	 * object is not touched
 	 */
-	int   (*oo_update)(struct nl_object *, struct nl_object *);
+	int   (*oo_update)(struct nl_object *, struct nl_object *,
+	                   enum oo_update_flags flags);
 
 	/**
 	 * Hash Key generator function
@@ -375,6 +383,10 @@ struct nl_object_ops
 	uint32_t   (*oo_hash_attrs_get)(struct nl_object *);
 };
 
+int _nl_object_update(struct nl_object *dst,
+                      struct nl_object *src,
+                      enum oo_update_flags flags);
+
 /** @} */
 
 #ifdef __cplusplus
diff --git a/include/netlink-private/types.h b/include/netlink-private/types.h
index ad1a6d0..0f67ddd 100644
--- a/include/netlink-private/types.h
+++ b/include/netlink-private/types.h
@@ -114,7 +114,6 @@ struct nl_parser_param;
 #define ID_COMPARISON           2
 
 #define NL_OBJ_MARK		1
-#define NL_OBJ_DUMP		2	/* object received due to a dump */
 
 struct nl_data
 {
diff --git a/lib/cache.c b/lib/cache.c
index f0a626c..20330d6 100644
--- a/lib/cache.c
+++ b/lib/cache.c
@@ -56,6 +56,10 @@
 #include <netlink/hashtable.h>
 #include <netlink/utils.h>
 
+static int _nl_cache_add(struct nl_cache *cache, struct nl_object *obj, int append);
+static int _nl_cache_include(struct nl_cache *cache, struct nl_object *obj,
+                             change_func_t change_cb, void *data, int is_dump);
+
 /**
  * @name Access Functions
  * @{
@@ -431,7 +435,7 @@ void nl_cache_put(struct nl_cache *cache)
  * @{
  */
 
-static int __cache_add(struct nl_cache *cache, struct nl_object *obj)
+static int __cache_add(struct nl_cache *cache, struct nl_object *obj, int append)
 {
 	int ret;
 
@@ -439,8 +443,7 @@ static int __cache_add(struct nl_cache *cache, struct nl_object *obj)
 
 	if (cache->hashtable) {
 		ret = _nl_hash_table_add(cache->hashtable, obj,
-		                         (obj->ce_msgflags & NLM_F_APPEND) ||
-		                         (obj->ce_flags & NL_OBJ_DUMP));
+		                         append);
 		if (ret < 0) {
 			obj->ce_cache = NULL;
 			return ret;
@@ -480,6 +483,12 @@ static int __cache_add(struct nl_cache *cache, struct nl_object *obj)
  */
 int nl_cache_add(struct nl_cache *cache, struct nl_object *obj)
 {
+	return _nl_cache_add(cache, obj,
+	                     (obj->ce_msgflags & NLM_F_APPEND));
+}
+
+static int _nl_cache_add(struct nl_cache *cache, struct nl_object *obj, int append)
+{
 	struct nl_object *new;
 	int ret = 0;
 
@@ -497,7 +506,7 @@ int nl_cache_add(struct nl_cache *cache, struct nl_object *obj)
 		new = obj;
 	}
 
-	ret = __cache_add(cache, new);
+	ret = __cache_add(cache, new, append);
 	if (ret < 0)
 		nl_object_put(new);
 
@@ -537,7 +546,8 @@ int nl_cache_move(struct nl_cache *cache, struct nl_object *obj)
 	if (!nl_list_empty(&obj->ce_list))
 		nl_cache_remove(obj);
 
-	return __cache_add(cache, obj);
+	return __cache_add(cache, obj,
+	                   (obj->ce_msgflags & NLM_F_APPEND));
 }
 
 /**
@@ -716,7 +726,6 @@ static int pickup_checkdup_cb(struct nl_object *c, struct nl_parser_param *p)
 {
 	struct nl_cache *cache = (struct nl_cache *)p->pp_arg;
 	struct nl_object *old;
-	int ret;
 
 	old = nl_cache_search(cache, c);
 	if (old) {
@@ -729,11 +738,7 @@ static int pickup_checkdup_cb(struct nl_object *c, struct nl_parser_param *p)
 		nl_object_put(old);
 	}
 
-	c->ce_flags |= NL_OBJ_DUMP;
-	ret = nl_cache_add(cache, c);
-	c->ce_flags &= ~NL_OBJ_DUMP;
-
-	return ret;
+	return _nl_cache_add(cache, c, 1);
 }
 
 static int pickup_cb(struct nl_object *c, struct nl_parser_param *p)
@@ -793,7 +798,7 @@ int nl_cache_pickup(struct nl_sock *sk, struct nl_cache *cache)
 
 static int cache_include(struct nl_cache *cache, struct nl_object *obj,
 			 struct nl_msgtype *type, change_func_t cb,
-			 void *data)
+			 void *data, int is_dump)
 {
 	struct nl_object *old;
 
@@ -807,7 +812,7 @@ static int cache_include(struct nl_cache *cache, struct nl_object *obj,
 			 * object with the old existing cache object.
 			 * Handle them first.
 			 */
-			if (nl_object_update(old, obj) == 0) {
+			if (_nl_object_update(old, obj, is_dump ? OO_UPDATE_FLAGS_IS_DUMP : 0) == 0) {
 				if (cb)
 					cb(cache, old, NL_ACT_CHANGE, data);
 				nl_object_put(old);
@@ -843,7 +848,13 @@ static int cache_include(struct nl_cache *cache, struct nl_object *obj,
 }
 
 int nl_cache_include(struct nl_cache *cache, struct nl_object *obj,
-		     change_func_t change_cb, void *data)
+                     change_func_t change_cb, void *data)
+{
+	return _nl_cache_include (cache, obj, change_cb, data, 0);
+}
+
+static int _nl_cache_include(struct nl_cache *cache, struct nl_object *obj,
+                             change_func_t change_cb, void *data, int is_dump)
 {
 	struct nl_cache_ops *ops = cache->c_ops;
 	int i;
@@ -854,7 +865,7 @@ int nl_cache_include(struct nl_cache *cache, struct nl_object *obj,
 	for (i = 0; ops->co_msgtypes[i].mt_id >= 0; i++)
 		if (ops->co_msgtypes[i].mt_id == obj->ce_msgtype)
 			return cache_include(cache, obj, &ops->co_msgtypes[i],
-					     change_cb, data);
+					     change_cb, data, is_dump);
 
 	NL_DBG(3, "Object %p does not seem to belong to cache %p <%s>\n",
 	       obj, cache, nl_cache_name(cache));
@@ -865,13 +876,8 @@ int nl_cache_include(struct nl_cache *cache, struct nl_object *obj,
 static int resync_cb(struct nl_object *c, struct nl_parser_param *p)
 {
 	struct nl_cache_assoc *ca = p->pp_arg;
-	int ret;
-
-	c->ce_flags |= NL_OBJ_DUMP;
-	ret = nl_cache_include(ca->ca_cache, c, ca->ca_change, ca->ca_change_data);
-	c->ce_flags &= ~NL_OBJ_DUMP;
 
-    return ret;
+	return _nl_cache_include(ca->ca_cache, c, ca->ca_change, ca->ca_change_data, 1);
 }
 
 int nl_cache_resync(struct nl_sock *sk, struct nl_cache *cache,
diff --git a/lib/object.c b/lib/object.c
index dcf2a36..577eceb 100644
--- a/lib/object.c
+++ b/lib/object.c
@@ -144,6 +144,18 @@ struct nl_object *nl_object_clone(struct nl_object *obj)
 	return new;
 }
 
+int _nl_object_update(struct nl_object *dst,
+                      struct nl_object *src,
+                      enum oo_update_flags flags)
+{
+	struct nl_object_ops *ops = obj_ops(dst);
+
+	if (ops->oo_update)
+		return ops->oo_update(dst, src, flags);
+
+	return -NLE_OPNOTSUPP;
+}
+
 /**
  * Merge a cacheable object
  * @arg dst		object to be merged into
@@ -153,12 +165,7 @@ struct nl_object *nl_object_clone(struct nl_object *obj)
  */
 int nl_object_update(struct nl_object *dst, struct nl_object *src)
 {
-	struct nl_object_ops *ops = obj_ops(dst);
-
-	if (ops->oo_update)
-		return ops->oo_update(dst, src);
-
-	return -NLE_OPNOTSUPP;
+	return _nl_object_update(dst, src, OO_UPDATE_FLAGS_NONE);
 }
 
 /**
diff --git a/lib/route/route_obj.c b/lib/route/route_obj.c
index 0261c42..36581b3 100644
--- a/lib/route/route_obj.c
+++ b/lib/route/route_obj.c
@@ -478,7 +478,8 @@ nh_mismatch:
 #undef ROUTE_DIFF
 }
 
-static int route_update(struct nl_object *old_obj, struct nl_object *new_obj)
+static int route_update(struct nl_object *old_obj, struct nl_object *new_obj,
+                        enum oo_update_flags flags)
 {
 	struct rtnl_route *new_route = (struct rtnl_route *) new_obj;
 	struct rtnl_route *old_route = (struct rtnl_route *) old_obj;
@@ -526,7 +527,7 @@ static int route_update(struct nl_object *old_obj, struct nl_object *new_obj)
 		if (!cloned_nh)
 			return -NLE_NOMEM;
 
-		if (new_obj->ce_flags & NL_OBJ_DUMP) {
+		if (flags & OO_UPDATE_FLAGS_IS_DUMP) {
 			struct rtnl_nexthop *old_nh;
 			char nbuf[256];
 			char obuf[256];