Skip to content
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

uhyve-interface: Don't take ownership on fn port() #757

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Conversation

jounathaen
Copy link
Member

It was almost impossible to use this fn before, as it takes the ownership of the whole struct, making access to the data impossible.
This was a mistake in the first place and should now be resolved.

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 67.24%. Comparing base (f407069) to head (88112da).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
uhyve-interface/src/lib.rs 0.00% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #757      +/-   ##
==========================================
- Coverage   67.45%   67.24%   -0.21%     
==========================================
  Files          20       18       -2     
  Lines        2338     2345       +7     
==========================================
  Hits         1577     1577              
- Misses        761      768       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Why not impl From<&Hypercall<'_>> for HypercallAddress and remove impl From<Hypercall<'_>> for HypercallAddress and use that impl From instead of repeating the match?

@jounathaen
Copy link
Member Author

My thought was, that from implies consuming the value. I don't want to consume the Hypercall (that makes it impossible to use) and I'll definitely remove that in V2 of the interface.
You can argue wether the reference is consumed with From<&Hypercall> and I'd be fine with that as well, but I think throwing out both in the end is the better approach.

@mkroening
Copy link
Member

Using From to consume a reference is totally fine. See std's impl From<&str> for String for example. I think having impl From<&Hypercall<'_>> for HypercallAddress would be the idiomatic way of doing this.

Another point: this is technically a semver break. If this is never used anywhere, fine, I guess, but technically, we should bump the minor version.

@jounathaen
Copy link
Member Author

Changed it to use From.

From the Semver documentation:

PATCH version when you make backward compatible bug fixes

I think this can be considered a bugfix 😉. It is definitely not used anywhere, but we can also increase the minor version.

@mkroening
Copy link
Member

My point is that removing Hypercall::port is not backwards compatible. I don't have a strong opinion on this, I just wanted to make sure that decision is made consciously.

@jounathaen
Copy link
Member Author

Ah, that was a mistake. Thanks for catching this.

@jounathaen jounathaen added this pull request to the merge queue Sep 23, 2024
Merged via the queue into main with commit 5dea30b Sep 23, 2024
10 checks passed
@jounathaen jounathaen deleted the uhvye_if_1-2 branch September 23, 2024 16:05
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