Allow to allocate cache manager with custom refill socket

[ievenbach]

Cache managers use two sockets: one for cache refill operation,
and another one for notifications.

In order to simulate NETLINK events by reading data from files,
we need to be able to overwrite callbacks for both sockets.

This new function allows us to set up refill socket any way we want.
It does have requirement that the refill socket be blocking.

[thom311]

In order to simulate NETLINK events by reading data from files,
we need to be able to overwrite both sockets.

Passing a nl_cb argument for the internal socket doesn't really overwrite the socket. Wouldn't it be better to have instead a struct nl_sock *sync_sk argument?

[thom311]

new ABI must be listed in libnl-3.sym file.

[thom311]

The new entry MUST be added to the bottom of the file. See the comment at the top of the file.

[ievenbach]

In order to simulate NETLINK events by reading data from files,
we need to be able to overwrite both sockets.

Passing a nl_cb argument for the internal socket doesn't really overwrite the socket. Wouldn't it be better to have instead a struct nl_sock *sync_sk argument?

The intent is not to overwrite the socket. I want as much of the functionality as possible intact. Instead, I want to provide a callback that overwrites the send/recv.

Why we need a separate socket at all is a different question.

[ievenbach]

In order to simulate NETLINK events by reading data from files,
we need to be able to overwrite both sockets.

Passing a nl_cb argument for the internal socket doesn't really overwrite the socket. Wouldn't it be better to have instead a struct nl_sock *sync_sk argument?

The intent is not to overwrite the socket. I want as much of the functionality as possible intact. Instead, I want to provide a callback that overwrites the send/recv.

Why we need a separate socket at all is a different question.

Bah. I should read my own commit messages better. Fixed the description.

[thom311]

Why we need a separate socket at all is a different question.

That is the real question. Having two sockets (one for async events and the other for sync fetches) unavoidably means that it's not possibly to bring the messages in a consistent state. This can only correctly work, by using one socket for everything, where all events (sync and async) are in order.

Anyway. That is out of scope for this PR and would be a large work. I don't think anybody is going to fix that.

[thom311]

The intent is not to overwrite the socket. I want as much of the functionality as possible intact. Instead, I want to provide a callback that overwrites the send/recv.

I see.

Since this is quite a special method, I still think it would be better to just accept a full struct nl_sock *sync_sock argument. For this more elaborate use-case, it provides the most flexibility, as the user can prepare the socket in any way the want. It's only mildly more cumbersome for the caller. Since struct nl_sock is not ref-counted, the function necessarily will take ownership of what you pass in.

Also, note that the result argument is an out-argument. Let's keep that the last parameter of the function.

[thom311]

not how for mngr->cm_sock, we call connect and non-blocking above. That means, for the sk argument, the caller is not expected to do that. That may be a questionable choice, but it is probably better to handle his consistently. Meaning: leave the nl_connect(cm_sync_sock) call here, also for the externally provided sk_blocking.

[thom311]

sk_blocking is internally assigned to cm_sync_sock. While it's unclear which is the best name, they probably should be consistent. I think the parameter should be called sync_sk.

[thom311]

this is inconsistent too what happens with sk. On failure, the sockets are left alone. The caller would need to handle them in that case. That is also missing in nl_cache_mngr_alloc() (but that code will go away).

[thom311]

as said, the logic of nl_connect() should not be on the caller.

Hence, most of this code is still in nl_cache_mngr_alloc_ex().

Allow NULL as sk_blocking (sync_sk), and then just do

int nl_cache_mngr_alloc(struct nl_sock *sk, int protocol, int flags,
			struct nl_cache_mngr **result)
{
    return nl_cache_mngr_alloc_ex(sk, NULL, protocol, flags, result);
}

[ievenbach]

That was how I made it originally. However I noticed that presents a problem: we allow passing in two sockets, but cannot differentiate between errors caused by either. Which means we would need extra steps to be taken, to disown them internally in case of error, so that caller could release these.
I just found it easier to be inconsistent with socket assumptions.
I can fix the assumption about the second socket as well to be consistent.

[thom311]

like sk, the new parameter sync_sk/sk_blocking should be optional/nullable. If the user did not specify a blocking socket, in case of error the internally allocated one will still be needed to released.

You need to right logic, so that all internally allocated data is correctly freed on every error path. Use _nl_auto_nl_socket instead of explicit free for that.

[thom311]

Note also NL_ALLOCATED_SOCK, where ownership is only assumed for internally allocated socket. You would need something similar for sync-sock.

[ievenbach]

Note also NL_ALLOCATED_SOCK, where ownership is only assumed for internally allocated socket. You would need something similar for sync-sock.

I rewrote the patch to follow this pattern.

[thom311]

merged, with two follow-up changes.

Thank you!

[ievenbach]

merged, with two follow-up changes.

Thank you!

Thanks!