Skip to content

vhost-device-can: use the same socket for send/recv #937

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

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

vhost-device-can: use the same socket for send/recv #937

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6c94dae
vhost-device-can: use the same socket for send/recv
MatiasVara Feb 18, 2026
468b098
Use can0 interface in all tests
MatiasVara Mar 3, 2026
09a1ec2
Set CAN_FRMF_TYPE_FD flag
MatiasVara Mar 3, 2026
3be95bc
Rename test_virtio_can_tx_device_started_test_send_fail()
MatiasVara Mar 3, 2026
2abb694
Add mock socket for tests
MatiasVara Mar 10, 2026
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
15 changes: 4 additions & 11 deletions vhost-device-can/src/backend.rs
View file
Open in desktop

@stefano-garzarella stefano-garzarella Apr 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

About commit 6c94dae can we improve better the commit description?

What I don't get is why having 1 socket fixes the issue we had (I don't know much about can sockets, etc.).

Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::{
thread,
};

use log::{error, info, warn};
use log::{info, warn};
use thiserror::Error as ThisError;
use vhost_user_backend::VhostUserDaemon;
use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap};
Expand All @@ -31,8 +31,6 @@ pub(crate) enum Error {
CouldNotFindCANDevs,
#[error("Could not create can controller: {0}")]
CouldNotCreateCanController(crate::can::Error),
#[error("Could not create can controller output socket: {0}")]
FailCreateCanControllerSocket(crate::can::Error),
#[error("Could not create can backend: {0}")]
CouldNotCreateBackend(crate::vhu_can::Error),
#[error("Could not create daemon: {0}")]
Expand Down Expand Up @@ -82,11 +80,6 @@ pub(crate) fn start_backend_server(socket: PathBuf, can_devs: String) -> Result<
VhostUserCanBackend::new(lockable_controller.clone())
.map_err(Error::CouldNotCreateBackend)?,
));
lockable_controller
.write()
.unwrap()
.open_can_socket()
.map_err(Error::FailCreateCanControllerSocket)?;

let read_handle = CanController::start_read_thread(lockable_controller.clone());

