refactor(connlib): remove Option<RawFd> return value from Callbacks
#4471
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output |
Performance Test ResultsTCP
UDP
|
thomaseizinger
force-pushed
the
refactor/connlib/android-set-tun
branch
from
4c0eecb
to
f7ccba4
Compare
| .expect("session must be set once callbacks get called") | ||
| .set_tun(tun), | ||
| Err(e) => { | ||
| tracing::error!("Failed to make new `Tun`: {e}"); |
There was a problem hiding this comment.
Currently, any error when creating a new Tun device is also only bubbled up
so this isn't a change in behaviour.
There was a problem hiding this comment.
If we fail to create the Tun for some reason, will the app silently go on without tunneling any traffic?
There was a problem hiding this comment.
Yes, that is the behaviour today.
|
I still need to test this on an actual device. |
| let fd = callbacks | ||
| .on_set_interface_config(config.ipv4, config.ipv6, dns_config) | ||
| .ok_or(Error::NoFd)?; | ||
| pub fn with_fd(fd: RawFd) -> Result<Self> { | ||
| // Safety: File descriptor is open. |
There was a problem hiding this comment.
How is this function safe if its safety depends on the caller passing an open fd?
There was a problem hiding this comment.
It isn't but this is already what is on main, I just moved things around a bit.
| .expect("session must be set once callbacks get called") | ||
| .set_tun(tun), | ||
| Err(e) => { | ||
| tracing::error!("Failed to make new `Tun`: {e}"); |
There was a problem hiding this comment.
If we fail to create the Tun for some reason, will the app silently go on without tunneling any traffic?
| Some(MAX_PARTITION_TIME), | ||
| runtime.handle().clone(), | ||
| ); | ||
|
|
||
| callback_handler.session = Some(session.clone()); |
There was a problem hiding this comment.
This isn't gonna work because the instance of CallbackHandler that got passed into Session doesn't have the Session set 🤦♂️
Need to think of something else here.
|
|
||
| // SAFETY: we expect the callback to return a valid file descriptor | ||
| unsafe { self.replace_fd(fd)? }; | ||
| let fd = callbacks.on_update_routes( |
There was a problem hiding this comment.
| let fd = callbacks.on_update_routes( | |
| callbacks.on_update_routes( |
Assignment of a unit-value.
thomaseizinger
force-pushed
the
refactor/connlib/android-set-tun
branch
from
f7ccba4
to
c91ea2e
Compare
|
Outdated. |
This represents a more minimal version of #4159 where we for now only change the Android-related code, making this a much smaller change.
The idea is that, instead of setting the new file descriptor based on the return value from the callback, it is the responsibility of the
connlib-client-androidglue code to construct a newTundevice and set it on theTunnelusing a new command.Related: #4473.