Re: [PATCH v4 3/3] j1939: add J1939 rx CTS tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing closing quote in bash script `test_rx_cts.sh` causing unexpected EOF syntax error, breaking the test.
- [Low] Incorrect `sizeof()` argument used when validating the received abort payload.
- [Low] Strict timing assertions might cause flaky test failures on heavily loaded CI systems.
--

--- Patch [3]: [PATCH v4 3/3] j1939: add J1939 rx CTS tests ---
commit 21e063d80602f46f314c362808e394f235e646f9
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

j1939: add J1939 rx CTS tests

This commit adds tests to verify the correct behavior of received J1939
CTS messages. It covers normal segmented transmission, transmission resuming
after a hold, aborting on requested already acked messages, and aborting on
too many retransmit requests.

> diff --git a/tools/testing/selftests/net/can/test_rx_cts.c b/tools/testing/selftests/net/can/test_rx_cts.c
> new file mode 100644
> index 0000000000000..1b437792d6772
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_rx_cts.c
[ ... ]
> +	/*
> +	 * The actual timeout is 1050ms. We define an acceptable window
> +	 * to account for CI scheduling variations.
> +	 */
> +	ASSERT_GE(elapsed_ms, 1000)
> +		TH_LOG("Abort received too early: %ld ms", elapsed_ms);
> +	ASSERT_LE(elapsed_ms, 1500)
> +		TH_LOG("Abort received too late: %ld ms", elapsed_ms);

[Severity: Low]
This isn't a bug, but could this strict timing assertion cause flaky test
failures?

An upper bound of 1500ms provides only a 450ms window for the 1050ms kernel
timeout, which might be easily exceeded due to thread scheduling delays under
heavy load on oversubscribed or virtualized CI runners.

[ ... ]
> +	/* Now send a CTS for an already acked frame */
> +	memcpy(tx_frame.data, CTS_1_FRAME_PAYLOAD, sizeof(CTS_1_FRAME_PAYLOAD));
> +	res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
> +	ASSERT_GT(res, 0)
> +		TH_LOG("failed to send second cts(1) with raw sock: %d", errno);
> +
> +	res = recv_payload(self->raw_sock, ABORT_DUP_SEQ_PAYLOAD, sizeof(DATA_2_PAYLOAD));

[Severity: Low]
Is this the correct size constant to use here?

It appears sizeof(ABORT_DUP_SEQ_PAYLOAD) should be used instead of
sizeof(DATA_2_PAYLOAD), even though both evaluate to 8 bytes.

[ ... ]
> diff --git a/tools/testing/selftests/net/can/test_rx_cts.sh b/tools/testing/selftests/net/can/test_rx_cts.sh
> new file mode 100755
> index 0000000000000..b2064ea7851e2
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_rx_cts.sh
[ ... ]
> +test_rx_cts()
> +{
> +	./test_rx_cts
> +	check_err $?
> +	log_test "test_rx_cts

[Severity: Medium]
Does this missing closing double quote cause an unexpected EOF syntax error
during shell parsing?

This would completely prevent the test script from running.

> +}
> +
> +trap cleanup EXIT

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706224548.100451-1-alexander.hoelzl@gmx.net?part=3