-
Notifications
You must be signed in to change notification settings - Fork 49
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
base: main
Are you sure you want to change the base?
vsock: Use PathBuf for socket paths instead of Strings #446
Conversation
Before I do the same for the rest of the crates, is there any reason behind such pattern? |
There was a problem hiding this 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.
I don't think so, thanks for fixing that. |
781a611
to
765c925
Compare
There was a problem hiding this 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.
4edc548
to
830543b
Compare
830543b
to
67bf380
Compare
@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. |
This change is definitely necessary. Path names are not guaranteed to be UTF-8 hence why Path/OsStr exists. |
@bilelmoussaoui are you interested in picking this up again? We started doing this on the other device crates (see latest PRs) |
67bf380
to
b565398
Compare
Signed-off-by: Bilal Elmoussaoui <[email protected]>
Signed-off-by: Bilal Elmoussaoui <[email protected]>
Signed-off-by: Bilal Elmoussaoui <[email protected]>
Signed-off-by: Bilal Elmoussaoui <[email protected]>
Signed-off-by: Bilal Elmoussaoui <[email protected]>
b565398
to
04a69be
Compare
it took me too long, but i finally did! |
Although a test is failing with the following
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()); |
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vhost-device/vhost-device-i2c/src/main.rs
Lines 239 to 256 in 1e6667f
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() | |
} | |
} |
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:
git commit -s
), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.