|SUCCESS| [v2,2/2] ethdev: add telemetry endpoint for list names

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164189

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-20

Reviewing the patch series for DPDK coding standards and best practices.

git sha: 20260520093804.29102-2-fengchengwen at huawei.com (patch 1/2)
git sha: 20260520093804.29102-3-fengchengwen at huawei.com (patch 2/2)
Author: Chengwen Feng <fengchengwen at huawei.com>
Subject: [PATCH v2 1/2] dmadev: add telemetry endpoint for list names
Subject: [PATCH v2 2/2] ethdev: add telemetry endpoint for list names

This patch series adds telemetry endpoints to return device names as
dictionaries for dmadev and ethdev subsystems, allowing users to
identify devices by name directly from telemetry output.

> diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
> index b75b4f9bd1..98ef7e2b93 100644
> --- a/lib/dmadev/rte_dmadev.c
> +++ b/lib/dmadev/rte_dmadev.c

[ ... ]

> +static int
> +dmadev_handle_dev_list_names(const char *cmd __rte_unused,
> +		const char *params __rte_unused,
> +		struct rte_tel_data *d)
> +{
> +	char id_str[RTE_TEL_MAX_STRING_LEN];
> +	struct rte_dma_dev *dev;
> +	int dev_id;
> +
> +	rte_tel_data_start_dict(d);
> +	for (dev_id = 0; dev_id < dma_devices_max; dev_id++) {
> +		if (!rte_dma_is_valid(dev_id))
> +			continue;
> +		dev = &rte_dma_devices[dev_id];
> +		sprintf(id_str, "%d", dev_id);

Does this code risk buffer overflow with sprintf()? The DPDK coding
style prefers snprintf() for string formatting to prevent buffer
overflows. Consider using snprintf(id_str, sizeof(id_str), "%d", dev_id)
instead.

> +		rte_tel_data_add_dict_string(d, id_str, dev->data->dev_name);

Can dev->data be NULL here? The code dereferences dev->data->dev_name
without checking if dev->data is valid. In dmadev_handle_dev_list_names(),
after getting dev = &rte_dma_devices[dev_id], does the code need to
verify dev->data is non-NULL before accessing dev->data->dev_name?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
> index a910864bc5..0e71419473 100644
> --- a/lib/ethdev/rte_ethdev_telemetry.c
> +++ b/lib/ethdev/rte_ethdev_telemetry.c

[ ... ]

> +static int
> +eth_dev_handle_port_list_names(const char *cmd __rte_unused,
> +		const char *params __rte_unused,
> +		struct rte_tel_data *d)
> +{
> +	char id_str[RTE_TEL_MAX_STRING_LEN];
> +	struct rte_eth_dev *dev;
> +	int port_id;
> +
> +	rte_tel_data_start_dict(d);
> +	RTE_ETH_FOREACH_DEV(port_id) {
> +		dev = &rte_eth_devices[port_id];
> +		sprintf(id_str, "%d", port_id);

Same issue as dmadev: does this code risk buffer overflow with sprintf()?
Consider using snprintf(id_str, sizeof(id_str), "%d", port_id) to prevent
potential buffer overflows.

> +		rte_tel_data_add_dict_string(d, id_str, dev->data->name);

Similar to dmadev, can dev->data be NULL here? The code in
eth_dev_handle_port_list_names() dereferences dev->data->name without
NULL checking. Does RTE_ETH_FOREACH_DEV() guarantee that dev->data is
valid for all iterated ports?

> +	}
> +	return 0;
> +}

[ ... ]