Do not invoke accept() on an already-established connection
Hi,
First of all, thanks a lot for writing and maintaining the Rust reference implementation of varlink!
Here's a proposed fix for #73; the Listener::new() method already records the fact that this listener is actually a connection that has already been established via socket activation, and lets the Listener::accept() method honor that flag.
I have written a trivial varlink server - my varlink-hello GitLab project - to demonstrate the need for this change. Without it, with the stock varlink 11.0.1 crate, the varlink-hello program fails, since Listener::accept() attempts to invoke the accept() method of the "Unix listener" which is actually an already-connected socket.
Now, there are three things I don't completely like about this patch:
- I'm not overly fond of the use of
unsafe { ... }inListener::accept(). A cleaner way to do that would be to add two new values to theListenerenum, e.g.TCPAcceptedandUNIXAccepted, and makeListener::new()create aTcpListeneror aUnixListenerdirectly, in its ownunsafe { ... }blocks. This might cause a bit of code churn; let me know if I should do it. - I currently have no way of testing the Windows implementation; I'd be very grateful if somebody could try to build the
varlink-helloprogram under Windows and see if a) it fails with the currently-releasedvarlinkcrate, and b) this change fixes it. - I am not entirely sure how to write tests for this change. An obvious way would be to copy the
test_listen()function invarlink/src/tests.rs, create a socketpair, and go for it, but the problem is that to really test it, theLISTEN_FDS,LISTEN_FDNAMES, andLISTEN_PIDenvironment variables need to be set, which might influence other tests run either at the same time or later. I'd be grateful for any ideas on how to approach this; writing a new binary crate that will only be used for testing almost seems like a viable approach, even though Cargo currently does not support test-only binary crates.
Thanks in advance for your time, and keep up the great work!
G'luck, Peter
JFTR, I edited the initial comment to add the third thing I'm not completely sure about.