-
Notifications
You must be signed in to change notification settings - Fork 447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use smart pointers in SocketSet's API #42
Conversation
2b701e5
to
8502574
Compare
src/socket/socket_ref.rs
Outdated
pub(crate) fn from_socket_ref(mut socket_ref: SocketRef<'a, Socket<'b, 'c>>) | ||
-> Option<SocketRef<'a, T>> { | ||
if let Some(socket) = T::from_socket(socket_ref.socket) { | ||
socket_ref.fused = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible I'm missing something here: I want to prevent a spurious destructor call, but I can't use mem::forget
since socket_ref
is being borrowed from until the end of the method. It doesn't prevent the destructor from being called tho, so I have to do it by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option<&mut T>
maybe? If I understand correctly, this Option would be deconstructed exactly three times, one in constructor, one in destructor and one in deref, so the one added jump is basically nothing. Then just take the option.
Not worth introducing unsafe code just for this IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option<&mut T>
in SocketRef
would mean DerefMut::deref_mut
could panic, and there's really no reason for it. I guess I leave it as it is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I'm significantly happier about this PR than the code you've submitted before.
examples/client.rs
Outdated
@@ -49,14 +48,14 @@ fn main() { | |||
let tcp_handle = sockets.add(tcp_socket); | |||
|
|||
{ | |||
let socket: &mut TcpSocket = sockets.get_mut(tcp_handle).as_socket(); | |||
let mut socket = sockets.get::<TcpSocket>(tcp_handle).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe dispatching over the socket type is the rare case (it only happens inside smoltcp and in OS/firmware code that's generic over the socket, e.g. fd handling), so let's have a panicking .get() and a non-panicking get_opt(), seems cleaner than unwrapping everywhere.
src/iface/ethernet.rs
Outdated
let mut device_result = Ok(()); | ||
let socket_result = | ||
match socket { | ||
&mut Socket::Raw(ref mut socket) => | ||
match *(&mut *socket) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
squint what... is this thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I've got carried away a little.
src/socket/mod.rs
Outdated
@@ -16,6 +16,7 @@ mod raw; | |||
mod udp; | |||
mod tcp; | |||
mod set; | |||
mod socket_ref; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mod ref
, use self::ref::Ref
and then reexport as a prefixed name; that's how everything else is done here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref
is a keyword, so I can't use mod ref;
, I'll rename socket_ref::SocketRef
to socket_ref::Ref
.
src/socket/socket_ref.rs
Outdated
} | ||
} | ||
|
||
impl<'a, T: SocketSession> Deref for SocketRef<'a, T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need Deref for SocketRef since all useful methods on sockets use &mut self anyway. It seems like this would lead to people thinking they can do something with &XxxSocket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DerefMut
requires Deref
.
src/socket/socket_ref.rs
Outdated
impl<'a, 'b> SocketSession for UdpSocket<'a, 'b> {} | ||
impl<'a> SocketSession for TcpSocket<'a> {} | ||
|
||
impl<'a, 'b> SocketSession for Socket<'a, 'b> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't (and can't, Rust enums are not extensible) support new socket types defined in external crates, so the entire SocketSession logic can simply be folded into the SocketRef Drop impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SocketRef
is generic over socket type, we can't put socket-specific cleanup code there. Besides, I'm going to extend SocketSession
with an associated type that holds socket's state, something like https://github.com/batonius/smoltcp/blob/packet_dispatch/src/socket/tracker.rs#L11 .
src/socket/set.rs
Outdated
pub fn iter_mut<'d>(&'d mut self) -> IterMut<'d, 'b, 'c> { | ||
IterMut { lower: self.sockets.iter_mut() } | ||
IterMut { lower: self.sockets.iter_mut(), index: 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more elegant to use .iter_mut().enumerate()
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I did initially, but std::iter::Enumerate
requires std
, so I have to do it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, I've missed core::iter
.
/// which is rather inelegant. Conversely, when `dispatch` is called, the packet length is | ||
/// not yet known and the packet storage has to be allocated; but the `&PacketRepr` is sufficient | ||
/// since the lower layers treat the packet as an opaque octet sequence. | ||
/// the [FromSocket](trait.FromSocket.html) trait, and call e.g. `UdpSocket::from_socket(socket)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth mentioning SocketSet here and that they're probably looking for socket_set.get::<TcpSocket>(tcp_handle)
instead
src/socket/mod.rs
Outdated
fn as_socket(&mut self) -> &mut T; | ||
fn try_as_socket(&mut self) -> Option<&mut T>; | ||
pub trait FromSocket<'a, 'b>: SocketSession + Sized { | ||
fn from_socket<'c>(&'c mut Socket<'a, 'b>) -> Option<&'c mut Self>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
downcast_socket
maybe? from_socket
is not really indicative of what this does.
src/socket/socket_ref.rs
Outdated
pub(crate) fn from_socket_ref(mut socket_ref: SocketRef<'a, Socket<'b, 'c>>) | ||
-> Option<SocketRef<'a, T>> { | ||
if let Some(socket) = T::from_socket(socket_ref.socket) { | ||
socket_ref.fused = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option<&mut T>
maybe? If I understand correctly, this Option would be deconstructed exactly three times, one in constructor, one in destructor and one in deref, so the one added jump is basically nothing. Then just take the option.
Not worth introducing unsafe code just for this IMO.
8502574
to
6c994c3
Compare
Updated. |
Merged manually with... lots of changes.
Please study them and take note. |
Problem: Currently
SocketSet
return sockets as&mut Socket
which makes impossible to automatically track state changes, which, in turn, makes implementing packet dispatch problematic.Solution: Implement
SocketRef
smart reference, which automatically calls bookkeeping code at the end of a usage session. ChangeSocketSet
to returnSocketRef
instead of&mut Socket
.Changes introduced by this pull request:
SocketRef
struct has been added;SocketSession
trait for socket type-specific usage session cleanup has been added;SocketSet
returnsSocketRef
instead of&mut Socket
;AsSocket
trait has been refactored intoFromSocket
;Other: Currently
SocketRef
holds only socket's handle and&mut *Socket
, and allSocketSession::finish
implementations are empty.This PR is a part of #19 effort.