On 19.07.22 09:45, Vincent Mailhol wrote:
On Tue. 19 Jul. 2022 at 14:46, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
Enable CAN_RAW sockets to read and write CAN XL frames analogue to the
CAN FD extension (new CAN_RAW_XL_FRAMES sockopt).
A CAN XL network interface is capable to handle Classical CAN, CAN FD and
CAN XL frames. When CAN_RAW_XL_FRAMES is enabled, the CAN_RAW socket checks
whether the addressed CAN network interface is capable to handle the
provided CAN frame.
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
---
include/uapi/linux/can/raw.h | 6 +++
net/can/raw.c | 97 +++++++++++++++++++++++++++++++-----
2 files changed, 90 insertions(+), 13 deletions(-)
diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 3386aa81fdf2..5a0e480887ff 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -60,8 +60,14 @@ enum {
CAN_RAW_ERR_FILTER, /* set filter for error frames */
CAN_RAW_LOOPBACK, /* local loopback (default:on) */
CAN_RAW_RECV_OWN_MSGS, /* receive my own msgs (default:off) */
CAN_RAW_FD_FRAMES, /* allow CAN FD frames (default:off) */
CAN_RAW_JOIN_FILTERS, /* all filters must match to trigger */
+ CAN_RAW_XL_FRAMES, /* allow CAN XL frames (default:off) */
};
+/* CAN XL data transfer modes for CAN_RAW_XL_FRAMES sockopt */
+#define CAN_RAW_XL_ENABLE (1 << 0) /* enable CAN XL frames on this socket */
+#define CAN_RAW_XL_RX_DYN (1 << 1) /* enable truncated data[] for read() */
+#define CAN_RAW_XL_TX_DYN (1 << 2) /* enable truncated data[] for write() */
This change leaves me perplexed. If I understand correctly, the only
purpose is to provide those two APIs is to have:
* one for the beginner users: fixed packet size in order to keep the
approach of CAN(-FD).
* one the advanced users: variable packet size.
I did not think about beginners or advances users.
To me it was like:
When I know that I often get 'shorter' CAN XL frames, I can enable this
option.
In fact the usage pattern with the truncated data is pretty handy:
https://github.com/hartkopp/can-tests/blob/canxl/raw/canxl-rx.c#L70
I don’t think that there are any precedent cases where the kernel
exposed two differents APIs for the sake of pleasing both beginner and
advanced users. Sending packets of variable sizes between the kernel
and the userland is a solved problem and I feel uncomfortable
introducing custom solutions in order to prevent hypothetical misuses.
I am not even convinced that this solution is safer than the V2. If
you introduce different options, there will always be someone who will
mix up things by copy pasting some random code snippets and get an
unsafe result. IMHO, it is better to build a simple interface and give
a reference implementation rather than trying to add complexity to
anticipate misuses.
Yes. Then I would vote for selecting the 'truncated' option only - and
therefore remove CAN_RAW_XL_[R|T]X_DYN flags that would be 'always on'.
Best regards,
Oliver
