private_data support in the subscription API

[zinccyy]

Hi,

are there any plans for adding support for user defined private_data to the subscription API, for example in the onOperGet() or onModuleChange() functions the same way it is defined in the sysrepo subscription API? We are considering switching to C++ and would like to have a user defined context stored and retrieved in operational/module change callbacks.

[jktjkt]

Can you share a use case for that? The C++ language supports lambda functions, and their closure can store any auxiliary data (i.e., captured variables) that you might need.

[zinccyy]

I know it's a trivial use case but for example improving code readability. While using lambdas would be ok for some callbacks, for example in the ietf-interfaces plugin we have about 30 operational callbacks and most of them require the custom operational context: https://github.com/telekom/sysrepo-plugin-interfaces/blob/main/src/interfaces/src/plugin/subscription/operational.h

If we were to use lambdas, all callbacks would be defined in the same function for them to be able to receive the context as a capture variable, or the other possibility is using one global context variable which we are trying to avoid.

[jktjkt]

The C++ bindings are written with the assumption that the code uses C++ idioms. That menas leveraging lambdas and closures instead of function pointers and opaque void* private_data.

I think that ietf-interfaces is a very good example, but to be honest, the current code in the repo you linked to contains a tremendous amount of repetition. Why do you need to have 40 separate operational CBs in the first place? Why not just register a single one which would fill in all the individual leafs at once? I just don't see a point of having a 60-line function for setting the neighbor/origin leaf, and another 60 lines for neighbor/is-router.

Our approach to ietf-interfaces is a bit different, we're using rt-netlink's realtime updates and therefore we've implemented push subscriptions instead of the pull-based ones as you're doing, but the same principles still apply: it's OK to have a single CB which fills in a full YANG subtree. Take a look at our implementation here.

[syyyr]

If we were to use lambdas, all callbacks would be defined in the same function for them to be able to receive the context as a capture variable, or the other possibility is using one global context variable which we are trying to avoid.

Using a class with an operator()() is also an option - you can define them outside of functions.

[syyyr]

If you absolutely must use callbacks with a void* private_data parameter, wrap them with this function:

using FuncType = sysrepo::ErrorCode(*)(sysrepo::Session session, uint32_t subscriptionId, std::string_view moduleName, std::optional<std::string_view> subXPath, sysrepo::Event event, uint32_t requestId, void* privateData);

sysrepo::ModuleChangeCb func_private_data_to_lambda(FuncType func, void* privateData, void (*privateDataDeleter)(void* data))
{
    return [func, smartPrivateData = std::shared_ptr<void>(privateData, privateDataDeleter)] (
            sysrepo::Session session,
            uint32_t subscriptionId,
            std::string_view moduleName,
            std::optional<std::string_view> subXPath,
            sysrepo::Event event,
            uint32_t requestId) -> sysrepo::ErrorCode
    {
        return func(session, subscriptionId, moduleName, subXPath, event, requestId, smartPrivateData.get());
    };
}

Then use it like this:

sysrepo::ErrorCode func_cb(sysrepo::Session session, uint32_t subscriptionId, std::string_view moduleName, std::optional<std::string_view> subXPath, sysrepo::Event event, uint32_t requestId, void* privateData)
{
    // ...
}
void private_data_deleter(void* data)
{
    // ...
}
...
...
...
auto private_data = ...;
auto sub = session.onModuleChange(moduleName, func_private_data_to_lambda(func_cb, private_data, private_data_deleter));

I didn't test this code, so I'm not 100% sure it is correct. There is a slight overhead with the shared_ptr. It is needed to automatically call your deleter. I doubt it's anything significant though. Then there's the additional function call and copies of arguments.

So, as I see it, your options are: 1) Use the tools available in C++ - function objects and lambdas. 2) Use the solution I posted - it'll probably work. Mind the slight overhead though. Also, this function will probably never exist in sysrepo-cpp, so you'd have to maintain it. 3) Don't use a callback context, i.e. use a global. 4) Don't switch to C++. 5) Switch to C++, but use the sysrepo C API.

[jktjkt]

Thanks for an excellent answer, @syyyr.

There is a slight overhead with the shared_ptr. It is needed to automatically call your deleter.

In @mcindrich's case they probably do not need lifetime management. Right now the registration looks like this, and the private_data is just passed along:

    for (size_t i = 0; i < ARRAY_SIZE(oper); i++) {
        const srpc_operational_t* op = &oper[i];

        SRPLG_LOG_INF(PLUGIN_NAME, "Subscribing operational callback %s:%s", op->module, op->path);

        // in case of work on a specific callback set it to NULL
        if (op->cb) {
            error = sr_oper_get_subscribe(running_session, op->module, op->path, op->cb, *private_data, SR_SUBSCR_DEFAULT, &subscription);
            if (error) {
                SRPLG_LOG_ERR(PLUGIN_NAME, "sr_oper_get_subscribe() error for \"%s\" (%d): %s", op->path, error, sr_strerror(error));
                goto error_out;
            }
        }
    }

In C++, that wrapping with a lambda can be a trivial adaptation. You'll have to change the signature of these types obviously, but you're already doing that because of the libyang-cpp data structures which are used instead of the C versions, so passing the private_data becomes a one-liner:

for (const auto& op: oper) {
  sub.onOperGet(op.module, op.path, [private_data](auto session, auto, auto moduleName, auto subXPath, auto requestXPath, auto, auto& output) {
    op.cb(session, requestXPath, output, private_data);
  }, op->path);
}

[zinccyy]

Thank you for providing all possible solutions. As for the larger number of callbacks, it was an internal decision to split one CB which retrieves the whole yang tree into multiple CBs due to code readability and task separation.

I think the best solution for us will be to use function objects as you mentioned. It makes the most sense in this case for storing user defined data in each CB.