Fix tests in vhost-can-device#957
Conversation
MatiasVara
requested review from
dorindabassey,
epilys,
stefano-garzarella,
stsquad and
vireshk
as code owners
| length: 64.into(), | ||
| reserved: 0.into(), | ||
| flags: 0.into(), | ||
| flags: CAN_FRMF_TYPE_FD.into(), |
There was a problem hiding this comment.
Shouldn't this be VIRTIO_CAN_FLAGS_FD?
There was a problem hiding this comment.
In this case, the frame is sent directly to the host backend through can_out(), which expect CAN_FRMF_TYPE_FD. It seems that when we get something from the guest in the tx queue, the flag could be VIRTIO_CAN_FLAGS_FD but then it is processed by check_tx_frame() that convert it to CAN_FRMF_TYPE_FD. This test would sit after check_tx_frame(). I think, to use VIRTIO_CAN_FLAGS_FD, we would need to add check_tx_frame() to the test.
There was a problem hiding this comment.
Ah, makes sense. This is not ideal modelling but this isn't this patch's fault. Is it easy to add check_tx_frame() here so that we are consistent?
There was a problem hiding this comment.
I will try it and let you know.
There was a problem hiding this comment.
check_tx_frame() is private in vhu_can.rs. I think if I want to use it in can.rs I would need to make it public. The other option is to move the test to vhu_can.rs and use check_tx_frame() with VIRTIO_CAN_FLAGS_FD.
|
@epilys if that's OK I will update the PR with other commits that fix tests so I do not open another PR. |
|
Sure thing |
MatiasVara
force-pushed
the
can-fix-test
branch
from
f02bd9f to
a905949
Compare
Fix test by using CAN_FRMF_TYPE_FD flag for 64 bytes packets and use VIRTIO_CAN_TX as type. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
Make the test finish when interface exists. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
Add a mock interface for tests and an env variable for running the tests with a real interface. To run with a real CAN interface named `vcan0`, run: TEST_CAN_INTERFACE=vcan0 cargo test --package vhost-device-can Also, remove the name of the interfaces from tests since we use either the mock interface or an existing interface. For those tests that do not require an interface because they test a failing path, use `nonexistent_can` interface. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
The CAN-FD flag inference logic in check_rx_frame was gated on can_name == "vcan0". This was added as a workaround for virtual CAN interfaces (vcan), which do not distinguish between CAN and CAN-FD frames at the socket level, i.e., all frames arrive as CanAnyFrame::Normal regardless of their actual type. However, the interface name check is unnecessary. Classic CAN frames can never exceed 8 bytes, so the condition `res_len > 8` is already sufficient to infer a CAN-FD frame when the feature has been negotiated. This holds for all interface types: - vcan: frames arrive without the FD flag, but res_len > 8 detects them - Real CAN-FD hardware: the FD flag is already set by process_frame, so the additional |= is redundant but harmless - Real classic CAN hardware: frames never exceed 8 bytes, so the condition is never triggered Removing the name check also fixes the limitation of only supporting an interface named exactly "vcan0", which excluded vcan1, vcan2, etc. Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
MatiasVara
force-pushed
the
can-fix-test
branch
from
a905949 to
8a2257e
Compare
2 participants
Summary of the PR
This PR includes some fixes for the test in vhost-user-can device. It also adds a mock interface to run the tests in an environment that does not have a can interface. It adds an option to set the name of the interface for tests so it can run on a real interface too. The PR also removes the name of the interface that is hardcoded in check_rx_frame() that is not required.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.