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

Port passthrough #135

Merged
merged 2 commits into from
Jan 31, 2025
Merged

Conversation

WhatAmISupposedToPutHere
Copy link
Collaborator

@WhatAmISupposedToPutHere WhatAmISupposedToPutHere commented Dec 31, 2024

Add an ability to configure the set of published ports via command line switches.

This is a continuation from #134 that moves all internal communications to vsock ports.

@teohhanhui
Copy link
Collaborator

Does this subsume #134?

@WhatAmISupposedToPutHere
Copy link
Collaborator Author

Not really. Those are prs for different features, that just happen to be dependent on each other.

This will allow implementing other network topologies.

Signed-off-by: Sasha Finkelstein <[email protected]>
@slp slp merged commit 49a0346 into AsahiLinux:main Jan 31, 2025
2 checks passed
.to_str()
.expect("server_path should not contain invalid UTF-8"),
)
.context("Failed to process `muvm-guest` path as it contains NUL characters")?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing this is a copy and paste error, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah... unlikely to ever be hit, but should probably fix that one

Copy link
Collaborator

@teohhanhui teohhanhui Jan 31, 2025

Choose a reason for hiding this comment

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

The POSIX spec says:

The value of an environment variable is an arbitrary sequence of bytes, except for the null byte.

https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap08.html

So I think we can just change this to expect everywhere, e.g.

Suggested change
.context("Failed to process `muvm-guest` path as it contains NUL characters")?;
.expect("server_path should not contain NUL character");

(I can do this after the rest is merged, to not create more work for you...)

let mut stream =
TcpStream::connect(format!("127.0.0.1:{server_port}")).map_err(LaunchError::Connection)?;
let run_path = env::var("XDG_RUNTIME_DIR")
.map_err(|e| anyhow!("unable to get XDG_RUNTIME_DIR: {:?}", e))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As done elsewhere...

Suggested change
.map_err(|e| anyhow!("unable to get XDG_RUNTIME_DIR: {:?}", e))?;
.context("Failed to read XDG_RUNTIME_DIR environment variable")?;

This would already wrap the internal error.

(I can do this after the rest is merged, to not create more work for you...)

Comment on lines +62 to +74
let optslash = if self.ip.is_empty() { "" } else { "/" };
[
if self.udp { "-u" } else { "-t" }.to_owned(),
format!(
"{}{}{}-{}:{}-{}",
self.ip,
optslash,
self.host_range.0,
self.host_range.1,
self.guest_range.0,
self.guest_range.1
),
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self:

Suggested change
let optslash = if self.ip.is_empty() { "" } else { "/" };
[
if self.udp { "-u" } else { "-t" }.to_owned(),
format!(
"{}{}{}-{}:{}-{}",
self.ip,
optslash,
self.host_range.0,
self.host_range.1,
self.guest_range.0,
self.guest_range.1
),
]
[
if self.udp { "-u" } else { "-t" }.to_owned(),
format!(
"{ip}{slash}{host_start}-{host_end}:{guest_start}-{guest_end}",
ip = self.ip,
slash = if self.ip.is_empty() { "" } else { "/" },
host_start = self.host_range.0,
host_end = self.host_range.1,
guest_start = self.guest_range.0,
guest_end = self.guest_range.1
),
]

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