-
Notifications
You must be signed in to change notification settings - Fork 13.7k
std: sys: net: uefi: tcp: Initial TcpListener support #145339
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
base: master
Are you sure you want to change the base?
Conversation
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
Passing this along to someone else, who hopefully has more context. r? libs |
@rustbot label +O-UEFI |
Thom is off the review rotation Also gentle ping @nicholasbishop for a review since we usually defer UEFI to you |
temp.configure(false, None, Some(x))?; | ||
Ok(Tcp::V4(temp)) | ||
} | ||
SocketAddr::V6(_) => todo!(), |
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.
Rather than todo!()
, maybe add an IPV6_UNSUPPORTED
error to
rust/library/std/src/io/error.rs
Line 78 in b3cfb8f
impl Error { |
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 think it would be better to return normal unsupported instead of adding a new error variant. Since IPv6 is just not implemented by std right now. It is supported by UEFI. And I am planning to implement it once IPv4 is complete.
} else { | ||
(DEFAULT_ADDR, 0) | ||
}; | ||
let subnet_mask = helpers::ipv4_to_r_efi(crate::net::Ipv4Addr::new(255, 255, 255, 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.
Is this a breaking change? If the mask was previously 0.0.0.0 (all addresses), it seems like this may surprise some users
Worth a comment in any case
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.
No, this is not a breaking change. That is because the docs state the following: Not used when UseDefaultAddress is TRUE.
And previously, UseDefaultAddress
was always true.
// The spec does not seem to state if we need to call ServiceBinding->DestroyChild for | ||
// this handle |
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.
Is there a reference implementation that does or doesn't do this? Or a way to check whether or not the handle is valid after this call?
@@ -15,7 +15,7 @@ pub(crate) struct Tcp4 { | |||
protocol: NonNull<tcp4::Protocol>, | |||
flag: AtomicBool, | |||
#[expect(dead_code)] |
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.
Does this lint still fire?
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.
Yes, just tested
54d3d51
to
5459fc4
Compare
☔ The latest upstream changes (presumably #146418) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
Generally LGTM but left a few remarks
} | ||
} | ||
|
||
pub fn connect(addr: io::Result<&SocketAddr>) -> io::Result<TcpStream> { |
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.
Why are you passing in a result here if you to addr?
in a next step? I never saw this in Rust, seems to be an antipattern?
PS: Still not familiar with the development model and best practices within Rust std itself.
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.
Well, that was the internal API till now due to SGX target. It has been changed now in the following PR: #145327 so I have updated things accordingly.
let use_default_address = efi::Boolean::TRUE; | ||
let (station_address, station_port) = (DEFAULT_ADDR, 0); | ||
let subnet_mask = helpers::ipv4_to_r_efi(crate::net::Ipv4Addr::new(0, 0, 0, 0)); | ||
let use_default_address = station_address.is_none().into(); |
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 makes .into()
code easier to read if one is not using an IDE
let use_default_address = station_address.is_none().into(); | |
let use_default_address: bool = station_address.is_none().into(); |
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 have added the annotation, but it is not actually bool
, but rather r_efi::efi::Boolean
.
} else { | ||
(DEFAULT_ADDR, 0) | ||
}; | ||
let subnet_mask = helpers::ipv4_to_r_efi(crate::net::Ipv4Addr::new(255, 255, 255, 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.
This is more readable
let subnet_mask = helpers::ipv4_to_r_efi(crate::net::Ipv4Addr::new(255, 255, 255, 0)); | |
let subnet_mask = crate::net::Ipv4Addr::new(255, 255, 255, 0); | |
let subnet_mask = helpers::ipv4_to_r_efi(subnet_mask); |
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.
Added
Add support for binding and accepting TCP4 connections. While testing, the following network options were used with QEMU + OVMF: -nic user,hostfwd=tcp::12345-:12345 The default localhost address on qemu seems to be 10.0.2.15. UEFI spec does not seem to state that the TCP Handle returned by the Accept method has a ServiceBinding Protocol. So have made the ServiceBinding Protocol optional. Signed-off-by: Ayush Singh <[email protected]>
5459fc4
to
9f020a9
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Add support for binding and accepting TCP4 connections.
While testing, the following network options were used with QEMU + OVMF: -nic user,hostfwd=tcp::12345-:12345
The default localhost address on qemu seems to be 10.0.2.15.
UEFI spec does not seem to state that the TCP Handle returned by the Accept method has a ServiceBinding Protocol. So have made the ServiceBinding Protocol optional.
cc @nicholasbishop