Bump: Modernize tun-tap#18
Conversation
* Async API completely rewritten: `Stream + Sink` (tokio-core 0.1 / futures 0.1) replaced with `AsyncRead + AsyncWrite` (tokio 1.x / `AsyncFd`). The `Async` struct no longer implements `Stream` or `Sink`; use `AsyncReadExt` / `AsyncWriteExt` methods instead. * Feature renamed: `tokio` → `async`. Enable with `--features async` (on by default). * `set_recv_bufsize` removed from `Async`; external buffer sizing is now caller-managed via `AsyncRead`. * `tokio-core` 0.1 → `tokio` 1.x (features: net, rt, rt-multi-thread, io-util, macros, time) * `futures` 0.1 → removed (tokio provides async traits) * `mio` 0.6 → removed (tokio's `AsyncFd` replaces raw mio) * `etherparse` 0.9 → 0.17 (dev-dependency) * `serial_test` 0.4 → 3.x (dev-dependency) * `version-sync` → removed * Edition 2024 compliance: `extern "C"` → `unsafe extern "C"`, removed `extern crate` declarations * `build.rs`: removed `extern crate cc;` * `[badges]` section removed from `Cargo.toml` (deprecated by crates.io) * Travis CI replaced with GitHub Actions (`.github/workflows/ci.yml`) * `tests/version.rs` removed (depended on dropped `version-sync`) * Fixed `cmd` function in examples to accept arbitrary command instead of hardcoded `"ip"` (vorner#17) * Fixed typo in doc comment (vorner#16)
vorner
left a comment
There was a problem hiding this comment.
Hello
I'm a bit split about this. On one hand, it's kind of nice that someone found this thing I've kind of considered dead for a long time - there are certainly more modern, more complete and more downloaded crates that do the same thing.
On the other hand, it seems many things are changed without an obvious motivation (or outright deleted without a replacement). Furthermore, many of these changes seem to be done mechanically without some deeper design behind them - things like the r#, or using the AsyncRead and AsyncWrite traits…
| * Async API completely rewritten: `Stream + Sink` (tokio-core 0.1 / futures 0.1) replaced with | ||
| `AsyncRead + AsyncWrite` (tokio 1.x / `AsyncFd`). The `Async` struct no longer implements | ||
| `Stream` or `Sink`; use `AsyncReadExt` / `AsyncWriteExt` methods instead. | ||
| * Feature renamed: `tokio` → `async`. Enable with `--features async` (on by default). |
There was a problem hiding this comment.
I don't think renaming to async is entirely correct - this does depend on a specific async runtime, tokio.
Also, I don't think it makes sense to list documentation to Rust, namely how to enable features - and besides, nobody really does it by that flag, they do so through their Cargo.toml.
| * `build.rs`: removed `extern crate cc;` | ||
| * `[badges]` section removed from `Cargo.toml` (deprecated by crates.io) | ||
| * Travis CI replaced with GitHub Actions (`.github/workflows/ci.yml`) | ||
| * `tests/version.rs` removed (depended on dropped `version-sync`) |
There was a problem hiding this comment.
These are somewhat internal things. I don't think these belong to changelog - that's for consumers of the crate.
| #[test] | ||
| fn test_html_root_url() { | ||
| assert_html_root_url_updated!("src/lib.rs"); | ||
| } |
There was a problem hiding this comment.
Can you explain the motivation for dropping this one? It did catch me previously forgetting to update the link in lib.rs, so it feels like it's kind of useful.
| #[cfg(feature = "tokio")] | ||
| pub mod async; | ||
| #[cfg(feature = "async")] | ||
| pub mod r#async; |
There was a problem hiding this comment.
Given we are doing a breaking change, I'd say it makes sense to rename the module instead of using the r# hack (which forces downstream users to use it too).
| //! You really do want better error handling than all these unwraps. | ||
|
|
||
| extern crate tun_tap; | ||
| //! that. |
| let iface = Iface::new("testtun%d", Mode::Tun).unwrap(); | ||
| eprintln!("Iface: {:?}", iface); | ||
| // Configure the „local“ (kernel) endpoint. Kernel is (the host) 10.107.1.3, we (the app) | ||
| // Configure the "local" (kernel) endpoint. Kernel is (the host) 10.107.1.3, we (the app) |
There was a problem hiding this comment.
On one hand, you're replacing dashes with the more correct long ones, on the other, you're removing the typographically correct quotes?
| } | ||
| }); | ||
| core.run(reader.join(writer)).unwrap(); | ||
| tokio::try_join!(write_task, read_task).unwrap(); |
There was a problem hiding this comment.
What's the motivation behind making it run forever instead of just once? The PR seems to be about modernizing things, but you seem to be doing a lot more for unexplained reasons.
| /// Returns a shared reference to the underlying [`Iface`]. | ||
| pub fn get_ref(&self) -> &Iface { | ||
| self.inner.get_ref() | ||
| } |
There was a problem hiding this comment.
I don't think these are a particularly good idea. Given we have switched the interface to asynchronous mode, its own interface won't really work like designed (everything will just fail on it most of the time), creating a footgun.
| }, | ||
| Err(ref e) if e.kind() == ErrorKind::WouldBlock => Ok(FAsync::NotReady), | ||
| Err(e) => Err(e), | ||
| impl AsyncRead for Async { |
There was a problem hiding this comment.
AsyncRead and AsyncWrite are the wrong abstraction layer here. These model „streaming“ interfaces - things like File or TcpStream. If you read a bit, you continue where you left off. Similarly, you have things like write_all that loop until everything is written by chunks.
The tun/tap things are message based. Each interaction must be a whole, independent packet. If you read just 5 bytes of the packet, you've lost the rest - next time you get the next packet. If you use write_all and try to send one „big“ packet and it happens to get split, you send two (damaged) packets.
Using these traits would lead to occasional and subtle bugs that would be hard to debug.
Notice that things like UdpStream in tokio also don't implement AsyncRead and AsyncWrite.
| } | ||
| Ok(Err(e)) if e.kind() == io::ErrorKind::WouldBlock => continue, | ||
| Ok(Err(e)) => return Poll::Ready(Err(e)), | ||
| Err(_would_block) => continue, |
There was a problem hiding this comment.
The fact we get WouldBlock at two different levels seems somewhat suspicious.
Remove loop in `writer_task` and add a filter in `read_task`, so we can get exact `PING`.
We change the name `async` to avoid using `r#` hack
Replace the stream-based (AsyncRead + AsyncWrite) async interface with datagram-style recv/send methods to match the TUN device's message semantics.
|
Thanks for your review! The initial commits had a number of issues — some due to carelessness on my part, others from design immaturity. I really appreciate you pointing them out and providing valuable suggestions. Here are the key changes I've made since:
|
Summary
Modernize
tun-tapfrom 0.1.4 to 0.2.0: tokio 1.x, edition 2024, and general cleanup. This is a breaking change — the Async API is rewritten fromStream + Sink(futures 0.1) toAsyncRead + AsyncWrite(tokio 1.x).Breaking Changes
Stream + Sink(tokio-core 0.1 / futures 0.1) →AsyncRead + AsyncWrite(tokio 1.x /AsyncFd). UseAsyncReadExt::read/AsyncWriteExt::writeinstead ofStream::poll/Sink::start_send.tokio→async. Enable with--features async(on by default).Async::newsignature: No longer takes aHandle— justAsync::new(iface).set_recv_bufsizeremoved: Buffer sizing is now caller-managed viaAsyncRead.Changes
Dependency upgrades
tokio-core0.1tokio1.xfutures0.1mio0.6etherparse0.9 (dev)serial_test0.4 (dev)version-sync(dev)Code modernization
unsafe extern "C", removedextern cratedeclarationsbuild.rs: removedextern crate cc;[badges]section removed fromCargo.toml(deprecated by crates.io).github/workflows/ci.yml)tests/version.rsremoved (depended on droppedversion-sync)asyncmodule renamed tor#asyncfor edition 2024 keyword compatibilityMigration Guide