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

vsock: Use PathBuf for socket paths instead of Strings #446

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bilelmoussaoui
Copy link
Contributor

Summary of the PR

For reasons I ignore, String was used all over the place where a PathBuf makes more sense.

cc @Ablu, similar to yesterday's discussion regarding vhost-video

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@bilelmoussaoui
Copy link
Contributor Author

Before I do the same for the rest of the crates, is there any reason behind such pattern?

Copy link
Contributor

@Ablu Ablu left a comment

Choose a reason for hiding this comment

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

Before I do the same for the rest of the crates, is there any reason behind such pattern?

I am all for PathBuf/Path. Makes the type clear and provides all the useful path manipulation functions.

crates/vsock/src/main.rs Outdated Show resolved Hide resolved
@stefano-garzarella
Copy link
Member

Before I do the same for the rest of the crates, is there any reason behind such pattern?

I don't think so, thanks for fixing that.

@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui/vsock-path branch 2 times, most recently from 781a611 to 765c925 Compare September 15, 2023 08:48
crates/i2c/src/main.rs Outdated Show resolved Hide resolved
crates/vsock/src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Ablu Ablu left a comment

Choose a reason for hiding this comment

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

Found a few cases where we turn an existing (and unneeded PathBuf) into a ref only to turn it back into a PathBuf. But maybe thats just against my own taste, so I do not see it as blocker.

crates/i2c/src/main.rs Outdated Show resolved Hide resolved
crates/vsock/src/thread_backend.rs Outdated Show resolved Hide resolved
crates/vsock/src/vhu_vsock.rs Outdated Show resolved Hide resolved
crates/vsock/src/vhu_vsock_thread.rs Outdated Show resolved Hide resolved
@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui/vsock-path branch 2 times, most recently from 4edc548 to 830543b Compare September 15, 2023 09:26
@stefano-garzarella
Copy link
Member

@bilelmoussaoui I'd suggest updating the PR title, since we are now covering multiple crates

@bilelmoussaoui
Copy link
Contributor Author

@bilelmoussaoui I'd suggest updating the PR title, since we are now covering multiple crates

Right, will do once I figured out how to handle the tests situation & migrate scmi to use clap directly.

@epilys
Copy link
Member

epilys commented Oct 18, 2023

This change is definitely necessary. Path names are not guaranteed to be UTF-8 hence why Path/OsStr exists.

@Ablu Ablu mentioned this pull request Nov 30, 2023
4 tasks
crates/gpio/src/backend.rs Outdated Show resolved Hide resolved
@Ablu Ablu mentioned this pull request Dec 5, 2023
4 tasks
@epilys
Copy link
Member

epilys commented Dec 5, 2023

@bilelmoussaoui are you interested in picking this up again? We started doing this on the other device crates (see latest PRs)

@bilelmoussaoui
Copy link
Contributor Author

@bilelmoussaoui are you interested in picking this up again? We started doing this on the other device crates (see latest PRs)

it took me too long, but i finally did!

@bilelmoussaoui
Copy link
Contributor Author

Although a test is failing with the following

thread 'backend::tests::test_gpio_fail_listener_mock' panicked at vhost-device-gpio/src/backend.rs:350:9:
assertion failed: `CouldNotOpenDevice(GpiodFailed(OperationFailed(ChipOpen, Errno { code: 2, description: Some("No such file or directory") })))` does not match `Error::ServeFailed(_)`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I suppose that is kind of expected, could I change the expected error in the test?

@@ -196,7 +197,7 @@ fn start_backend(args: GpioArgs) -> Result<()> {
let mut handles = Vec::new();

for i in 0..config.socket_count {
let socket = config.socket_path.to_owned() + &i.to_string();
let socket = config.socket_path.join(i.to_string());
Copy link
Member

Choose a reason for hiding this comment

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

Path::join does not append the suffix to the file name component, but pushes a new component (e.g. "foo.sock0" vs "foo.sock/0")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess something like let socket = config.socket_path.with_extension(format!("sock{i}")); would work better? but that hardcodes the extension though.

Copy link
Member

Choose a reason for hiding this comment

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

pub fn generate_socket_paths(&self) -> Vec<PathBuf> {
let socket_file_name = self
.socket_path
.file_name()
.expect("socket_path has no filename.");
let socket_file_parent = self
.socket_path
.parent()
.expect("socket_path has no parent directory.");
let make_socket_path = |i: usize| -> PathBuf {
let mut file_name = socket_file_name.to_os_string();
file_name.push(std::ffi::OsStr::new(&i.to_string()));
socket_file_parent.join(&file_name)
};
(0..self.socket_count).map(make_socket_path).collect()
}
}

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.

7 participants