Skip to content

std: sys: net: uefi: Implement TCP4 connect #139254

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 24, 2025
Merged

std: sys: net: uefi: Implement TCP4 connect #139254

merged 1 commit into from
May 24, 2025

Conversation

Ayush1325
Copy link
Contributor

  • Implement TCP4 connect using EFI_TCP4_PROTOCOL.
  • Tested on QEMU setup with connecting to TCP server on host.

@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 2, 2025
@Ayush1325
Copy link
Contributor Author

cc @nicholasbishop

@Ayush1325 Ayush1325 mentioned this pull request Apr 2, 2025
Open
@Ayush1325 Ayush1325 force-pushed the uefi-tcp4-connect branch from c00e658 to 0f3036c Compare April 8, 2025 06:28
@Ayush1325
Copy link
Contributor Author

@rustbot label +O-UEFI

@rustbot rustbot added the O-UEFI UEFI label Apr 11, 2025
joboet
joboet reviewed Apr 13, 2025
View reviewed changes
library/std/src/sys/net/connection/uefi/tcp4.rs
efi::EVT_NOTIFY_SIGNAL,
efi::TPL_CALLBACK,
Some(toggle_atomic_flag),
Some(unsafe { NonNull::new_unchecked(self.flag.as_ptr().cast()) }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes wait_for_flag unsound – there is no guarantee that the structure will not be moved in the time between the calls, which would invalidate this pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I use Condvar then? Or is there a way to have Atomic on Heap or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the flag, make the notification function a no-op and use CheckEvent on the event.

Copy link
Contributor Author

@Ayush1325 Ayush1325 Apr 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event is of type Signal. The CheckEvent doc states the following

If Event is of type EVT_NOTIFY_SIGNAL, then EFI_INVALID_PARAMETER is returned.

The reason the event is of type Signal is because the tcp4 protocol connect doc states the following

The Event to signal after request is finished and Status field is updated by the EFI TCPv4 Protocol driver. The
type of Event must be EVT_NOTIFY_SIGNAL, and its Task Priority Level (TPL) must be lower than or equal to TPL_CALLBACK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that. Then the easiest way is probably just to ensure that the flag outlives the event. Why not just inline wait_for_flag and make flag a local variable? Alternatively, make create_evt unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that. Then the easiest way is probably just to ensure that the flag outlives the event. Why not just inline wait_for_flag and make flag a local variable? Alternatively, make create_evt unsafe.

I would prefer not to inline wait_for_flag since that will be repeated in almost all other implementations soon enough (read, write, close, etc). I have made create_evt unsafe. Is that ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the cleanest solution ever™, but I guess it's fine. In the future it might be nice to create a single global event for all "blocking" I/O calls in std.

@Noratrieb
Copy link
Member

r? joboet

@rustbot rustbot assigned joboet and unassigned Noratrieb Apr 20, 2025
joboet
joboet reviewed May 23, 2025
View reviewed changes
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not getting back to you for so long. This looks mostly fine to me.

library/std/src/sys/net/connection/uefi/tcp4.rs
efi::EVT_NOTIFY_SIGNAL,
efi::TPL_CALLBACK,
Some(toggle_atomic_flag),
Some(unsafe { NonNull::new_unchecked(self.flag.as_ptr().cast()) }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the cleanest solution ever™, but I guess it's fine. In the future it might be nice to create a single global event for all "blocking" I/O calls in std.

@joboet joboet added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2025
@Ayush1325 Ayush1325 force-pushed the uefi-tcp4-connect branch from a1e459d to 3d0d7c6 Compare May 23, 2025 17:13
joboet
joboet requested changes May 23, 2025
View reviewed changes
@Ayush1325
std: sys: net: uefi: Implement TCP4 connect
e21aab5 
- Implement TCP4 connect using EFI_TCP4_PROTOCOL.
- Tested on QEMU setup with connecting to TCP server on host.

Signed-off-by: Ayush Singh <[email protected]>
@Ayush1325 Ayush1325 force-pushed the uefi-tcp4-connect branch from 3d0d7c6 to e21aab5 Compare May 23, 2025 20:32
@Ayush1325
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 24, 2025
@joboet
Copy link
Member

joboet commented May 24, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented May 24, 2025

📌 Commit e21aab5 has been approved by joboet

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request May 24, 2025
Merged
bors added a commit that referenced this pull request May 24, 2025
@bors
Auto merge of #141506 - matthiaskrgr:rollup-80f1geg, r=matthiaskrgr
038d599 
Rollup of 6 pull requests

Successful merges:

 - #137323 (Guarantee behavior of transmuting `Option::<T>::None` subject to NPO)
 - #139254 (std: sys: net: uefi: Implement TCP4 connect)
 - #141432 (refactor `CanonicalVarKind`)
 - #141480 (document some -Z flags as living in the rustc-dev-guide)
 - #141486 (rustdoc book: add argument explanation for `html_playground_url`)
 - #141496 (Enable `[issue-links]` and `[no-mentions]` in triagebot)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d292040 into rust-lang:master May 24, 2025
6 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 24, 2025
rust-timer added a commit that referenced this pull request May 24, 2025
@rust-timer
Unrolled build for #139254
0bd126c 
Rollup merge of #139254 - Ayush1325:uefi-tcp4-connect, r=joboet

std: sys: net: uefi: Implement TCP4 connect

- Implement TCP4 connect using EFI_TCP4_PROTOCOL.
- Tested on QEMU setup with connecting to TCP server on host.
@Ayush1325 Ayush1325 deleted the uefi-tcp4-connect branch May 24, 2025 18:31
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request May 26, 2025
@matthiaskrgr
Rollup merge of rust-lang#139254 - Ayush1325:uefi-tcp4-connect, r=joboet
e2b0757 
std: sys: net: uefi: Implement TCP4 connect

- Implement TCP4 connect using EFI_TCP4_PROTOCOL.
- Tested on QEMU setup with connecting to TCP server on host.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joboet joboet joboet requested changes

Assignees

@joboet joboet

Labels
O-UEFI UEFI S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.89.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Ayush1325 @rustbot @Noratrieb @joboet @bors