From cae64f81d5a3d370f232af1a6f82c2213763a26c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 29 Jun 2016 13:07:30 +0200 Subject: 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 --- include/netlink-private/object-api.h | 14 ++++++++++- include/netlink-private/types.h | 1 - lib/cache.c | 48 ++++++++++++++++++++---------------- lib/object.c | 19 +++++++++----- lib/route/route_obj.c | 5 ++-- 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 #include +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]; -- cgit v0.12