Expand Down Expand Up @@ -165,7 +158,7 @@ mod tests {
use assert_matches::assert_matches;

use super::*;
use crate::{backend::Error::FailCreateCanControllerSocket, can::Error::SocketOpen, CanArgs};
use crate::{backend::Error::CouldNotCreateCanController, can::Error::SocketOpen, CanArgs};

#[test]
fn test_can_valid_configuration() {
Expand Down Expand Up @@ -248,7 +241,7 @@ mod tests {

assert_matches!(
start_backend(config),
Err(FailCreateCanControllerSocket(SocketOpen))
Err(CouldNotCreateCanController(SocketOpen))
);
}

Expand All @@ -259,7 +252,7 @@ mod tests {

assert_matches!(
start_backend_server(socket_path, can_devs),
Err(FailCreateCanControllerSocket(SocketOpen))
Err(CouldNotCreateCanController(SocketOpen))
);
}
}
117 changes: 38 additions & 79 deletions vhost-device-can/src/can.rs
View file
Open in desktop
Comment thread
MatiasVara marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::{
thread::{spawn, JoinHandle},
};

use log::{error, info, trace, warn};
use log::{info, trace, warn};
use thiserror::Error as ThisError;
use vmm_sys_util::eventfd::{EventFd, EFD_NONBLOCK};
extern crate queues;
Expand Down Expand Up @@ -45,15 +45,15 @@ pub(crate) enum Error {
EventFdFailed,
#[error("Push can element operation failed")]
PushFailed,
#[error("No output interface available")]
NoOutputInterface,
#[error("Can socket configuration failed")]
SocketConfig,
}

#[derive(Debug)]
pub(crate) struct CanController {
pub config: VirtioCanConfig,
pub can_name: String,
pub can_socket: Option<CanFdSocket>,
pub can_socket: CanFdSocket,
pub rx_event_fd: EventFd,
rx_fifo: Queue<VirtioCanFrame>,
pub status: bool,
Expand All @@ -69,10 +69,22 @@ impl CanController {
let rx_fifo = Queue::new();
let rx_efd = EventFd::new(EFD_NONBLOCK).map_err(|_| Error::EventFdFailed)?;

let socket: CanFdSocket = match CanFdSocket::open(can_name.as_str()) {
Ok(socket) => socket,
Err(_) => {
warn!("Error opening CAN socket");
return Err(Error::SocketOpen);
}
};

socket
.set_nonblocking(true)
.map_err(|_| Error::SocketConfig)?;

Ok(CanController {
config: VirtioCanConfig { status: 0x0.into() },
can_name,
can_socket: None,
can_socket: socket,
rx_event_fd: rx_efd,
rx_fifo,
status: true,
Expand Down Expand Up @@ -127,17 +139,6 @@ impl CanController {
}
}

pub fn open_can_socket(&mut self) -> Result<()> {
self.can_socket = match CanFdSocket::open(&self.can_name) {
Ok(socket) => Some(socket),
Err(_) => {
warn!("Error opening CAN socket");
return Err(Error::SocketOpen);
}
};
Ok(())
}

// Helper function to process frame
fn process_frame<F: Frame>(frame: F, is_fd: bool) -> VirtioCanFrame {
VirtioCanFrame {
Expand All @@ -159,25 +160,6 @@ impl CanController {
}

pub fn read_can_socket(controller: Arc<RwLock<CanController>>) -> Result<()> {
let can_name = &controller.read().unwrap().can_name.clone();
log::debug!("Start reading from {can_name} socket!");
let socket = match CanFdSocket::open(can_name) {
Ok(socket) => socket,
Err(_) => {
warn!("Error opening CAN socket");
return Err(Error::SocketOpen);
}
};

// Set non-blocking otherwise the device will not restart immediately
// when the VM closes, and a new canfd message needs to be received for
// restart to happen.
// This caused by the fact that the thread is stacked in read function
// and does not go to the next loop to check the status condition.
socket
.set_nonblocking(true)
.expect("Cannot set nonblocking");

// Receive CAN messages
loop {
// If the status variable is false then break and exit.
Expand All @@ -186,7 +168,12 @@ impl CanController {
return Ok(());
}

if let Ok(frame) = socket.read_frame() {
let frame_result = {
let ctrl = controller.read().unwrap();
Comment thread
stefano-garzarella marked this conversation as resolved.
ctrl.can_socket.read_frame()
};

if let Ok(frame) = frame_result {

@stefano-garzarella stefano-garzarella Apr 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happens here if there is no frame returned by read_frame() ?
Are we spinning acquiring now the shared lock?

Maybe this is fine, since we are releasing it in the loop, but I'd like just to understand better if it's expected.

@MatiasVara MatiasVara Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did not get the question.

@stefano-garzarella stefano-garzarella Apr 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mean, if read_frame() doesn't return any frame, here we loop calling read_frame() in a loop, since IIUC the socket is no-blocking.

Is that okay?

@MatiasVara MatiasVara Apr 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if that's OK. It seems working, i.e., I did not observe starvation in the other thread that get a write lock.

// If ctrl_state is stopped, consume the received CAN/FD frame
// and loop till the ctrl_state changes to started or the thread
// to exit.
Expand Down Expand Up @@ -272,17 +259,12 @@ impl CanController {
};

// Send the CAN/FD frame
let socket = self.can_socket.as_ref().ok_or("No available device");

match socket {
Ok(socket) => match socket.write_frame(&frame) {
Ok(_) => Ok(()),
Err(_) => {
warn!("Error write CAN socket");
Err(Error::SocketWrite)
}
},
Err(_) => Err(Error::NoOutputInterface),
match self.can_socket.write_frame(&frame) {
Ok(_) => Ok(()),
Err(_) => {
warn!("Error write CAN socket");
Err(Error::SocketWrite)
}
}
}
}
Expand All @@ -292,7 +274,6 @@ mod tests {
use std::sync::{Arc, RwLock};

use super::*;
use crate::vhu_can::VhostUserCanBackend;

#[test]
fn test_can_controller_creation() {
Expand Down Expand Up @@ -337,7 +318,7 @@ mod tests {
#[test]
fn test_can_controller_operation() {
let can_name = "can".to_string();
let mut controller = CanController::new(can_name.clone()).unwrap();
let controller = CanController::new(can_name.clone()).unwrap();

let frame = VirtioCanFrame {
msg_type: VIRTIO_CAN_RX.into(),
Expand All @@ -348,14 +329,9 @@ mod tests {
sdu: [0; 64],
};

match controller.open_can_socket() {
Ok(_) => {
// Test operation
let operation_result = controller.can_out(frame);
assert!(operation_result.is_ok());
}
Err(_) => warn!("There is no CAN interface with {can_name} name"),
}
// Test operation
let operation_result = controller.can_out(frame);
assert!(operation_result.is_ok());
}

#[test]
Expand All @@ -366,32 +342,15 @@ mod tests {

// Test start_read_thread
let thread_handle = CanController::start_read_thread(arc_controller.clone());

// Signal the thread to exit and wait for it
arc_controller.write().unwrap().exit_read_thread();
assert!(thread_handle.join().is_ok());
}

#[test]
fn test_can_open_socket_fail() {
let controller =
CanController::new("can0".to_string()).expect("Could not build controller");
let controller = Arc::new(RwLock::new(controller));
VhostUserCanBackend::new(controller.clone()).expect("Could not build vhucan device");

assert_eq!(
controller.write().unwrap().open_can_socket(),
Err(Error::SocketOpen)
);
}

#[test]
fn test_can_read_socket_fail() {
let controller =
CanController::new("can0".to_string()).expect("Could not build controller");
let controller = Arc::new(RwLock::new(controller));
VhostUserCanBackend::new(controller.clone()).expect("Could not build vhucan device");

assert_eq!(
CanController::read_can_socket(controller),
Err(Error::SocketOpen)
);
let err = CanController::new("can0".to_string()).unwrap_err();
assert_eq!(err, Error::SocketOpen);
}
}
2 changes: 1 addition & 1 deletion vhost-device-can/src/vhu_can.rs
View file
Open in desktop

@stefano-garzarella stefano-garzarella Apr 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mmm, so if we run cargo test before commit 3be95bc but after commit 1 is it working or failing?

IMO we should keep tests running to avoid breaking the bisectability, so better to move this commit or squash.

@MatiasVara MatiasVara Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see you mean that test shall pass for each commit right?

@stefano-garzarella stefano-garzarella Apr 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yep, each commit should build and pass tests

Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::{
sync::{Arc, RwLock},
};

use log::{error, trace, warn};
use log::{trace, warn};
use thiserror::Error as ThisError;
use vhost::vhost_user::message::{VhostUserProtocolFeatures, VhostUserVirtioFeatures};
use vhost_user_backend::{VhostUserBackendMut, VringEpollHandler, VringRwLock, VringT};
Expand Down