Re: [dpdk-dev] [PATCH v1 05/13] ethdev: add private generic device iterator

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 08/30/2018 04:41 PM, Gaetan Rivet wrote:
> This iterator can be customized with a comparison function that will
> trigger a stopping condition.
>
> It can be leveraged to write several different iterators that have
> similar but non-identical purposes.
>
> It is private to librte_ethdev.
>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>   lib/librte_ethdev/Makefile      |  1 +
>   lib/librte_ethdev/eth_private.c | 31 +++++++++++++++++++++++++++++++
>   lib/librte_ethdev/eth_private.h | 26 ++++++++++++++++++++++++++
>   lib/librte_ethdev/meson.build   |  3 ++-
>   4 files changed, 60 insertions(+), 1 deletion(-)
>   create mode 100644 lib/librte_ethdev/eth_private.c
>   create mode 100644 lib/librte_ethdev/eth_private.h
>
> diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
> index 0935a275e..3c1c92cb9 100644
> --- a/lib/librte_ethdev/Makefile
> +++ b/lib/librte_ethdev/Makefile
> @@ -18,6 +18,7 @@ EXPORT_MAP := rte_ethdev_version.map
>   
>   LIBABIVER := 10
>   
> +SRCS-y += eth_private.c
>   SRCS-y += rte_ethdev.c
>   SRCS-y += rte_flow.c
>   SRCS-y += rte_tm.c
> diff --git a/lib/librte_ethdev/eth_private.c b/lib/librte_ethdev/eth_private.c
> new file mode 100644
> index 000000000..d565568a0
> --- /dev/null
> +++ b/lib/librte_ethdev/eth_private.c

Just a nit
I think it is better to name it ethdev_private.c since we already
have ethdev_profile.[ch] and it is about ethdev, not Ethernet.

> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Gaëtan Rivet
> + */
> +
> +#include "rte_ethdev.h"
> +#include "eth_private.h"
> +
> +struct rte_eth_dev *
> +eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
> +		const void *data)
> +{
> +	struct rte_eth_dev *edev;
> +	ptrdiff_t idx;
> +
> +	/* Avoid Undefined Behaviour */
> +	if (start != NULL &&
> +	    (start < &rte_eth_devices[0] ||
> +	     start > &rte_eth_devices[RTE_MAX_ETHPORTS]))
> +		return NULL;
> +	if (start != NULL)
> +		idx = start - &rte_eth_devices[0] + 1;
> +	else
> +		idx = 0;
> +	for (; idx < RTE_MAX_ETHPORTS; idx++) {
> +		edev = &rte_eth_devices[idx];

Shouldn't we limit it to valid ports only?
If no, I think it would be useful to highlight it in the function
description that it iterates over all devices including unused.

> +		if (cmp(edev, data) == 0)
> +			return edev;
> +	}
> +	return NULL;
> +}
> +
> diff --git a/lib/librte_ethdev/eth_private.h b/lib/librte_ethdev/eth_private.h
> new file mode 100644
> index 000000000..0f5c6d5c4
> --- /dev/null
> +++ b/lib/librte_ethdev/eth_private.h

ethdev_private.h

<...>