Hi Vincent, On 2024-01-28 11:13, Vincent MAILHOL wrote:
Sorry for the delay, last weeks were busier than average.
No problem. I just wanted to finalize the data structure for further work on the CAN XL hardware driver and some CAN CiA protocol implementations.
I am not yet aware of all the details of CAN XL and its VCID, but to the extent of my knowledge, the patch looks good.
Thanks.
I left two nitpicks. Notwithstanding: you can add my review tag to v5. Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
I've sent out a v5 that addresses both of your good remarks: https://lore.kernel.org/linux-can/20240128183758.68401-1-socketcan@xxxxxxxxxxxx/
+ * functionality as is holds the @can_id flags CAN_xxx_FLAG^^ as it holds
Fixed.
@@ -814,12 +859,27 @@ static bool raw_bad_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu) (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu))) return false;
(..)
Just in terms of software design, I am not sure why this sanitization is part of raw_bad_txframe(). The purpose of this function is, so far, to just return a boolean telling whether or not the frame is valid. It is surprising to repurpose this to also do the sanitization. I would rather see the above code go in a new dedicated function (maybe raw_sanitize_tx_frame()) and call that new function from raw_sendmsg().
Right. I felt similar that the naming does not fit that good anymore when adding the VCID handling :-/
I splitted up the raw_bad_txframe() function and created two new functions: raw_check_txframe() (is raw_bad_txframe() with a non-bool return value) raw_put_canxl_vcid() (which handles the VCID content for CAN XL frames) Best regards, Oliver
