On 5/18/26 5:04 PM, Reuven Plevinsky via dev wrote:
> The kernel datapath cannot hold ports whose names are IFNAMSIZ bytes
> or longer, so dpif_netlink_port_query_by_name() can never find them.
>
> During restart the ofproto construct path calls dpif_port_exists() for
> every configured interface. For ports with long names (for example
> patch ports) this reached the kernel, which returned an error that was
> logged as a spurious "failed to query port <long_port_name>: Invalid
> argument" warning.
>
> Return ENODEV early when the name is too long, avoiding the round-trip
> and the misleading log message.
>
> Fixes: c19e653509de ("datapath: Change userspace vport interface to use
> Netlink attributes.")
> Signed-off-by: Reuven Plevinsky <[email protected]>
Hi, Reuven. This patch seems AI-generated (mainly judging by the comments).
If that's the case, you need to disclose that with the appropriate tags,
see the contribution guide.
> ---
> lib/dpif-netlink.c | 4 ++++
> tests/system-interface.at | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 35 insertions(+)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index f22a87934089..0f2bc5e7eed2 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1263,6 +1263,10 @@ dpif_netlink_port_query_by_name(const struct dpif
> *dpif_, const char *devname,
> {
> struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>
> + if (strlen(devname) >= IFNAMSIZ) {
> + return ENODEV;
> + }
> +
> return dpif_netlink_port_query__(dpif, 0, devname, dpif_port);
> }
>
> diff --git a/tests/system-interface.at b/tests/system-interface.at
> index 20a882d1cfe6..e7b11773fe90 100644
> --- a/tests/system-interface.at
> +++ b/tests/system-interface.at
> @@ -231,3 +231,34 @@ AT_CHECK([grep 'name too long' stderr], [0], [ignore])
>
> OVS_TRAFFIC_VSWITCHD_STOP(["/could not open network device.*name too
> long/d"])
> AT_CLEANUP
> +
> +AT_SETUP([interface - patch port name exceeds IFNAMSIZ on vswitchd restart])
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +dnl Kernel datapath only (see system-kmod-testsuite).
> system-userspace-testsuite
> +dnl also includes this file but uses netdev datapath — same pattern as
> "datapath ports gc".
This comment includes a lot of unnecessary details and generally pointless.
use a short comment as other tests do.
> +AT_SKIP_IF([! ovs-appctl dpctl/show 2>/dev/null | grep -q ovs-system])
> +
> +dnl Port names longer than IFNAMSIZ-1 (Linux). Uses kernel (system)
> datapath — see
> +dnl system-kmod-macros _ADD_BR / _OVS_VSWITCHD_START (no --disable-system).
This is hard to understand and mostly pointless as well. Also, avoid the
em-dashes.
> +AT_CHECK([ovs-vsctl -- \
> + add-br br1 -- set bridge br1 fail-mode=secure \
> + -- add-port br0 long_patch_peer_one \
> + -- set interface long_patch_peer_one type=patch \
> + options:peer=long_patch_peer_two \
> + -- add-port br1 long_patch_peer_two \
> + -- set interface long_patch_peer_two type=patch \
> + options:peer=long_patch_peer_one])
> +
> +dnl Restart ovs-vswitchd with a fresh ovs-vswitchd.log
> +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
> +mv ovs-vswitchd.log ovs-vswitchd_1.log
Why removing the old one?
> +AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn
> -vofproto_dpif -vunixctl],
> + [0], [], [stderr])
> +OVS_WAIT_UNTIL([ovs-appctl -t ovs-vswitchd version 2>/dev/null | grep -q
> 'Open vSwitch'])
Not sure this is needed.
> +
> +AT_CHECK([grep -E 'failed to query port.*Invalid argument'
> ovs-vswitchd.log], [1], [], [])
This is also not needed, the warnings will be checked at the end. Just add a
comment.
Looking for specific warnings is unreliable.
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
