vhost-device-can: use the same socket for send/recv by MatiasVara · Pull Request #937 · rust-vmm/vhost-device

MatiasVara · 2026-02-23T09:40:12Z

Summary of the PR

Use the same socket for send/receive in the backend to prevent guest to receiving two copies of the message when it sends to the local CAN bus.

Fixes #925

Reported-by: Francesco Valla francesco@valla.it

Requirements

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

All commits in this PR have Signed-Off-By trailers (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.

MatiasVara · 2026-03-03T18:18:59Z

When fixing this, some tests started to fail since now the opening of the socket is happening in new(). Before, those tests did not require to open a socket to work. I fixed those tests by adding a few commits together with the fix. I wonder if these tests should be fixed here or in another PR.

MatiasVara · 2026-03-03T18:33:36Z

To run the tests, we require a virtual can interface in the host named can0.

stefano-garzarella · 2026-03-04T09:23:22Z

To run the tests, we require a virtual can interface in the host named can0.

Is this something new (i.e. added here) or was there before this PR?
If it's something new, I think we need to update the rust-vmm-container image to provide that device.

MatiasVara · 2026-03-04T10:26:01Z

To run the tests, we require a virtual can interface in the host named can0.

Is this something new (i.e. added here) or was there before this PR? If it's something new, I think we need to update the rust-vmm-container image to provide that device.

It was in the code before but it was not being tested. The tests were done to not use a can interface. I am not sure if that was done on purpose.

epilys · 2026-03-04T10:44:21Z

I think we should wrap the CanFdSocket field into an enum, and add a #[cfg(test)] variant that is a mock socket.

This way we can add support in the future for classic Can sockets (i.e. not FD) which is something I wanted to do anyway.

stefano-garzarella · 2026-03-04T11:07:23Z

To run the tests, we require a virtual can interface in the host named can0.

Is this something new (i.e. added here) or was there before this PR? If it's something new, I think we need to update the rust-vmm-container image to provide that device.

It was in the code before but it was not being tested. The tests were done to not use a can interface. I am not sure if that was done on purpose.

Maybe to avoid updating the container image xD (I have no idea).
So can we enable some can0 stub/looback interface in the container?

epilys · 2026-03-04T12:11:42Z

To run the tests, we require a virtual can interface in the host named can0.

Is this something new (i.e. added here) or was there before this PR? If it's something new, I think we need to update the rust-vmm-container image to provide that device.

It was in the code before but it was not being tested. The tests were done to not use a can interface. I am not sure if that was done on purpose.

Maybe to avoid updating the container image xD (I have no idea). So can we enable some can0 stub/looback interface in the container?

If the linux images contain the vcan kernel driver yes:

sudo modprobe vcan
sudo ip link add dev vcan0 type vcan
sudo ip link set vcan0 up

MatiasVara · 2026-03-04T14:14:29Z

I think we should wrap the CanFdSocket field into an enum, and add a #[cfg(test)] variant that is a mock socket.

Thanks! I'll try that. I did not get it completely yet though.

Use the same socket for send/receive in the backend to prevent the guest from receiving two copies of the message when it sends to the local CAN bus. Fix tests and remove test_can_read_socket_fail() test since read_can_socket() does not open the socket anymore. Fixes rust-vmm#925 Reported-by: Francesco Valla <francesco@valla.it> Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

Most of the tests were using `can0` as interface but some used `can`. Use `can0` interface for all tests and `canx` for those that expect an interface down. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

To support 64 bytes packet, set CAN_FRMF_TYPE_FD flag in test_can_controller_operation test. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

MatiasVara · 2026-03-11T11:14:32Z

The PR has now a wider scope :(. I am OK to split if it is required. Bear in mind that I used Claude for the last commit. Claude proposed a single builder for both socket types but I ignored it for the moment.

Rename this test that previously had to fail since the interface was down. The previous test was testing the NoOutputInterface path which does not exist anymore. Make the test pass when transmission succeeds. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

Add a mock socket for tests so that tests no longer require a CAN interface on the host. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

stefano-garzarella

Maybe we should re-order commit, for example the last 1 and other fixes should come first, but I did just a quick look, I may be wrong.

stefano-garzarella · 2026-04-15T13:07:47Z

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.).

stefano-garzarella · 2026-04-15T13:14:09Z

+                ctrl.can_socket.read_frame()
+            };
+
+            if let Ok(frame) = frame_result {


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.

I did not get the question.

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?

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.

stefano-garzarella · 2026-04-15T13:17:52Z

-        if self.check_features(VIRTIO_CAN_F_CAN_FD) && res_len > 8 && can_name == "vcan0" {
+        if self.check_features(VIRTIO_CAN_F_CAN_FD) && res_len > 8 && can_name == "can0" {
            res_flags |= CAN_FRMF_TYPE_FD;
            warn!("\n\n\nCANFD VCAN0\n\n");


stil VCAN0 here?

BTW are we sure it's fine to can0 instead of vcan0?
Not sure if v prefix was done for a virtual purporse.

The v is from virtual CAN which is the interface used in the host although it can be named also can0. In the guest, the driver is a real can, e.g., virtio-can. Virtual CAN is like an emulator.

sorry, now I'm more confused than before :-)

What this code is doing ? Is related to test ?

This code is not related with the test. There is a problem with this code. It is inferring that the host is using virtual can since the name is vcan0, which can be false since the name can be anything. I missed that when I changed the name although it would work because I can just set the name of the interface to can0 but it does not seem correct.

ah, so maybe better to use a dedicated commit for this and explain better the issue

stefano-garzarella · 2026-04-15T13:19:05Z

@@ -248,7 +248,7 @@ mod tests {
    #[test]
    fn test_can_valid_configuration_start_backend_server_fail() {


I think this is pre-existing, but the test name here is a bit confusing, I mean "valid_configuration" while we are opening an invalid interface IIUC.

yes it is, and I wonder what is the difference with test_can_valid_configuration_start_backend() test

stefano-garzarella · 2026-04-15T13:22:35Z

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.

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

yep, each commit should build and pass tests

stefano-garzarella · 2026-04-15T13:26:53Z

+        match self {
+            CanSocketKind::Fd(socket) => socket.read_frame(),
+            #[cfg(test)]
+            CanSocketKind::Mock => Err(std::io::ErrorKind::WouldBlock.into()),


Why returning WouldBlock ?

It seems it tries to emulate what recv() in socket.read_frame() returns when there is nothing to read, which is always the case for the Mock interface.

Worth a comment IMO

MatiasVara requested review from dorindabassey, epilys, stefano-garzarella, stsquad and vireshk as code owners February 23, 2026 09:40

MatiasVara mentioned this pull request Feb 23, 2026

vhost-device-can: consume desc chain based on rx fifo queue #924

Open

4 tasks

stefano-garzarella reviewed Feb 23, 2026

View reviewed changes

Comment thread vhost-device-can/src/can.rs

stefano-garzarella reviewed Feb 23, 2026

View reviewed changes

Comment thread vhost-device-can/src/can.rs Outdated

MatiasVara force-pushed the fix-for-925 branch from f460f1c to d2564a0 Compare March 3, 2026 18:19

MatiasVara added 3 commits March 11, 2026 10:52

Use can0 interface in all tests

468b098

Most of the tests were using `can0` as interface but some used `can`. Use `can0` interface for all tests and `canx` for those that expect an interface down. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

Set CAN_FRMF_TYPE_FD flag

09a1ec2

To support 64 bytes packet, set CAN_FRMF_TYPE_FD flag in test_can_controller_operation test. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

MatiasVara force-pushed the fix-for-925 branch from d2564a0 to e2a0f2a Compare March 11, 2026 11:12

MatiasVara added 2 commits March 13, 2026 11:26

Add mock socket for tests

2abb694

Add a mock socket for tests so that tests no longer require a CAN interface on the host. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

MatiasVara force-pushed the fix-for-925 branch from e2a0f2a to 2abb694 Compare March 13, 2026 10:27

stefano-garzarella reviewed Apr 15, 2026

View reviewed changes

		@@ -248,7 +248,7 @@ mod tests {
		#[test]
		fn test_can_valid_configuration_start_backend_server_fail() {

Uh oh!

Conversation

MatiasVara commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the PR

Requirements

Uh oh!

Uh oh!

Uh oh!

MatiasVara commented Mar 3, 2026

Uh oh!

MatiasVara commented Mar 3, 2026

Uh oh!

stefano-garzarella commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatiasVara commented Mar 4, 2026

Uh oh!

epilys commented Mar 4, 2026

Uh oh!

stefano-garzarella commented Mar 4, 2026

Uh oh!

epilys commented Mar 4, 2026

Uh oh!

MatiasVara commented Mar 4, 2026

Uh oh!

MatiasVara commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefano-garzarella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatiasVara Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatiasVara Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

MatiasVara commented Feb 23, 2026 •

edited

Loading

stefano-garzarella commented Mar 4, 2026 •

edited

Loading

MatiasVara commented Mar 11, 2026 •

edited

Loading

MatiasVara Apr 21, 2026 •

edited

Loading

MatiasVara Apr 21, 2026 •

edited

Loading