On 1/27/26 4:11 PM, Matteo Perin via dev wrote:
> This commit adds the peer_ifindex to the interface status column for
> Linux veth devices, allowing applications like OVN to programmatically
> discover veth pairs without requiring additional system calls or manual
> configuration.
>
> The implementation leverages the existing ethtool infrastructure:
> - Uses ETHTOOL_GDRVINFO to identify veth devices and get n_stats
> - Queries ETH_SS_STATS string set to get statistic names
> - Dynamically allocates buffer based on n_stats
> - Uses ETHTOOL_GSTATS to retrieve veth statistics
> - Finds peer_ifindex by name in the statistics array
>
> The ifindex value is cached to avoid scalability issues with a
> considerable amount of ports.
> The cached peer_ifindex is then properly invalidated when the
> corresponding veth device is removed.
> This ensures that, when veth pairs are deleted and recreated with
> the same name, the new peer_ifindex is correctly detected and reported.
>
> A test that create veth pairs across network namespaces, verifies
> peer_ifindex reporting, and validates correct detection of ifindex
> changes when pairs are recreated was added as well.
>
> Signed-off-by: Matteo Perin <[email protected]>
> ---
> lib/netdev-linux-private.h | 1 +
> lib/netdev-linux.c | 49 ++++++++++++++++++++++++++++++++++++++
> tests/system-interface.at | 47 ++++++++++++++++++++++++++++++++++++
> vswitchd/vswitch.xml | 13 ++++++++++
> 4 files changed, 110 insertions(+)
Hi, Matteo. Thanks for the patch! It seems functionally correct (though
I didn't actually try it). I left a few comments below, mostly style
and formatting related.
Best regards, Ilya Maximets.
>
> diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
> index cf4a021d3..3f3fc04b5 100644
> --- a/lib/netdev-linux-private.h
> +++ b/lib/netdev-linux-private.h
> @@ -97,6 +97,7 @@ struct netdev_linux {
> uint8_t current_duplex; /* Cached from ETHTOOL_GSET. */
>
> struct ethtool_drvinfo drvinfo; /* Cached from ETHTOOL_GDRVINFO. */
> + uint64_t peer_ifindex; /* Cached from ETHTOOL_GSTATS (veth). */
> struct tc *tc;
>
> /* For devices of class netdev_tap_class only. */
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 93a064cc2..45779cffa 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -3730,11 +3730,52 @@ netdev_linux_get_status(const struct netdev *netdev_,
> struct smap *smap)
>
> COVERAGE_INC(netdev_get_ethtool);
> memset(&netdev->drvinfo, 0, sizeof netdev->drvinfo);
> + netdev->peer_ifindex = 0;
> error = netdev_linux_do_ethtool(netdev->up.name,
> cmd,
> ETHTOOL_GDRVINFO,
> "ETHTOOL_GDRVINFO");
> if (!error) {
> + /* For veth devices, also get the 'peer_ifindex'. */
> + size_t n_stats = netdev->drvinfo.n_stats;
> + if (!strcmp(netdev->drvinfo.driver, "veth") &&
> + n_stats > 0) {
This fits into a single line. But also there is too much nesting
happening here and it's hard to read. The content of this if
statement can be moved into a separate function.
Also, while writing the function, please convert all the blocks
like:
if (!error) {
<do something>
}
into:
if (error) {
return;
}
<do something>
To save on indentation.
> + struct ethtool_gstrings *names = NULL;
> + struct ethtool_stats *stats = NULL;
> + int s_error;
> +
> + /* Get the names of the statistics. */
> + s_error = netdev_linux_read_definitions(netdev,
> + ETH_SS_STATS,
> + &names);
Indent to the opening parenthesis.
> + if (!s_error) {
> + stats = xzalloc(sizeof *stats +
> + n_stats * sizeof stats->data[0]);
> + stats->cmd = ETHTOOL_GSTATS;
> + stats->n_stats = n_stats;
> + /* Get the statistics values. */
> + s_error = netdev_linux_do_ethtool(netdev->up.name,
> + (struct ethtool_cmd *) stats,
> + ETHTOOL_GSTATS,
> + "ETHTOOL_GSTATS");
This should also be indented to the opening parenthesis. If the cast
doesn't fit, can cast and assing to 'cmd' before the call.
> + if (!s_error) {
> + /* Find 'peer_ifindex' in names and get value. */
> + for (uint32_t i = 0;
> + i < names->len && i < stats->n_stats;
> + i++) {
Indent to the opening parenthesis, i.e. 1 space to the right.
> + char *name =
> + (char *) &names->data[i * ETH_GSTRING_LEN];
> + if (!strcmp(name, "peer_ifindex") &&
> + stats->data[i] > 0) {
> + netdev->peer_ifindex = stats->data[i];
> + break;
> + }
> + }
> + }
> + free(stats);
> + }
> + free(names);
No need to free names on error.
> + }
> netdev->cache_valid |= VALID_DRVINFO;
> }
> }
> @@ -3743,6 +3784,14 @@ netdev_linux_get_status(const struct netdev *netdev_,
> struct smap *smap)
> smap_add(smap, "driver_name", netdev->drvinfo.driver);
> smap_add(smap, "driver_version", netdev->drvinfo.version);
> smap_add(smap, "firmware_version", netdev->drvinfo.fw_version);
> +
> + /* Report the cached 'peer_ifindex' (for veth devices). */
> + if (netdev->peer_ifindex > 0) {
It seems like the value can't be negative in this code, so the
'> 0' part is redundant.
> + smap_add_format(smap,
> + "peer_ifindex",
> + "%"PRIu64,
> + netdev->peer_ifindex);
This can be two lines instead of 4 - everything on the same line as smap_add
and the last value on its own, indented to the opening parenthesis.
> + }
> }
> ovs_mutex_unlock(&netdev->mutex);
>
> diff --git a/tests/system-interface.at b/tests/system-interface.at
> index 15c4a0e2e..b24cbc855 100644
> --- a/tests/system-interface.at
> +++ b/tests/system-interface.at
> @@ -221,3 +221,50 @@ half
>
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> +
> +dnl Test that peer_ifindex is correctly reported for veth devices.
> +AT_SETUP([interface - veth peer_ifindex status])
> +AT_SKIP_IF([test $HAVE_ETHTOOL = "no"])
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +dnl Create a network namespace for the peer veth end.
> +AT_CHECK([ip netns add testns])
> +on_exit 'ip netns del testns'
We have ADD_NAMESPACES macro that handles everything.
> +
> +dnl Create a veth pair: ovs-veth0 (main ns) <-> ovs-veth1 (testns).
> +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1])
> +AT_CHECK([ip link set ovs-veth1 netns testns])
> +on_exit 'ip link del ovs-veth0'
This should be right after addition.
> +
> +dnl Add ovs-veth0 to br0.
This is obvious, no need for a comment.
> +AT_CHECK([ovs-vsctl add-port br0 ovs-veth0])
> +
> +dnl Get the actual ifindex of ovs-veth1 in testns and verify it's reported.
> +PEER1_IFINDEX=$(ip netns exec testns ip link show ovs-veth1 | sed -n
> 's/^\([[0-9]]*\):.*/\1/p')
May be better to use lowercase for shell variables to easier distinguish
them from m4 macros. Also, might be better to AT_CHECK into stdout and
then parse it. It would also save some line length.
> +
> +dnl Verify OVS reports the correct peer_ifindex.
> +OVS_WAIT_UNTIL_EQUAL([ovs-vsctl get interface ovs-veth0
> status:peer_ifindex], ["\"$PEER1_IFINDEX\""])
may split the line after the comma. Indent to the opening parenthesis.
Also, I didn't try this test, but is it necessary to add those quotes?
> +
> +dnl The veth pair is deleted and then recreated with the same name, this
> tests ifindex stale values detection.
Line is a bit too long.
> +AT_CHECK([ip link del ovs-veth0])
> +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1])
> +AT_CHECK([ip link set ovs-veth1 netns testns])
> +
> +dnl Get the NEW peer's ifindex (will be different).
> +PEER2_IFINDEX=$(ip netns exec testns ip link show ovs-veth1 | sed -n
> 's/^\([[0-9]]*\):.*/\1/p')
Same here.
> +
> +dnl Verify OVS detects the change and reports the NEW peer_ifindex.
> +OVS_WAIT_UNTIL_EQUAL([ovs-vsctl get interface ovs-veth0
> status:peer_ifindex], ["\"$PEER2_IFINDEX\""])
And here.
> +
> +dnl Verify the two peer ifindexes are different.
> +test "$PEER1_IFINDEX" != "$PEER2_IFINDEX"
This should be under AT_CHECK, otherwise it doesn't really verify much.
> +
> +OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> +/could not open network device ovs-veth0/d
> +/cannot get .*STP status on nonexistent port/d
> +/ethtool command .*on network device ovs-veth0 failed/d
> +/error receiving .*ovs-veth0/d
> +/ovs-veth0: removing policing failed/d"])
> +
> +AT_CLEANUP
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index b7a5afc0a..25f635e82 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3746,6 +3746,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> The version string of the network adapter's firmware, if available.
> </column>
>
> + <column name="status" key="peer_ifindex">
> + <p>
> + For veth devices on Linux, this column contains the interface index
> + of the peer veth device. This allows applications to
> + programmatically discover veth pairs without requiring additional
> + system calls or parsing network interface information.
> + </p>
> + <p>
> + Only present for veth devices (when <ref column="status"
> + key="driver_name"/> is "veth"). Not available on non-Linux
> systems.
> + </p>
> + </column>
> +
> <column name="status" key="source_ip">
> The source IP address used for an IPv4/IPv6 tunnel end-point, such as
> <code>gre</code>.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
