linux-can / can-utils

Linux-CAN / SocketCAN user space applications
2.31k stars 698 forks source link

can-utils: support CAN XL #504

Closed hartkopp closed 4 months ago

hartkopp commented 4 months ago

This PR adds support for CAN XL including VCIDs (Linux v6.9+)

Tools that completely support CAN XL:

CC/FD-only tools that have been adapted to the lib.[ch] changes:

Main changes:

marckleinebudde commented 4 months ago

Wow! That's a big patch, was probably quite some work!

Please don't make variables static. Why is there the checking of the AFRSZ?

hartkopp commented 4 months ago

Wow! That's a big patch, was probably quite some work!

Yes. With the introduction of CAN XL the data length and some new CAN XL features the infrastructure needs a heavy rework. But the idea of a self containing CAN frame union (cut) really helped to have all needed information in one place. And removing the fprint(long_)canframe stuff led to a good clean up and complexity reduction.

The patches have been created over some days, which helped to review and rethink details over night. There are many git commit -a --amend and git push -f sequences under the hood ;-)

hartkopp commented 4 months ago

Can we keep the usual return value semantics of snprintf()?

Upon successful return, these functions return the number of characters printed (excluding the null byte used to end output to strings). The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit, then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated.

Especially in snprintf(long)canframe() the ASCII content is created with direct writes to the given buffer - and not with snprintf() statements that would provide a good API to do 'safe' writes on every operation.

Btw. I can understand that

                /* check if the CAN frame fits into the provided buffer */
                if (sizeof("00123#11:22:12345678#") + 2 * len + sep ? len : 0 > size) {
                        buf[0] = 0;
                        return 0;
                }

does not really comply with the common snprintf() return pattern as it returns '0' in the case of an overflow and not 'size'.

Would such an approach work for you then:

                /* check if the CAN frame fits into the provided buffer */
                if (sizeof("00123#11:22:12345678#") + 2 * len + sep ? len : 0 > size) {
                        /* mark buffer overflow in output */
                        memset(buf, '-', size);
                        buf[size] = 0;
                        return size;
                }

Btw. is there probably an off-by-one problem with the size to place the trailing string termination '0'? If so, I would place a

if (size > 0)
        size--;

at the start of snprintf(long)canframe()

hartkopp commented 4 months ago

Answering myself: Yes, there was an off-by-one problem with the size value. So fixed that and also added the "overflow" marker