Re: [PATCH net-next v3 4/5] selftests: drv-net: psp: factor out psp connection setup

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Sat Nov 7, 2026 at 3:59 PM EST, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The commit message says "Here we are adding some more error
>   paths, so we just handle errors by closing the data sock".
>
> --
>
> selftests: drv-net: psp: factor out psp connection setup
>
> This commit moves the PSP connection setup and the assoc uapi calls in
> psp.py and psp_responder.c into helpers. Other tests that start with a key
> exchange can then reuse that sequence. It also makes run_session() close
> the data socket when conn_setup_psp() fails.
>
>> The caller of conn_setup_psp() not handling errors was preexisting. Here
>> we are adding some more error paths, so we just handle errors by closing
>> the data sock to make the code a bit more robust.
>
> [Severity: Low]
> This isn't a bug, but is the "adding some more error paths" part still
> accurate for v3?
>
> Before this patch, conn_setup_psp() returned -1 in three cases:
> psp_rx_assoc() returning NULL, recv() returning < 0, and psp_tx_assoc()
> returning NULL.
>
> After the patch, it fails in the same three cases:
>
>     rx_assoc() returns -1 when rsp is NULL
>     conn_setup_psp() returns -1 when recv() returns < 0
>     tx_assoc() returns -1 when tsp is NULL
>
> The v3 changelog says the per-version key length table was replaced with
> the key length from the kernel's rx-assoc reply. That table lookup looks
> like the only new way this function could fail, so the sentence may be
> left over from v1/v2.
>

Ah darn. I let local sashiko convince me it was a good idea not to
ignore the return code of conn_setup_psp(), even though it is purely a
preexisting "issue". Then, when I was writing the commit message, I
think I was looking at the code from the next patch where
handle_tx_assoc() rekey handler, has an extra check against the key
length.

If I have to respin, I'll probably drop the code chunk below entirely.
Otherwise if we can just edit the sentence:

"Here we are adding some more error paths, so we just handle errors by
closing the data sock".

so it says,

"Handle preexisting errors from conn_setup_psp(). No new error paths are
introduced."

Then it would be accurate.

> The only behaviour change is in run_session():
>
>     if (conn_setup_psp(ys, opts, data_sock)) {
>         close(data_sock);
>         data_sock = -1;
>     }
>
> Could the commit message give the actual reason for that change? For
> example, the existing failures can leave a half set up data socket open.