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.

Sign up for GitHub

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

Jump to bottom

Use Cow<'static, str> for VhostUserDaemon name field. #196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use Cow<'static, str> for VhostUserDaemon name field. #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/vhost-user-backend/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
- [[#192]](https://github.com/rust-vmm/vhost/pull/192) vhost-user-backend: remove return value from handle_event
- [[#155]](https://github.com/rust-vmm/vhost/pull/155) Converted generic type
parameters of VhostUserBackend into associated types.
- `name` field of VhostUserDaemon is now a `std::borrow::Cow<'static, str>`
instead of `String` to prevent allocating in the heap for static names, which
is the majority of uses of this API.

### Fixed

Expand Down
2 changes: 1 addition & 1 deletion crates/vhost-user-backend/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ where
V: VringT<GM<B>> + Clone + Send + Sync + 'static,
B: Bitmap + 'static,
{
pub fn new(name: String, backend: S, atomic_mem: GuestMemoryAtomic<GuestMemoryMmap<B>>) -> Result<Self>;
pub fn new(name: Cow<'static, str>, backend: S, atomic_mem: GuestMemoryAtomic<GuestMemoryMmap<B>>) -> Result<Self>;
pub fn start(&mut self, listener: Listener) -> Result<()>;
pub fn wait(&mut self) -> Result<()>;
pub fn get_epoll_handlers(&self) -> Vec<Arc<VringEpollHandler<S, V, B>>>;
Expand Down
13 changes: 7 additions & 6 deletions crates/vhost-user-backend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#[macro_use]
extern crate log;

use std::borrow::Cow;
use std::fmt::{Display, Formatter};
use std::path::Path;
use std::sync::{Arc, Mutex};
Expand Down Expand Up @@ -81,7 +82,7 @@ pub type Result<T> = std::result::Result<T, Error>;
/// This structure is the public API the backend is allowed to interact with in order to run
/// a fully functional vhost-user daemon.
pub struct VhostUserDaemon<T: VhostUserBackend> {
name: String,
name: Cow<'static, str>,
handler: Arc<Mutex<VhostUserHandler<T>>>,
main_thread: Option<thread::JoinHandle<Result<()>>>,
}
Expand All @@ -98,7 +99,7 @@ where
/// registered event. Those events can be vring events or custom events from the backend,
/// but they get to be registered later during the sequence.
pub fn new(
name: String,
name: Cow<'static, str>,
backend: T,
atomic_mem: GuestMemoryAtomic<GuestMemoryMmap<T::Bitmap>>,
) -> Result<Self> {
Expand All @@ -124,7 +125,7 @@ where
mut handler: BackendReqHandler<Mutex<VhostUserHandler<T>>>,
) -> Result<()> {
let handle = thread::Builder::new()
.name(self.name.clone())
.name(self.name.to_string())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... This is the only use and requires ownership. So should we maybe just drop it from the daemon struct and make it part of the start_daemon function (as a String that can be moved through)?

Though, overall, I think this is a lot of code churn (for everyone using this) and additional complexity for very little gain.

.spawn(move || loop {
handler.handle_request().map_err(Error::HandleRequest)?;
})
Expand Down Expand Up @@ -253,7 +254,7 @@ mod tests {
GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0x100000), 0x10000)]).unwrap(),
);
let backend = Arc::new(Mutex::new(MockVhostBackend::new()));
let mut daemon = VhostUserDaemon::new("test".to_owned(), backend, mem).unwrap();
let mut daemon = VhostUserDaemon::new("test".into(), backend, mem).unwrap();

let handlers = daemon.get_epoll_handlers();
assert_eq!(handlers.len(), 2);
Expand Down Expand Up @@ -286,7 +287,7 @@ mod tests {
GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0x100000), 0x10000)]).unwrap(),
);
let backend = Arc::new(Mutex::new(MockVhostBackend::new()));
let mut daemon = VhostUserDaemon::new("test".to_owned(), backend, mem).unwrap();
let mut daemon = VhostUserDaemon::new("test".into(), backend, mem).unwrap();

let handlers = daemon.get_epoll_handlers();
assert_eq!(handlers.len(), 2);
Expand Down Expand Up @@ -321,7 +322,7 @@ mod tests {
GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0x100000), 0x10000)]).unwrap(),
);
let backend = Arc::new(Mutex::new(MockVhostBackend::new()));
let mut daemon = VhostUserDaemon::new("test".to_owned(), backend.clone(), mem).unwrap();
let mut daemon = VhostUserDaemon::new("test".into(), backend.clone(), mem).unwrap();
let tmpdir = tempfile::tempdir().unwrap();
let socket_path = tmpdir.path().join("socket");

Expand Down
2 changes: 1 addition & 1 deletion crates/vhost-user-backend/tests/vhost-user-server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ fn vhost_user_client(path: &Path, barrier: Arc<Barrier>) {
fn vhost_user_server(cb: fn(&Path, Arc<Barrier>)) {
let mem = GuestMemoryAtomic::new(GuestMemoryMmap::<()>::new());
let backend = Arc::new(Mutex::new(MockVhostBackend::new()));
let mut daemon = VhostUserDaemon::new("test".to_owned(), backend, mem).unwrap();
let mut daemon = VhostUserDaemon::new("test".into(), backend, mem).unwrap();

let barrier = Arc::new(Barrier::new(2));
let tmpdir = tempfile::tempdir().unwrap();
Expand Down