Skip to content

Conversation

@JayKickliter
Copy link

@JayKickliter JayKickliter commented Oct 21, 2025

This is PR 2 of 2 adding TCP to uefi-rs, and is a replacement for #1779. Note that this PR contains the commits from #1797 and will need to rebased before merging.

TODO before merging

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@phip1611 phip1611 marked this pull request as draft October 22, 2025 05:05
@phip1611
Copy link
Member

typically we mark PRs as draft when they wait to be rebased onto another PR - so I marked this PR as draft.

Thanks for working on this and splitting this into multiple PRs!

@JayKickliter JayKickliter force-pushed the jsk/uefi/add-tcpv4 branch 4 times, most recently from ebf20de to dee28f7 Compare October 22, 2025 18:15
@JayKickliter JayKickliter force-pushed the jsk/uefi/add-tcpv4 branch 2 times, most recently from 05b5155 to f547536 Compare November 11, 2025 18:30

impl Tcp4 {
/// See [Tcp4Protocol::configure].
pub fn configure(&mut self, config: &ConfigData, options: Option<&ConfigOptions>) -> Result {
Copy link
Author

Choose a reason for hiding this comment

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

I just realize this fn duplicates a lot of Ip4Config2::ifup() . Should I just make Tcp4 take a NIC Handle that's already been configured like Http does?

Copy link
Member

@phip1611 phip1611 Nov 19, 2025

Choose a reason for hiding this comment

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

makes sense. in uefi, we prefer to hide lower-level details of UEFI from public API. Convenient high-level abstractions are very appreciated

@JayKickliter JayKickliter force-pushed the jsk/uefi/add-tcpv4 branch 3 times, most recently from 6e51b3d to 327b902 Compare November 12, 2025 18:23
@phip1611
Copy link
Member

#1797 is merged, please rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants