Skip to content

Support metadata for network north interfaces#169

Merged
jaybosamiya-ms merged 7 commits intomainfrom
copilot/fix-119
Jun 25, 2025
Merged

Support metadata for network north interfaces#169
jaybosamiya-ms merged 7 commits intomainfrom
copilot/fix-119

Conversation

Copy link

Copilot AI commented Jun 20, 2025

This PR implements metadata support for network interfaces, similar to what was done for file systems in PR #76. This addresses the discussion point from PR #116 about adding metadata capabilities to network sockets.

This finishes part of the work towards #119.

Co-authored-by: jaybosamiya-ms <171180729+jaybosamiya-ms@users.noreply.github.com>
Copilot AI changed the title [WIP] Support metadata for network north interfaces Support metadata for network north interfaces Jun 20, 2025
Copilot AI requested a review from jaybosamiya-ms June 20, 2025 21:50
Copy link
Member

@jaybosamiya-ms jaybosamiya-ms left a comment

Choose a reason for hiding this comment

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

Some changes are needed. Comments inline.

Top level comments:

  • Make sure to run cargo fmt.
  • FD-specific metadata requires coordination with #120 and #31, so should not be done as part of this PR. This also means that this does not lead to #119 being closed as done, but instead only completes a part of it. This is fine, the rest of it should be done as a separate PR anyways.

… metadata

- Remove InvalidFd from MetadataError and SetMetadataError
- Remove fd_metadata field from SocketHandle
- Remove set_fd_metadata method entirely
- Update metadata methods to only work with socket_metadata
- Remove test_fd_metadata and test_metadata_priority tests
- Add TODO comments linking to #120 and #31 for future FD-specific metadata
- Keep SetMetadataError as non-exhaustive but empty enum with phantom type

Co-authored-by: jaybosamiya-ms <171180729+jaybosamiya-ms@users.noreply.github.com>

This comment was marked as resolved.

Copy link
Member

@jaybosamiya-ms jaybosamiya-ms left a comment

Choose a reason for hiding this comment

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

Thanks, mostly looks good, minor suggestions. Please also make sure to run cargo fmt on the workspace, thanks!

Co-authored-by: jaybosamiya-ms <171180729+jaybosamiya-ms@users.noreply.github.com>

This comment was marked as resolved.

Copy link
Member

@jaybosamiya-ms jaybosamiya-ms left a comment

Choose a reason for hiding this comment

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

This PR should have also been largely straightforward for Copilot to do, but seemed to require a little more hand-holding. Part of it is that I mildly sent it down a tiny tangent, which I asked it to then revert, but the revert did not work out as well as I'd have liked.

Anyways, I've now manually fixed up the code needed for it. I don't know if I can say that this one in particular saved me enough time, but at least I think I now understand the scale of things that I can/should send over to Copilot coding agent, and which ones I should not.

@jaybosamiya-ms jaybosamiya-ms marked this pull request as ready for review June 21, 2025 01:44
@jaybosamiya-ms jaybosamiya-ms requested a review from CvvT June 21, 2025 01:44
@jaybosamiya-ms
Copy link
Member

NOTE: The breaking changes from this (i.e. Network no longer being Send/Sync/UnwindSafe) is an unfortunate consequence of us (currently) storing the handles in a Vec, and should be fixed up as part of #31 to bring back Send/Sync to Network, because otherwise, the places it can be used gets somewhat throttled.

Copy link
Contributor

@CvvT CvvT left a comment

Choose a reason for hiding this comment

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

This one looks like taking more manual efforts to make it right. Thanks Jay!

@github-actions
Copy link

🤖 SemverChecks 🤖 ⚠️ Potential breaking API changes detected ⚠️

Click for details
--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type Network is no longer Send, in /home/runner/work/litebox/litebox/litebox/src/net/mod.rs:57
  type Network is no longer Sync, in /home/runner/work/litebox/litebox/litebox/src/net/mod.rs:57
  type Network is no longer RefUnwindSafe, in /home/runner/work/litebox/litebox/litebox/src/net/mod.rs:57

@jaybosamiya-ms jaybosamiya-ms merged commit 1bd4f28 into main Jun 25, 2025
6 checks passed
@jaybosamiya-ms jaybosamiya-ms deleted the copilot/fix-119 branch June 25, 2025 23:06
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.

3 participants