[PATCH v2] cache_mngr: add include callback v2
Tobias Jungel
tobias.jungel at bisdn.de
Mon Sep 26 00:34:04 PDT 2016
Hi Thomas,
all inline
On Sa, 2016-09-24 at 16:23 +0200, Thomas Haller wrote:
> On Mon, 2016-09-12 at 21:54 +0200, Tobias Jungel wrote:
> >
> > This patch adds change_func_v2_t to add a more detailed callback in
> > case of a cache change. In case the new change function is set
> > using
> > nl_cache_mngr_set_change_func_v2, nl_cache_include_v2 and thus
> > cache_include_v2 will be used perform the cache inclusion.
> >
> > The parameter of change_func_v2_t are the following:
> > * struct nl_cache * => cache
> > * struct nl_object * => the old/deleted nl_object
> > * struct nl_object * => the new nl_object
> > * uint64_t => the result of nl_object_diff64 in case of a change
> > * int => NL_ACT_*
> > * void * => data
> >
> > Signed-off-by: Tobias Jungel <tobias.jungel at bisdn.de>
>
> Hi Tobias,
>
>
> adding a v2 callback is not so nice... Ok, I don't see how to do it
> better, if you need this functionality.
>
> Can you justify shortly why you (really) need this functionality?
Yes it would be really great to have this functionality. Otherwise I
might have to have a second cache to keep track of the old entries. If
I am not wrong there is currently no way to determine what has actually
changed, there is just the updated entry. I am open to ideas to make
this (and even the name) even nicer.
Even if its a bit old there was an Issue, that I should have referenced
as well:
https://github.com/thom311/libnl/issues/71
>
>
>
> >
> > ---
> > include/netlink-private/cache-api.h | 1 +
> > include/netlink-private/types.h | 1 +
> > include/netlink/cache.h | 10 +++++
> > lib/cache.c | 68
> > +++++++++++++++++++++++++++++-----
> > lib/cache_mngr.c | 73
> > ++++++++++++++++++++++++++++++++++++-
> > libnl-3.sym | 2 +
> > 6 files changed, 144 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/netlink-private/cache-api.h b/include/netlink-
> > private/cache-api.h
> > index f3d9f01..c306eb5 100644
> > --- a/include/netlink-private/cache-api.h
> > +++ b/include/netlink-private/cache-api.h
> > @@ -236,6 +236,7 @@ struct nl_cache_ops
> > *
> > * @see nl_cache_include()
> > */
> > + // TODO this has to be updated as well to support cb_v2
> > int (*co_include_event)(struct nl_cache *cache, struct
> > nl_object *obj,
>
> this is private API, so you can just change it as there is no stable
> ABI between libnl-route-3.so and libnl-3.so.
Indeed, but I wanted to get at least some feedback before I start with
this one. Another way would have been to expose this callback to the
public API, but it seems to me pretty internal, so I went for the other
approach.
>
> >
> > change_func_t change_cb, void
> > *data);
> >
> > diff --git a/include/netlink-private/types.h b/include/netlink-
> > private/types.h
> > index e80c8a1..4702162 100644
> > --- a/include/netlink-private/types.h
> > +++ b/include/netlink-private/types.h
> > @@ -96,6 +96,7 @@ struct nl_cache_assoc
> > {
> > struct nl_cache * ca_cache;
> > change_func_t ca_change;
> > + change_func_v2_t ca_change_v2;
> > void * ca_change_data;
> > };
> >
> > diff --git a/include/netlink/cache.h b/include/netlink/cache.h
> > index 71eaceb..ad8da60 100644
> > --- a/include/netlink/cache.h
> > +++ b/include/netlink/cache.h
> > @@ -35,6 +35,8 @@ enum {
> >
> > struct nl_cache;
> > typedef void (*change_func_t)(struct nl_cache *, struct nl_object
> > *,
> > int, void *);
> > +typedef void (*change_func_v2_t)(struct nl_cache *, struct
> > nl_object
> > *,
> > + struct nl_object *, uint64_t, int, void *);
> >
> > /**
> > * @ingroup cache
> > @@ -88,6 +90,10 @@ extern int nl_cache_inclu
> > de
> > (struct nl_cache *,
> > struct nl_object
> > *,
> > change_func_t,
> > void *);
> > +extern int nl_cache_include_v2(struct
> > nl_cache *,
> > + struct
> > nl_object
> > *,
> > + change_func_v2
> > _t
> > ,
> > + void *);
> > extern void nl_cache_set_arg1(struct
> > nl_cache
> > *, int);
> > extern void nl_cache_set_arg2(struct
> > nl_cache
> > *, int);
> > extern void nl_cache_set_flags(struct
> > nl_cache *, unsigned int);
> > @@ -154,6 +160,10 @@ extern int nl_cache_mng
> > r_
> > add(struct nl_cache_mngr *,
> > extern int nl_cache_mngr_add_cache(struct
> > nl_cache_mngr *mngr,
> > struct
> > nl_cache *cache,
> > change_fun
> > c_
> > t cb, void *data);
> > +extern int nl_cache_mngr_set_change_func_v2
> > (s
> > truct nl_cache_mngr *mngr,
> > + s
> > tr
> > uct nl_cache *cache,
> > + c
> > ha
> > nge_func_v2_t cb,
> > + v
> > oi
> > d *data);
> > extern int nl_cache_mngr_get_fd(struct
> > nl_cache_mngr *);
> > extern int nl_cache_mngr_poll(struct
> > nl_cache_mngr *,
> > int);
> > diff --git a/lib/cache.c b/lib/cache.c
> > index d8592b6..8e9de0d 100644
> > --- a/lib/cache.c
> > +++ b/lib/cache.c
> > @@ -784,30 +784,45 @@ 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)
> > + struct nl_msgtype *type, change_func_t
> > cb,
> > + change_func_v2_t cb_v2, void *data)
> > {
> > struct nl_object *old;
> > + struct nl_object *clone = NULL;
>
> trailing whitespace
>
> >
> > + uint64_t diff;
> >
> > switch (type->mt_act) {
> > case NL_ACT_NEW:
> > case NL_ACT_DEL:
> > old = nl_cache_search(cache, obj);
> > if (old) {
> > + if (cb_v2) {
> > + clone = nl_object_clone(old);
> > + diff = nl_object_diff64(old, obj);
> > + }
> > /*
> > * Some objects types might support
> > merging
> > the new
> > * object with the old existing cache
> > object.
> > * Handle them first.
> > */
> > if (nl_object_update(old, obj) == 0) {
> > - if (cb)
> > + if (cb_v2) {
> > + cb_v2(cache, clone, obj,
> > diff,
> > + NL_ACT_CHANGE,
> > data);
>
> in this case the old-argument "clone" was never part of the cache.
> I think that is rather unexpected, isn't it?
Well, it came from nl_cache_search and in case the following
nl_object_update succeeds, there is no way of retrieving the old entry,
is it?
>
> Maybe there should be a new NL_ACT_UPDATE for this case. Then you
> could
> also avoid calculating "diff", because it's clear that with
> NL_ACT_UPDATE:
>
> - diff is 0
> - old is a clone before the update (but itself was never inside
> the
> cache)
> - new is the updated object aftet the update.
>
> after all, "diff" is only mildly useful because all the flags are
> internal API, so a user doesn't know what these flags mean anyway.
> For that reason, maybe the "diff" argument shouldn't be passed to the
> callback, but well...
Right, either the flags go to the external API or the diff param goes
away. Sorry, I didn't check this since nl_object_diff64 is available on
the external API including all bits and not just being boolean.
>
>
> >
> > + nl_object_put(clone);
> > + } else if (cb)
> > cb(cache, old,
> > NL_ACT_CHANGE, data);
> > nl_object_put(old);
> > return 0;
> > }
> > + nl_object_put(clone);
> >
> > nl_cache_remove(old);
> > if (type->mt_act == NL_ACT_DEL) {
> > - if (cb)
> > + if (cb_v2)
> > + cb_v2(cache, old, NULL, 0,
> > NL_ACT_DEL,
> > + data);
>
> When the command of on "if" spawns more then one line, please { }.
> Same below.
>
> >
> > + else if (cb)
> > cb(cache, old, NL_ACT_DEL,
> > data);
> > nl_object_put(old);
> > }
> > @@ -815,10 +830,20 @@ static int cache_include(struct nl_cache
> > *cache, struct nl_object *obj,
> >
> > if (type->mt_act == NL_ACT_NEW) {
> > nl_cache_move(cache, obj);
> > - if (old == NULL && cb)
> > - cb(cache, obj, NL_ACT_NEW, data);
> > - else if (old) {
> > - if (nl_object_diff(old, obj) &&
> > cb)
> > + if (old == NULL) {
> > + if (cb_v2)
> > + cb_v2(cache, NULL, obj, 0,
> > NL_ACT_NEW,
> > + data);
> > + else if (cb)
> > + cb(cache, obj, NL_ACT_NEW,
> > data);
> > + } else if (old) {
> > + uint64_t diff = 0;
> > + if (cb || cb_v2)
>
> whitespace.
>
> >
> >
> > + diff =
> > nl_object_diff64(old,
> > obj);
> > + if (diff && cb_v2)
> > + cb_v2(cache, old, obj,
> > diff,
> > NL_ACT_CHANGE,
> > + data);
> > + else if (diff && cb)
> > cb(cache, obj,
> > NL_ACT_CHANGE, data);
> >
> > nl_object_put(old);
> > @@ -845,7 +870,27 @@ 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, NULL,
> > data);
> > +
> > + NL_DBG(3, "Object %p does not seem to belong to cache %p
> > <%s>\n",
> > + obj, cache, nl_cache_name(cache));
> > +
> > + return -NLE_MSGTYPE_NOSUPPORT;
> > +}
> > +
> > +int nl_cache_include_v2(struct nl_cache *cache, struct nl_object
> > *obj,
> > + change_func_v2_t change_cb, void *data)
> > +{
> > + struct nl_cache_ops *ops = cache->c_ops;
> > + int i;
> > +
> > + if (ops->co_obj_ops != obj->ce_ops)
> > + return -NLE_OBJ_MISMATCH;
> > +
> > + 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],
> > + NULL, change_cb,
> > data);
> >
> > NL_DBG(3, "Object %p does not seem to belong to cache %p
> > <%s>\n",
> > obj, cache, nl_cache_name(cache));
> > @@ -857,7 +902,12 @@ static int resync_cb(struct nl_object *c,
> > struct
> > nl_parser_param *p)
> > {
> > struct nl_cache_assoc *ca = p->pp_arg;
> >
> > - return nl_cache_include(ca->ca_cache, c, ca->ca_change,
> > ca-
> > >
> > > ca_change_data);
> > + if (ca->ca_change_v2)
> > + return nl_cache_include_v2(ca->ca_cache, c, ca-
> > >
> > > ca_change_v2,
> > + ca->ca_change_data);
> > + else
> > + return nl_cache_include(ca->ca_cache, c, ca-
> > >
> > > ca_change,
> > + ca->ca_change_data);
> > }
> >
> > int nl_cache_resync(struct nl_sock *sk, struct nl_cache *cache,
> > diff --git a/lib/cache_mngr.c b/lib/cache_mngr.c
> > index 1f23eb1..8715931 100644
> > --- a/lib/cache_mngr.c
> > +++ b/lib/cache_mngr.c
> > @@ -61,8 +61,13 @@ static int include_cb(struct nl_object *obj,
> > struct nl_parser_param *p)
> > if (ops->co_include_event)
> > return ops->co_include_event(ca->ca_cache, obj,
> > ca-
> > >
> > > ca_change,
> > ca->ca_change_data);
> > - else
> > - return nl_cache_include(ca->ca_cache, obj, ca-
> > >
> > > ca_change, ca->ca_change_data);
> > + else {
> > + if (ca->ca_change_v2)
> > + return nl_cache_include_v2(ca->ca_cache,
> > obj, ca->ca_change_v2, ca->ca_change_data);
> > + else
> > + return nl_cache_include(ca->ca_cache, obj,
> > ca->ca_change, ca->ca_change_data);
> > + }
> > +
> > }
> >
> > static int event_input(struct nl_msg *msg, void *arg)
> > @@ -349,6 +354,70 @@ errout_free_cache:
> > }
> >
> > /**
> > + * Set change_func_v2 for cache manager
> > + * @arg mngr Cache manager.
> > + * @arg cache Cache associated with the callback
> > + * @arg cb Function to be called upon changes.
> > + * @arg data Argument passed on to change callback
> > + *
> > + * Adds callback change_func_v2 to a registered cache. This
> > callback
> > provides
> > + * in like the standard change_func the added or remove netlink
> > object. In case
> > + * of a change the old and the new object is provided as well as
> > the
> > according
> > + * diff. If this callback is registered this has a higher priority
> > then the
> > + * change_func registered during cache registration. Hence only
> > one
> > callback is
> > + * executed.
> > + *
> > + * The first netlink object in the callback is refering to the old
> > object and
> > + * the second to the new. This means on NL_ACT_CHANGE the first is
> > the previous
> > + * object in the cache and the second the updated version. On
> > NL_ACT_DEL the
> > + * first is the deleted object the second is NULL. On NL_ACT_NEW
> > the
> > first is
> > + * NULL and the second the new netlink object.
> > + *
> > + * The user is responsible for calling nl_cache_mngr_poll() or
> > monitor
> > + * the socket and call nl_cache_mngr_data_ready() to allow the
> > library
> > + * to process netlink notification events.
> > + *
> > + * @see nl_cache_mngr_poll()
> > + * @see nl_cache_mngr_data_ready()
> > + *
> > + * @return 0 on success or a negative error code.
> > + * @return -NLE_PROTO_MISMATCH Protocol mismatch between cache
> > manager and
> > + * cache type
> > + * @return -NLE_OPNOTSUPP Cache type does not support updates
> > + * @return -NLE_RANGE Cache of this type is not registered
> > + */
> > +int nl_cache_mngr_set_change_func_v2(struct nl_cache_mngr *mngr,
> > + struct nl_cache *cache,
> > + change_func_v2_t cb, void
>
> is a user supposed to call first nl_cache_mngr_add_cache() followed
> by nl_cache_mngr_set_change_func_v2()?
>
> That seems confusing, maybe there should be a
> nl_cache_mngr_add_cache_v2().
Agree.
>
>
>
> >
> > *data)
> > +{
> > + struct nl_cache_ops *ops;
> > + int i;
> > +
> > + ops = cache->c_ops;
> > + if (!ops)
> > + return -NLE_INVAL;
> > +
> > + if (ops->co_protocol != mngr->cm_protocol)
> > + return -NLE_PROTO_MISMATCH;
> > +
> > + if (ops->co_groups == NULL)
> > + return -NLE_OPNOTSUPP;
> > +
> > + for (i = 0; i < mngr->cm_nassocs; i++)
> > + if (mngr->cm_assocs[i].ca_cache == cache)
> > + break;
> > +
> > + if (i >= mngr->cm_nassocs) {
> > + return -NLE_RANGE;
> > + }
> > +
> > + mngr->cm_assocs[i].ca_change_v2 = cb;
> > + mngr->cm_assocs[i].ca_change_data = data; // TODO
> > different
> > data for v2?
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > * Get socket file descriptor
> > * @arg mngr Cache Manager
> > *
> > diff --git a/libnl-3.sym b/libnl-3.sym
> > index 9119e66..0ba82e0 100644
> > --- a/libnl-3.sym
> > +++ b/libnl-3.sym
> > @@ -68,6 +68,7 @@ global:
> > nl_cache_get_ops;
> > nl_cache_get_prev;
> > nl_cache_include;
> > + nl_cache_include_v2;
>
> a linker symbols section must not be modified/extended after the
> release. New symbols must go to a separate section, that is:
> libnl_3_2_29.
Oh right. Thanks!
>
> >
> > nl_cache_is_empty;
> > nl_cache_mark_all;
> > nl_cache_mngr_add;
> > @@ -82,6 +83,7 @@ global:
> > nl_cache_mngt_register;
> > nl_cache_mngt_require;
> > nl_cache_mngt_require_safe;
> > + nl_cache_mngr_set_change_func_v2;
> > nl_cache_mngt_unprovide;
> > nl_cache_mngt_unregister;
> > nl_cache_move;
>
>
> Thomas
ACK on all syntax/style issues. I will come up with a v3 as soon as we
have clarified the statements above.
Best
Tobi
More information about the libnl
mailing list