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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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
Copy link
Contributor Author

@rustbot label +O-UEFI

@rustbot rustbot added the O-UEFI UEFI label Apr 11, 2025
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

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?

- 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]>
@Noratrieb
Copy link
Member

r? joboet

@rustbot rustbot assigned joboet and unassigned Noratrieb Apr 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-UEFI UEFI 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants