[PATCH v2] cache_mngr: add include callback v2
Thomas Haller
thaller at redhat.com
Tue Sep 27 06:53:09 PDT 2016
On Mon, 2016-09-26 at 09:34 +0200, Tobias Jungel wrote:
Hi Tobias,
> 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
I wrtote that? Totally forgot :)
It's exactly what you are implementing, isn't it?
Please mention the URI of the github issue in the commit message.
> > > ---
> > > 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.
I think it's not nice that we don't have a stable ABI between libnl-
route-3.so and libnl-3.so. It means, if you are about to upgrade libnl3
library, and an application is starting at the same time it might load
these two libraries in differing versions, resulting in a crash. That
is unlikely to happen, but still ugly.
Having a stable ABI is a lot of effort and we didn't do that before
either. So, I'd be fine with changing this internal structure in a non-
compatbile way. I guess..., what do you think?
>
> > > 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;
> > > };
> > >
> > > * 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.
I think it's fine to pass on the u64 diff flags. That the meaning of
the flags is (currently) not public API is a missing feature. One day,
we might make them public API. Currently it merely tells you:
"something changed".
But I think a special NL_ACT_UPDATE type would be good to distinguish
from NL_ACT_CHANGE. It really behaves differently and that might be
relevant for the callee.
> ACK on all syntax/style issues. I will come up with a v3 as soon as
> we
> have clarified the statements above.
great;
thanks!!
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/libnl/attachments/20160927/9b3639d7/attachment.sig>
More information about the libnl
mailing list