thom311 / libnl

Netlink Library Suite
GNU Lesser General Public License v2.1
419 stars 311 forks source link

Allow to allocate cache manager with custom refill socket #378

Closed ievenbach closed 4 months ago

ievenbach commented 4 months ago

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.

ievenbach commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

ievenbach commented 4 months ago

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 commented 4 months ago

merged, with two follow-up changes.

Thank you!

ievenbach commented 4 months ago

merged, with two follow-up changes.

Thank you!

Thanks!