[PATCH 0/4] support more ethdev iterator filters

[PATCH 0/4] support more ethdev iterator filters

[Thomas Monjalon]

The new ethdev iterator lacks the support of filtering
by representor port id or by MAC address.

This series is not fully tested yet.

Thomas Monjalon (4):
  kvargs: support list value
  mk: remove broken check
  ethdev: move representor parsing functions
  ethdev: support representor id for iterating ports

 drivers/net/i40e/i40e_vf_representor.c   |  1 +
 drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
 drivers/net/mlx5/mlx5.c                  |  4 +-
 lib/librte_ethdev/ethdev_private.c       | 82 +++++++++++++++++++++++
 lib/librte_ethdev/ethdev_private.h       |  6 ++
 lib/librte_ethdev/rte_class_eth.c        | 44 ++++++++++++
 lib/librte_ethdev/rte_ethdev.c           | 85 ------------------------
 lib/librte_ethdev/rte_ethdev_core.h      |  2 +
 lib/librte_kvargs/rte_kvargs.c           | 10 +++
 mk/internal/rte.compile-pre.mk           |  1 -
 test/test/test_kvargs.c                  | 17 +++++
 11 files changed, 166 insertions(+), 87 deletions(-)

-- 
2.19.0

[PATCH 1/4] kvargs: support list value

[Thomas Monjalon]

If a value contains a comma, rte_kvargs_tokenize() will split here.
In order to support list syntax [a,b] as value, an extra parsing of
the square brackets is added.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_kvargs/rte_kvargs.c | 10 ++++++++++
 test/test/test_kvargs.c        | 17 +++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index a28f76945..b0774effb 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -44,6 +44,16 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 		    kvlist->pairs[i].value == NULL)
 			return -1;
 
+		/* Detect list [a,b] to skip comma delimiter in list. */
+		if (kvlist->pairs[i].value[0] == '[') {
+			/* Parse the end of the list. */
+			str = strtok_r(NULL, "]", &ctx1);
+			if (str == NULL)
+				return -1; /* no closing bracket */
+			/* Add back the bracket erased by strtok_t(). */
+			*(str - 1) = ']';
+		}
+
 		kvlist->count++;
 		str = NULL;
 	}
diff --git a/test/test/test_kvargs.c b/test/test/test_kvargs.c
index e6738624e..cb38b385c 100644
--- a/test/test/test_kvargs.c
+++ b/test/test/test_kvargs.c
@@ -137,6 +137,22 @@ static int test_valid_kvargs(void)
 	}
 	rte_kvargs_free(kvlist);
 
+	/* third test using list as value */
+	args = "foo=[0,1],check=value2";
+	valid_keys = valid_keys_list;
+	kvlist = rte_kvargs_parse(args, valid_keys);
+	if (kvlist == NULL) {
+		printf("rte_kvargs_parse() error");
+		goto fail;
+	}
+	count = kvlist->count;
+	if (count != 2) {
+		printf("invalid count value %d\n", count);
+		rte_kvargs_free(kvlist);
+		goto fail;
+	}
+	rte_kvargs_free(kvlist);
+
 	return 0;
 
  fail:
@@ -162,6 +178,7 @@ static int test_invalid_kvargs(void)
 		"foo=1,,foo=2",    /* empty key/value */
 		"foo=1,foo",       /* no value */
 		"foo=1,=2",        /* no key */
+		"foo=[1,2",        /* no closing bracket in value */
 		",=",              /* also test with a smiley */
 		NULL };
 	const char **args;
-- 
2.19.0

Re: [PATCH 1/4] kvargs: support list value

[Gaëtan Rivet]

Hi Thomas,

On Tue, Oct 09, 2018 at 04:18:55AM +0200, Thomas Monjalon wrote:
> If a value contains a comma, rte_kvargs_tokenize() will split here.
> In order to support list syntax [a,b] as value, an extra parsing of
> the square brackets is added.
> 

Nice, I was actually planning to do this.

I think it could be useful to also support () and {}, as well as
recursive lists, but it is best to have a first version to support
representor and go from this.

> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_kvargs/rte_kvargs.c | 10 ++++++++++
>  test/test/test_kvargs.c        | 17 +++++++++++++++++
>  2 files changed, 27 insertions(+)
> 

-- 
Gaëtan Rivet
6WIND

Re: [PATCH 1/4] kvargs: support list value

[Thomas Monjalon]

09/10/2018 16:14, Gaëtan Rivet:
> Hi Thomas,
> 
> On Tue, Oct 09, 2018 at 04:18:55AM +0200, Thomas Monjalon wrote:
> > If a value contains a comma, rte_kvargs_tokenize() will split here.
> > In order to support list syntax [a,b] as value, an extra parsing of
> > the square brackets is added.
> > 
> 
> Nice, I was actually planning to do this.
> 
> I think it could be useful to also support () and {}, as well as
> recursive lists, but it is best to have a first version to support
> representor and go from this.

Yes, we have no usage of () and {} so far.

Re: [PATCH 1/4] kvargs: support list value

[Stephen Hemminger]

On Tue, 09 Oct 2018 16:31:24 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 09/10/2018 16:14, Gaëtan Rivet:
> > Hi Thomas,
> > 
> > On Tue, Oct 09, 2018 at 04:18:55AM +0200, Thomas Monjalon wrote:  
> > > If a value contains a comma, rte_kvargs_tokenize() will split here.
> > > In order to support list syntax [a,b] as value, an extra parsing of
> > > the square brackets is added.
> > >   
> > 
> > Nice, I was actually planning to do this.
> > 
> > I think it could be useful to also support () and {}, as well as
> > recursive lists, but it is best to have a first version to support
> > representor and go from this.  
> 
> Yes, we have no usage of () and {} so far.

This is getting complex enough that doing a real parser maybe necessary.
Why not lex/yacc?

Or better yet go to real syntax like JSON.

Re: [PATCH 1/4] kvargs: support list value

[Thomas Monjalon]

09/10/2018 17:11, Stephen Hemminger:
> On Tue, 09 Oct 2018 16:31:24 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 09/10/2018 16:14, Gaëtan Rivet:
> > > Hi Thomas,
> > > 
> > > On Tue, Oct 09, 2018 at 04:18:55AM +0200, Thomas Monjalon wrote:  
> > > > If a value contains a comma, rte_kvargs_tokenize() will split here.
> > > > In order to support list syntax [a,b] as value, an extra parsing of
> > > > the square brackets is added.
> > > >   
> > > 
> > > Nice, I was actually planning to do this.
> > > 
> > > I think it could be useful to also support () and {}, as well as
> > > recursive lists, but it is best to have a first version to support
> > > representor and go from this.  
> > 
> > Yes, we have no usage of () and {} so far.
> 
> This is getting complex enough that doing a real parser maybe necessary.
> Why not lex/yacc?

I don't know how much it fits with our needs for devargs.

> Or better yet go to real syntax like JSON.

JSON is not suitable for one-line string as devargs.

Re: [PATCH 1/4] kvargs: support list value

[Remy Horton]

On 09/10/2018 16:11, Stephen Hemminger wrote:
> On Tue, 09 Oct 2018 16:31:24 +0200
[..]
> This is getting complex enough that doing a real parser maybe necessary.

I thought the same thing back in April with the port representor 
patchsets. Quickly run out of things to use as delimiters otherwise.


> Why not lex/yacc?

Think Yacc/Bison are vastly overcomplex and a bit of a pain to 
integrate. The sort of config grammar needed here will most likely be 
LL(1), so fairly easy to do by hand anyway.


> Or better yet go to real syntax like JSON.
>

[PATCH 2/4] mk: remove broken check

[Thomas Monjalon]

Next patch does not pass the experimental check
because it has an (unrelated) experimental function in his context.

Cc: Neil Horman <nhorman@tuxdriver.com>

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 mk/internal/rte.compile-pre.mk | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mk/internal/rte.compile-pre.mk b/mk/internal/rte.compile-pre.mk
index a734cbbd0..acb9796fa 100644
--- a/mk/internal/rte.compile-pre.mk
+++ b/mk/internal/rte.compile-pre.mk
@@ -75,7 +75,6 @@ C_TO_O_DO = @set -e; \
 	echo $(C_TO_O_DISP); \
 	$(C_TO_O) && \
 	$(PMDINFO_TO_O) && \
-	$(CHECK_EXPERIMENTAL) && \
 	echo $(C_TO_O_CMD) > $(call obj2cmd,$(@)) && \
 	sed 's,'$@':,dep_'$@' =,' $(call obj2dep,$(@)).tmp > $(call obj2dep,$(@)) && \
 	rm -f $(call obj2dep,$(@)).tmp
-- 
2.19.0

Re: [PATCH 2/4] mk: remove broken check

[Neil Horman]

On Tue, Oct 09, 2018 at 04:18:56AM +0200, Thomas Monjalon wrote:
> Next patch does not pass the experimental check
> because it has an (unrelated) experimental function in his context.
> 
> Cc: Neil Horman <nhorman@tuxdriver.com>
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  mk/internal/rte.compile-pre.mk | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mk/internal/rte.compile-pre.mk b/mk/internal/rte.compile-pre.mk
> index a734cbbd0..acb9796fa 100644
> --- a/mk/internal/rte.compile-pre.mk
> +++ b/mk/internal/rte.compile-pre.mk
> @@ -75,7 +75,6 @@ C_TO_O_DO = @set -e; \
>  	echo $(C_TO_O_DISP); \
>  	$(C_TO_O) && \
>  	$(PMDINFO_TO_O) && \
> -	$(CHECK_EXPERIMENTAL) && \
>  	echo $(C_TO_O_CMD) > $(call obj2cmd,$(@)) && \
>  	sed 's,'$@':,dep_'$@' =,' $(call obj2dep,$(@)).tmp > $(call obj2dep,$(@)) && \
>  	rm -f $(call obj2dep,$(@)).tmp
> -- 
> 2.19.0
> 
> 

NACK.

Its a bit vague to get the meaning of your changelog, but based on the
context of the actual other patch I think what you're saying is that patch 3/4 in
this series has the __rte_experimental token in it as part of the context of a
diff, rather than as a change itself, and the check-experimental-syms script is
matching on that when it shouldn't.  The correct fix is to teach the difference
between changes and context to the check-experimental-syms.sh script, not to
break the whole thing by just removing the call to the script.


Neil

Re: [PATCH 2/4] mk: remove broken check

[Thomas Monjalon]

09/10/2018 13:43, Neil Horman:
> On Tue, Oct 09, 2018 at 04:18:56AM +0200, Thomas Monjalon wrote:
> > Next patch does not pass the experimental check
> > because it has an (unrelated) experimental function in his context.
> > 
> > Cc: Neil Horman <nhorman@tuxdriver.com>
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  mk/internal/rte.compile-pre.mk | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/mk/internal/rte.compile-pre.mk b/mk/internal/rte.compile-pre.mk
> > index a734cbbd0..acb9796fa 100644
> > --- a/mk/internal/rte.compile-pre.mk
> > +++ b/mk/internal/rte.compile-pre.mk
> > @@ -75,7 +75,6 @@ C_TO_O_DO = @set -e; \
> >  	echo $(C_TO_O_DISP); \
> >  	$(C_TO_O) && \
> >  	$(PMDINFO_TO_O) && \
> > -	$(CHECK_EXPERIMENTAL) && \
> >  	echo $(C_TO_O_CMD) > $(call obj2cmd,$(@)) && \
> >  	sed 's,'$@':,dep_'$@' =,' $(call obj2dep,$(@)).tmp > $(call obj2dep,$(@)) && \
> >  	rm -f $(call obj2dep,$(@)).tmp
> 
> NACK.

Yes, of course.
I have added this patch in the series in order to allow building the patches.

> Its a bit vague to get the meaning of your changelog, but based on the
> context of the actual other patch I think what you're saying is that patch 3/4 in
> this series has the __rte_experimental token in it as part of the context of a
> diff, rather than as a change itself, and the check-experimental-syms script is
> matching on that when it shouldn't.  The correct fix is to teach the difference
> between changes and context to the check-experimental-syms.sh script, not to
> break the whole thing by just removing the call to the script.

I agree, but I don't understand how the patch context is related
to the checks done in buildtools.
Please could you fix it? It is blocking the integration of this series.

Thanks

Re: [PATCH 2/4] mk: remove broken check

[Neil Horman]

On Tue, Oct 09, 2018 at 01:53:46PM +0200, Thomas Monjalon wrote:
> 09/10/2018 13:43, Neil Horman:
> > On Tue, Oct 09, 2018 at 04:18:56AM +0200, Thomas Monjalon wrote:
> > > Next patch does not pass the experimental check
> > > because it has an (unrelated) experimental function in his context.
> > > 
> > > Cc: Neil Horman <nhorman@tuxdriver.com>
> > > 
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > ---
> > >  mk/internal/rte.compile-pre.mk | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/mk/internal/rte.compile-pre.mk b/mk/internal/rte.compile-pre.mk
> > > index a734cbbd0..acb9796fa 100644
> > > --- a/mk/internal/rte.compile-pre.mk
> > > +++ b/mk/internal/rte.compile-pre.mk
> > > @@ -75,7 +75,6 @@ C_TO_O_DO = @set -e; \
> > >  	echo $(C_TO_O_DISP); \
> > >  	$(C_TO_O) && \
> > >  	$(PMDINFO_TO_O) && \
> > > -	$(CHECK_EXPERIMENTAL) && \
> > >  	echo $(C_TO_O_CMD) > $(call obj2cmd,$(@)) && \
> > >  	sed 's,'$@':,dep_'$@' =,' $(call obj2dep,$(@)).tmp > $(call obj2dep,$(@)) && \
> > >  	rm -f $(call obj2dep,$(@)).tmp
> > 
> > NACK.
> 
> Yes, of course.
> I have added this patch in the series in order to allow building the patches.
> 
> > Its a bit vague to get the meaning of your changelog, but based on the
> > context of the actual other patch I think what you're saying is that patch 3/4 in
> > this series has the __rte_experimental token in it as part of the context of a
> > diff, rather than as a change itself, and the check-experimental-syms script is
> > matching on that when it shouldn't.  The correct fix is to teach the difference
> > between changes and context to the check-experimental-syms.sh script, not to
> > break the whole thing by just removing the call to the script.
> 
> I agree, but I don't understand how the patch context is related
> to the checks done in buildtools.
> Please could you fix it? It is blocking the integration of this series.
> 
Yeah, I'll get to it as soon as possible, but please lead with this rather than
just dismantling the feature :)

Neil

> Thanks
> 
> 
> 

[PATCH 3/4] ethdev: move representor parsing functions

[Thomas Monjalon]

The functions for representor devargs parsing were static
in the file rte_ethdev.c.
In order to reuse them in the file rte_class_eth.c,
they are moved to the files ethdev_private.c/.h.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/ethdev_private.c | 82 ++++++++++++++++++++++++++++
 lib/librte_ethdev/ethdev_private.h |  6 +++
 lib/librte_ethdev/rte_ethdev.c     | 85 ------------------------------
 3 files changed, 88 insertions(+), 85 deletions(-)

diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c
index acc787dba..c34f3ce99 100644
--- a/lib/librte_ethdev/ethdev_private.c
+++ b/lib/librte_ethdev/ethdev_private.c
@@ -3,6 +3,7 @@
  */
 
 #include "rte_ethdev.h"
+#include "rte_ethdev_driver.h"
 #include "ethdev_private.h"
 
 uint16_t
@@ -37,3 +38,84 @@ eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
 	return NULL;
 }
 
+int
+rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
+	void *data)
+{
+	char *str_start;
+	int state;
+	int result;
+
+	if (*str != '[')
+		/* Single element, not a list */
+		return callback(str, data);
+
+	/* Sanity check, then strip the brackets */
+	str_start = &str[strlen(str) - 1];
+	if (*str_start != ']') {
+		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'", str);
+		return -EINVAL;
+	}
+	str++;
+	*str_start = '\0';
+
+	/* Process list elements */
+	state = 0;
+	while (1) {
+		if (state == 0) {
+			if (*str == '\0')
+				break;
+			if (*str != ',') {
+				str_start = str;
+				state = 1;
+			}
+		} else if (state == 1) {
+			if (*str == ',' || *str == '\0') {
+				if (str > str_start) {
+					/* Non-empty string fragment */
+					*str = '\0';
+					result = callback(str_start, data);
+					if (result < 0)
+						return result;
+				}
+				state = 0;
+			}
+		}
+		str++;
+	}
+	return 0;
+}
+
+static int
+rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
+	const uint16_t max_list)
+{
+	uint16_t lo, hi, val;
+	int result;
+
+	result = sscanf(str, "%hu-%hu", &lo, &hi);
+	if (result == 1) {
+		if (*len_list >= max_list)
+			return -ENOMEM;
+		list[(*len_list)++] = lo;
+	} else if (result == 2) {
+		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)
+			return -EINVAL;
+		for (val = lo; val <= hi; val++) {
+			if (*len_list >= max_list)
+				return -ENOMEM;
+			list[(*len_list)++] = val;
+		}
+	} else
+		return -EINVAL;
+	return 0;
+}
+
+int
+rte_eth_devargs_parse_representor_ports(char *str, void *data)
+{
+	struct rte_eth_devargs *eth_da = data;
+
+	return rte_eth_devargs_process_range(str, eth_da->representor_ports,
+		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
+}
diff --git a/lib/librte_ethdev/ethdev_private.h b/lib/librte_ethdev/ethdev_private.h
index e67cf6831..7b787bf97 100644
--- a/lib/librte_ethdev/ethdev_private.h
+++ b/lib/librte_ethdev/ethdev_private.h
@@ -25,6 +25,12 @@ struct rte_eth_dev *
 eth_find_device(const struct rte_eth_dev *_start, rte_eth_cmp_t cmp,
 		const void *data);
 
+/* Parse devargs value for representor parameter. */
+typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
+int rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
+	void *data);
+int rte_eth_devargs_parse_representor_ports(char *str, void *data);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 9aef0a943..b20082b54 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4304,8 +4304,6 @@ rte_eth_switch_domain_free(uint16_t domain_id)
 	return 0;
 }
 
-typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
-
 static int
 rte_eth_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 {
@@ -4370,89 +4368,6 @@ rte_eth_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 	}
 }
 
-static int
-rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
-	void *data)
-{
-	char *str_start;
-	int state;
-	int result;
-
-	if (*str != '[')
-		/* Single element, not a list */
-		return callback(str, data);
-
-	/* Sanity check, then strip the brackets */
-	str_start = &str[strlen(str) - 1];
-	if (*str_start != ']') {
-		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'", str);
-		return -EINVAL;
-	}
-	str++;
-	*str_start = '\0';
-
-	/* Process list elements */
-	state = 0;
-	while (1) {
-		if (state == 0) {
-			if (*str == '\0')
-				break;
-			if (*str != ',') {
-				str_start = str;
-				state = 1;
-			}
-		} else if (state == 1) {
-			if (*str == ',' || *str == '\0') {
-				if (str > str_start) {
-					/* Non-empty string fragment */
-					*str = '\0';
-					result = callback(str_start, data);
-					if (result < 0)
-						return result;
-				}
-				state = 0;
-			}
-		}
-		str++;
-	}
-	return 0;
-}
-
-static int
-rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
-	const uint16_t max_list)
-{
-	uint16_t lo, hi, val;
-	int result;
-
-	result = sscanf(str, "%hu-%hu", &lo, &hi);
-	if (result == 1) {
-		if (*len_list >= max_list)
-			return -ENOMEM;
-		list[(*len_list)++] = lo;
-	} else if (result == 2) {
-		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)
-			return -EINVAL;
-		for (val = lo; val <= hi; val++) {
-			if (*len_list >= max_list)
-				return -ENOMEM;
-			list[(*len_list)++] = val;
-		}
-	} else
-		return -EINVAL;
-	return 0;
-}
-
-
-static int
-rte_eth_devargs_parse_representor_ports(char *str, void *data)
-{
-	struct rte_eth_devargs *eth_da = data;
-
-	return rte_eth_devargs_process_range(str, eth_da->representor_ports,
-		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
-}
-
 int __rte_experimental
 rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 {
-- 
2.19.0

Re: [PATCH 3/4] ethdev: move representor parsing functions

[Andrew Rybchenko]

On 10/9/18 5:18 AM, Thomas Monjalon wrote:
> The functions for representor devargs parsing were static
> in the file rte_ethdev.c.
> In order to reuse them in the file rte_class_eth.c,
> they are moved to the files ethdev_private.c/.h.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [PATCH 3/4] ethdev: move representor parsing functions

[Remy Horton]

On 09/10/2018 03:18, Thomas Monjalon wrote:
> The functions for representor devargs parsing were static
> in the file rte_ethdev.c.
> In order to reuse them in the file rte_class_eth.c,
> they are moved to the files ethdev_private.c/.h.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

There is overlap between these functions and some of the rte_kvargs_*() 
functions. Is it feasible/desirable to combine them?

..Remy

Re: [PATCH 3/4] ethdev: move representor parsing functions

[Thomas Monjalon]

09/10/2018 14:38, Remy Horton:
> 
> On 09/10/2018 03:18, Thomas Monjalon wrote:
> > The functions for representor devargs parsing were static
> > in the file rte_ethdev.c.
> > In order to reuse them in the file rte_class_eth.c,
> > they are moved to the files ethdev_private.c/.h.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> There is overlap between these functions and some of the rte_kvargs_*() 
> functions. Is it feasible/desirable to combine them?

Yes!
This is what I wanted to suggest.
I think we should parse list and range as part of the kvargs library.
Would you be interested to rework those functions in a next release?

[PATCH 4/4] ethdev: support representor id for iterating ports

[Thomas Monjalon]

The representor id is added in rte_eth_dev_data in order to be able
to match a port with its representor id in devargs.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/i40e/i40e_vf_representor.c   |  1 +
 drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
 drivers/net/mlx5/mlx5.c                  |  4 ++-
 lib/librte_ethdev/rte_class_eth.c        | 44 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_core.h      |  2 ++
 5 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
index 24751d13c..a377c1064 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -504,6 +504,7 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 	}
 
 	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+	ethdev->data->representor_id = representor->vf_id;
 
 	/* Setting the number queues allocated to the VF */
 	ethdev->data->nb_rx_queues = vf->vsi->nb_qps;
diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c b/drivers/net/ixgbe/ixgbe_vf_representor.c
index b0fbbc49f..26e2af4f8 100644
--- a/drivers/net/ixgbe/ixgbe_vf_representor.c
+++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
@@ -192,6 +192,7 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 		return -ENODEV;
 
 	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+	ethdev->data->representor_id = representor->vf_id;
 
 	/* Set representor device ops */
 	ethdev->dev_ops = &ixgbe_vf_representor_dev_ops;
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index cf258345f..de8bab342 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1075,8 +1075,10 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 		err = ENOMEM;
 		goto error;
 	}
-	if (priv->representor)
+	if (priv->representor) {
 		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+		eth_dev->data->representor_id = priv->representor_id;
+	}
 	eth_dev->data->dev_private = priv;
 	priv->dev_data = eth_dev->data;
 	eth_dev->data->mac_addrs = priv->mac;
diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
index 58fed694b..fef431f33 100644
--- a/lib/librte_ethdev/rte_class_eth.c
+++ b/lib/librte_ethdev/rte_class_eth.c
@@ -12,13 +12,16 @@
 
 #include "rte_ethdev.h"
 #include "rte_ethdev_core.h"
+#include "rte_ethdev_driver.h"
 #include "ethdev_private.h"
 
 enum eth_params {
+	RTE_ETH_PARAM_REPRESENTOR,
 	RTE_ETH_PARAM_MAX,
 };
 
 static const char * const eth_params_keys[] = {
+	[RTE_ETH_PARAM_REPRESENTOR] = "representor",
 	[RTE_ETH_PARAM_MAX] = NULL,
 };
 
@@ -33,10 +36,44 @@ struct eth_dev_match_arg {
 		.kvlist = (k), \
 	})
 
+static int
+eth_representor_cmp(const char *key __rte_unused,
+		const char *value, void *opaque)
+{
+	int ret;
+	char *values;
+	const struct rte_eth_dev_data *data = opaque;
+	struct rte_eth_devargs representors;
+	int index;
+
+	if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
+		return -1; /* not a representor port */
+
+	/* Parse devargs representor values. */
+	values = strdup(value);
+	if (values == NULL)
+		return -1;
+	memset(&representors, 0, sizeof(representors));
+	ret = rte_eth_devargs_parse_list(values,
+			rte_eth_devargs_parse_representor_ports,
+			&representors);
+	free(values);
+	if (ret != 0)
+		return -1; /* invalid devargs value */
+
+	/* Return 0 if representor id is matching one of the values. */
+	for (index = 0; index < representors.nb_representor_ports; index++)
+		if (data->representor_id ==
+				representors.representor_ports[index])
+			return 0;
+	return -1; /* no match */
+}
+
 static int
 eth_dev_match(const struct rte_eth_dev *edev,
 	      const void *_arg)
 {
+	int ret;
 	const struct eth_dev_match_arg *arg = _arg;
 	const struct rte_kvargs *kvlist = arg->kvlist;
 
@@ -47,6 +84,13 @@ eth_dev_match(const struct rte_eth_dev *edev,
 	if (kvlist == NULL)
 		/* Empty string matches everything. */
 		return 0;
+
+	ret = rte_kvargs_process(kvlist,
+			eth_params_keys[RTE_ETH_PARAM_REPRESENTOR],
+			eth_representor_cmp, edev->data);
+	if (ret != 0)
+		return -1;
+
 	return 0;
 }
 
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 33d12b3a2..ac67fde28 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -613,6 +613,8 @@ struct rte_eth_dev_data {
 	struct rte_vlan_filter_conf vlan_filter_conf;
 	/**< VLAN filter configuration. */
 	struct rte_eth_dev_owner owner; /**< The port owner. */
+	uint16_t representor_id;
+	/**< switch specific identifier - valid if RTE_ETH_DEV_REPRESENTOR */
 } __rte_cache_aligned;
 
 /**
-- 
2.19.0

Re: [PATCH 4/4] ethdev: support representor id for iterating ports

[Andrew Rybchenko]

On 10/9/18 5:18 AM, Thomas Monjalon wrote:
> The representor id is added in rte_eth_dev_data in order to be able
> to match a port with its representor id in devargs.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

[PATCH v2 0/4] support more ethdev iterator filters

[Thomas Monjalon]

The new ethdev iterator lacks the support of filtering
by representor port id or by MAC address.


Changes in v2:
  - fix list parsing in kvargs
  - support mac= parameter


Thomas Monjalon (4):
  kvargs: support list value
  ethdev: move representor parsing functions
  ethdev: support representor id as iterator filter
  ethdev: support MAC address as iterator filter

 drivers/net/i40e/i40e_vf_representor.c   |  1 +
 drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
 drivers/net/mlx5/mlx5.c                  |  4 +-
 lib/Makefile                             |  1 +
 lib/librte_cmdline/meson.build           |  4 ++
 lib/librte_ethdev/Makefile               |  2 +-
 lib/librte_ethdev/ethdev_private.c       | 82 +++++++++++++++++++++++
 lib/librte_ethdev/ethdev_private.h       |  6 ++
 lib/librte_ethdev/meson.build            |  2 +-
 lib/librte_ethdev/rte_class_eth.c        | 80 ++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           | 85 ------------------------
 lib/librte_ethdev/rte_ethdev_core.h      |  2 +
 lib/librte_kvargs/rte_kvargs.c           | 14 ++++
 lib/meson.build                          |  5 +-
 test/test/test_kvargs.c                  | 21 ++++++
 15 files changed, 220 insertions(+), 90 deletions(-)

-- 
2.19.0

[PATCH v2 1/4] kvargs: support list value

[Thomas Monjalon]

If a value contains a comma, rte_kvargs_tokenize() will split here.
In order to support list syntax [a,b] as value, an extra parsing of
the square brackets is added.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_kvargs/rte_kvargs.c | 14 ++++++++++++++
 test/test/test_kvargs.c        | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index a28f76945..e616da351 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -44,6 +44,20 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 		    kvlist->pairs[i].value == NULL)
 			return -1;
 
+		/* Detect list [a,b] to skip comma delimiter in list. */
+		str = kvlist->pairs[i].value;
+		if (str[0] == '[') {
+			/* Find the end of the list. */
+			while (str[strlen(str) - 1] != ']') {
+				/* Restore the comma erased by strtok_r(). */
+				str[strlen(str)] = ',';
+				/* Parse until next comma. */
+				str = strtok_r(NULL, RTE_KVARGS_PAIRS_DELIM, &ctx1);
+				if (str == NULL)
+					return -1; /* no closing bracket */
+			}
+		}
+
 		kvlist->count++;
 		str = NULL;
 	}
diff --git a/test/test/test_kvargs.c b/test/test/test_kvargs.c
index e6738624e..a42056f36 100644
--- a/test/test/test_kvargs.c
+++ b/test/test/test_kvargs.c
@@ -137,6 +137,26 @@ static int test_valid_kvargs(void)
 	}
 	rte_kvargs_free(kvlist);
 
+	/* third test using list as value */
+	args = "foo=[0,1],check=value2";
+	valid_keys = valid_keys_list;
+	kvlist = rte_kvargs_parse(args, valid_keys);
+	if (kvlist == NULL) {
+		printf("rte_kvargs_parse() error");
+		goto fail;
+	}
+	if (strcmp(kvlist->pairs[0].value, "[0,1]") != 0) {
+		printf("wrong value %s", kvlist->pairs[0].value);
+		goto fail;
+	}
+	count = kvlist->count;
+	if (count != 2) {
+		printf("invalid count value %d\n", count);
+		rte_kvargs_free(kvlist);
+		goto fail;
+	}
+	rte_kvargs_free(kvlist);
+
 	return 0;
 
  fail:
@@ -162,6 +182,7 @@ static int test_invalid_kvargs(void)
 		"foo=1,,foo=2",    /* empty key/value */
 		"foo=1,foo",       /* no value */
 		"foo=1,=2",        /* no key */
+		"foo=[1,2",        /* no closing bracket in value */
 		",=",              /* also test with a smiley */
 		NULL };
 	const char **args;
-- 
2.19.0

[PATCH v2 2/4] ethdev: move representor parsing functions

[Thomas Monjalon]

The functions for representor devargs parsing were static
in the file rte_ethdev.c.
In order to reuse them in the file rte_class_eth.c,
they are moved to the files ethdev_private.c/.h.

A log is fixed by adding a missing line feed.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/ethdev_private.c | 82 ++++++++++++++++++++++++++++
 lib/librte_ethdev/ethdev_private.h |  6 +++
 lib/librte_ethdev/rte_ethdev.c     | 85 ------------------------------
 3 files changed, 88 insertions(+), 85 deletions(-)

diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c
index acc787dba..162a502fe 100644
--- a/lib/librte_ethdev/ethdev_private.c
+++ b/lib/librte_ethdev/ethdev_private.c
@@ -3,6 +3,7 @@
  */
 
 #include "rte_ethdev.h"
+#include "rte_ethdev_driver.h"
 #include "ethdev_private.h"
 
 uint16_t
@@ -37,3 +38,84 @@ eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
 	return NULL;
 }
 
+int
+rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
+	void *data)
+{
+	char *str_start;
+	int state;
+	int result;
+
+	if (*str != '[')
+		/* Single element, not a list */
+		return callback(str, data);
+
+	/* Sanity check, then strip the brackets */
+	str_start = &str[strlen(str) - 1];
+	if (*str_start != ']') {
+		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str);
+		return -EINVAL;
+	}
+	str++;
+	*str_start = '\0';
+
+	/* Process list elements */
+	state = 0;
+	while (1) {
+		if (state == 0) {
+			if (*str == '\0')
+				break;
+			if (*str != ',') {
+				str_start = str;
+				state = 1;
+			}
+		} else if (state == 1) {
+			if (*str == ',' || *str == '\0') {
+				if (str > str_start) {
+					/* Non-empty string fragment */
+					*str = '\0';
+					result = callback(str_start, data);
+					if (result < 0)
+						return result;
+				}
+				state = 0;
+			}
+		}
+		str++;
+	}
+	return 0;
+}
+
+static int
+rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
+	const uint16_t max_list)
+{
+	uint16_t lo, hi, val;
+	int result;
+
+	result = sscanf(str, "%hu-%hu", &lo, &hi);
+	if (result == 1) {
+		if (*len_list >= max_list)
+			return -ENOMEM;
+		list[(*len_list)++] = lo;
+	} else if (result == 2) {
+		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)
+			return -EINVAL;
+		for (val = lo; val <= hi; val++) {
+			if (*len_list >= max_list)
+				return -ENOMEM;
+			list[(*len_list)++] = val;
+		}
+	} else
+		return -EINVAL;
+	return 0;
+}
+
+int
+rte_eth_devargs_parse_representor_ports(char *str, void *data)
+{
+	struct rte_eth_devargs *eth_da = data;
+
+	return rte_eth_devargs_process_range(str, eth_da->representor_ports,
+		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
+}
diff --git a/lib/librte_ethdev/ethdev_private.h b/lib/librte_ethdev/ethdev_private.h
index e67cf6831..7b787bf97 100644
--- a/lib/librte_ethdev/ethdev_private.h
+++ b/lib/librte_ethdev/ethdev_private.h
@@ -25,6 +25,12 @@ struct rte_eth_dev *
 eth_find_device(const struct rte_eth_dev *_start, rte_eth_cmp_t cmp,
 		const void *data);
 
+/* Parse devargs value for representor parameter. */
+typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
+int rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
+	void *data);
+int rte_eth_devargs_parse_representor_ports(char *str, void *data);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 9ac802f3f..3062dc711 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4309,8 +4309,6 @@ rte_eth_switch_domain_free(uint16_t domain_id)
 	return 0;
 }
 
-typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
-
 static int
 rte_eth_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 {
@@ -4375,89 +4373,6 @@ rte_eth_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 	}
 }
 
-static int
-rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
-	void *data)
-{
-	char *str_start;
-	int state;
-	int result;
-
-	if (*str != '[')
-		/* Single element, not a list */
-		return callback(str, data);
-
-	/* Sanity check, then strip the brackets */
-	str_start = &str[strlen(str) - 1];
-	if (*str_start != ']') {
-		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'", str);
-		return -EINVAL;
-	}
-	str++;
-	*str_start = '\0';
-
-	/* Process list elements */
-	state = 0;
-	while (1) {
-		if (state == 0) {
-			if (*str == '\0')
-				break;
-			if (*str != ',') {
-				str_start = str;
-				state = 1;
-			}
-		} else if (state == 1) {
-			if (*str == ',' || *str == '\0') {
-				if (str > str_start) {
-					/* Non-empty string fragment */
-					*str = '\0';
-					result = callback(str_start, data);
-					if (result < 0)
-						return result;
-				}
-				state = 0;
-			}
-		}
-		str++;
-	}
-	return 0;
-}
-
-static int
-rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
-	const uint16_t max_list)
-{
-	uint16_t lo, hi, val;
-	int result;
-
-	result = sscanf(str, "%hu-%hu", &lo, &hi);
-	if (result == 1) {
-		if (*len_list >= max_list)
-			return -ENOMEM;
-		list[(*len_list)++] = lo;
-	} else if (result == 2) {
-		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)
-			return -EINVAL;
-		for (val = lo; val <= hi; val++) {
-			if (*len_list >= max_list)
-				return -ENOMEM;
-			list[(*len_list)++] = val;
-		}
-	} else
-		return -EINVAL;
-	return 0;
-}
-
-
-static int
-rte_eth_devargs_parse_representor_ports(char *str, void *data)
-{
-	struct rte_eth_devargs *eth_da = data;
-
-	return rte_eth_devargs_process_range(str, eth_da->representor_ports,
-		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
-}
-
 int __rte_experimental
 rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 {
-- 
2.19.0

[PATCH v2 3/4] ethdev: support representor id as iterator filter

[Thomas Monjalon]

The representor id is added in rte_eth_dev_data in order to be able
to match a port with its representor id in devargs.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/net/i40e/i40e_vf_representor.c   |  1 +
 drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
 drivers/net/mlx5/mlx5.c                  |  4 ++-
 lib/librte_ethdev/rte_class_eth.c        | 44 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_core.h      |  2 ++
 5 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
index 24751d13c..a377c1064 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -504,6 +504,7 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 	}
 
 	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+	ethdev->data->representor_id = representor->vf_id;
 
 	/* Setting the number queues allocated to the VF */
 	ethdev->data->nb_rx_queues = vf->vsi->nb_qps;
diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c b/drivers/net/ixgbe/ixgbe_vf_representor.c
index b0fbbc49f..26e2af4f8 100644
--- a/drivers/net/ixgbe/ixgbe_vf_representor.c
+++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
@@ -192,6 +192,7 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 		return -ENODEV;
 
 	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+	ethdev->data->representor_id = representor->vf_id;
 
 	/* Set representor device ops */
 	ethdev->dev_ops = &ixgbe_vf_representor_dev_ops;
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index cf258345f..de8bab342 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1075,8 +1075,10 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 		err = ENOMEM;
 		goto error;
 	}
-	if (priv->representor)
+	if (priv->representor) {
 		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+		eth_dev->data->representor_id = priv->representor_id;
+	}
 	eth_dev->data->dev_private = priv;
 	priv->dev_data = eth_dev->data;
 	eth_dev->data->mac_addrs = priv->mac;
diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
index 58fed694b..fca7fe4d4 100644
--- a/lib/librte_ethdev/rte_class_eth.c
+++ b/lib/librte_ethdev/rte_class_eth.c
@@ -12,13 +12,16 @@
 
 #include "rte_ethdev.h"
 #include "rte_ethdev_core.h"
+#include "rte_ethdev_driver.h"
 #include "ethdev_private.h"
 
 enum eth_params {
+	RTE_ETH_PARAM_REPRESENTOR,
 	RTE_ETH_PARAM_MAX,
 };
 
 static const char * const eth_params_keys[] = {
+	[RTE_ETH_PARAM_REPRESENTOR] = "representor",
 	[RTE_ETH_PARAM_MAX] = NULL,
 };
 
@@ -33,10 +36,44 @@ struct eth_dev_match_arg {
 		.kvlist = (k), \
 	})
 
+static int
+eth_representor_cmp(const char *key __rte_unused,
+		const char *value, void *opaque)
+{
+	int ret;
+	char *values;
+	const struct rte_eth_dev_data *data = opaque;
+	struct rte_eth_devargs representors;
+	uint16_t index;
+
+	if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
+		return -1; /* not a representor port */
+
+	/* Parse devargs representor values. */
+	values = strdup(value);
+	if (values == NULL)
+		return -1;
+	memset(&representors, 0, sizeof(representors));
+	ret = rte_eth_devargs_parse_list(values,
+			rte_eth_devargs_parse_representor_ports,
+			&representors);
+	free(values);
+	if (ret != 0)
+		return -1; /* invalid devargs value */
+
+	/* Return 0 if representor id is matching one of the values. */
+	for (index = 0; index < representors.nb_representor_ports; index++)
+		if (data->representor_id ==
+				representors.representor_ports[index])
+			return 0;
+	return -1; /* no match */
+}
+
 static int
 eth_dev_match(const struct rte_eth_dev *edev,
 	      const void *_arg)
 {
+	int ret;
 	const struct eth_dev_match_arg *arg = _arg;
 	const struct rte_kvargs *kvlist = arg->kvlist;
 
@@ -47,6 +84,13 @@ eth_dev_match(const struct rte_eth_dev *edev,
 	if (kvlist == NULL)
 		/* Empty string matches everything. */
 		return 0;
+
+	ret = rte_kvargs_process(kvlist,
+			eth_params_keys[RTE_ETH_PARAM_REPRESENTOR],
+			eth_representor_cmp, edev->data);
+	if (ret != 0)
+		return -1;
+
 	return 0;
 }
 
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 33d12b3a2..ac67fde28 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -613,6 +613,8 @@ struct rte_eth_dev_data {
 	struct rte_vlan_filter_conf vlan_filter_conf;
 	/**< VLAN filter configuration. */
 	struct rte_eth_dev_owner owner; /**< The port owner. */
+	uint16_t representor_id;
+	/**< switch specific identifier - valid if RTE_ETH_DEV_REPRESENTOR */
 } __rte_cache_aligned;
 
 /**
-- 
2.19.0

[PATCH v2 4/4] ethdev: support MAC address as iterator filter

[Thomas Monjalon]

The MAC addresses of a port can be matched with devargs.

As the conflict between rte_ether.h and netinet/ether.h is not resolved,
the MAC parsing is done with a rte_cmdline function.
As a result, cmdline library becomes a dependency of ethdev.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/Makefile                      |  1 +
 lib/librte_cmdline/meson.build    |  4 ++++
 lib/librte_ethdev/Makefile        |  2 +-
 lib/librte_ethdev/meson.build     |  2 +-
 lib/librte_ethdev/rte_class_eth.c | 36 +++++++++++++++++++++++++++++++
 lib/meson.build                   |  5 +++--
 6 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/lib/Makefile b/lib/Makefile
index 8c839425d..a264945d3 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -25,6 +25,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ethdev
 DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool librte_ring
 DEPDIRS-librte_ethdev += librte_mbuf
 DEPDIRS-librte_ethdev += librte_kvargs
+DEPDIRS-librte_ethdev += librte_cmdline
 DIRS-$(CONFIG_RTE_LIBRTE_BBDEV) += librte_bbdev
 DEPDIRS-librte_bbdev := librte_eal librte_mempool librte_mbuf
 DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += librte_cryptodev
diff --git a/lib/librte_cmdline/meson.build b/lib/librte_cmdline/meson.build
index 5741817ac..30498906c 100644
--- a/lib/librte_cmdline/meson.build
+++ b/lib/librte_cmdline/meson.build
@@ -1,6 +1,10 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2017 Intel Corporation
 
+# This library is processed before EAL
+includes = [global_inc]
+includes += include_directories('../librte_eal/common/include')
+
 version = 2
 sources = files('cmdline.c',
 	'cmdline_cirbuf.c',
diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
index e27bcd5ac..3e27ae466 100644
--- a/lib/librte_ethdev/Makefile
+++ b/lib/librte_ethdev/Makefile
@@ -12,7 +12,7 @@ CFLAGS += -DALLOW_EXPERIMENTAL_API
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 LDLIBS += -lrte_net -lrte_eal -lrte_mempool -lrte_ring
-LDLIBS += -lrte_mbuf -lrte_kvargs
+LDLIBS += -lrte_mbuf -lrte_kvargs -lrte_cmdline
 
 EXPORT_MAP := rte_ethdev_version.map
 
diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
index 6783013fd..a4d850268 100644
--- a/lib/librte_ethdev/meson.build
+++ b/lib/librte_ethdev/meson.build
@@ -26,4 +26,4 @@ headers = files('rte_ethdev.h',
 	'rte_tm.h',
 	'rte_tm_driver.h')
 
-deps += ['net', 'kvargs']
+deps += ['net', 'kvargs', 'cmdline']
diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
index fca7fe4d4..16b47c3bc 100644
--- a/lib/librte_ethdev/rte_class_eth.c
+++ b/lib/librte_ethdev/rte_class_eth.c
@@ -4,6 +4,7 @@
 
 #include <string.h>
 
+#include <cmdline_parse_etheraddr.h>
 #include <rte_class.h>
 #include <rte_compat.h>
 #include <rte_errno.h>
@@ -16,11 +17,13 @@
 #include "ethdev_private.h"
 
 enum eth_params {
+	RTE_ETH_PARAM_MAC,
 	RTE_ETH_PARAM_REPRESENTOR,
 	RTE_ETH_PARAM_MAX,
 };
 
 static const char * const eth_params_keys[] = {
+	[RTE_ETH_PARAM_MAC] = "mac",
 	[RTE_ETH_PARAM_REPRESENTOR] = "representor",
 	[RTE_ETH_PARAM_MAX] = NULL,
 };
@@ -36,6 +39,33 @@ struct eth_dev_match_arg {
 		.kvlist = (k), \
 	})
 
+static int
+eth_mac_cmp(const char *key __rte_unused,
+		const char *value, void *opaque)
+{
+	int ret;
+	struct ether_addr mac;
+	const struct rte_eth_dev_data *data = opaque;
+	struct rte_eth_dev_info dev_info;
+	uint32_t index;
+
+	/* Parse devargs MAC address. */
+	/*
+	 * cannot use ether_aton_r(value, &mac)
+	 * because of include conflict with rte_ether.h
+	 */
+	ret = cmdline_parse_etheraddr(NULL, value, &mac, sizeof(mac));
+	if (ret < 0)
+		return -1; /* invalid devargs value */
+
+	/* Return 0 if devargs MAC is matching one of the device MACs. */
+	rte_eth_dev_info_get(data->port_id, &dev_info);
+	for (index = 0; index < dev_info.max_mac_addrs; index++)
+		if (is_same_ether_addr(&mac, &data->mac_addrs[index]))
+			return 0;
+	return -1; /* no match */
+}
+
 static int
 eth_representor_cmp(const char *key __rte_unused,
 		const char *value, void *opaque)
@@ -85,6 +115,12 @@ eth_dev_match(const struct rte_eth_dev *edev,
 		/* Empty string matches everything. */
 		return 0;
 
+	ret = rte_kvargs_process(kvlist,
+			eth_params_keys[RTE_ETH_PARAM_MAC],
+			eth_mac_cmp, edev->data);
+	if (ret != 0)
+		return -1;
+
 	ret = rte_kvargs_process(kvlist,
 			eth_params_keys[RTE_ETH_PARAM_REPRESENTOR],
 			eth_representor_cmp, edev->data);
diff --git a/lib/meson.build b/lib/meson.build
index 3acc67e6e..f344ffdef 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -9,13 +9,14 @@
 # given as a dep, no need to mention ring. This is especially true for the
 # core libs which are widely reused, so their deps are kept to a minimum.
 libraries = [ 'compat', # just a header, used for versioning
-	'kvargs',
+	'cmdline', # ethdev depends on cmdline for parsing functions
+	'kvargs', # eal depends on kvargs
 	'eal', 'ring', 'mempool', 'mbuf', 'net', 'ethdev', 'pci', # core
 	'metrics', # bitrate/latency stats depends on this
 	'hash',    # efd depends on this
 	'timer',   # eventdev depends on this
 	'acl', 'bbdev', 'bitratestats', 'cfgfile',
-	'cmdline', 'compressdev', 'cryptodev',
+	'compressdev', 'cryptodev',
 	'distributor', 'efd', 'eventdev',
 	'gro', 'gso', 'ip_frag', 'jobstats',
 	'kni', 'latencystats', 'lpm', 'member',
-- 
2.19.0

[PATCH v3 0/4] support more ethdev iterator filters

[Thomas Monjalon]

The new ethdev iterator lacks the support of filtering
by representor port id or by MAC address.


Changes in v3:
  - improve comment for representor_id

Changes in v2:
  - fix list parsing in kvargs
  - support mac= parameter


Thomas Monjalon (4):
  kvargs: support list value
  ethdev: move representor parsing functions
  ethdev: support representor id as iterator filter
  ethdev: support MAC address as iterator filter

 drivers/net/i40e/i40e_vf_representor.c   |  1 +
 drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
 drivers/net/mlx5/mlx5.c                  |  4 +-
 lib/Makefile                             |  1 +
 lib/librte_cmdline/meson.build           |  4 ++
 lib/librte_ethdev/Makefile               |  2 +-
 lib/librte_ethdev/ethdev_private.c       | 82 +++++++++++++++++++++++
 lib/librte_ethdev/ethdev_private.h       |  6 ++
 lib/librte_ethdev/meson.build            |  2 +-
 lib/librte_ethdev/rte_class_eth.c        | 80 ++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           | 85 ------------------------
 lib/librte_ethdev/rte_ethdev_core.h      |  4 ++
 lib/librte_kvargs/rte_kvargs.c           | 14 ++++
 lib/meson.build                          |  5 +-
 test/test/test_kvargs.c                  | 21 ++++++
 15 files changed, 222 insertions(+), 90 deletions(-)

-- 
2.19.0

[PATCH v3 1/4] kvargs: support list value

[Thomas Monjalon]

If a value contains a comma, rte_kvargs_tokenize() will split here.
In order to support list syntax [a,b] as value, an extra parsing of
the square brackets is added.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_kvargs/rte_kvargs.c | 14 ++++++++++++++
 test/test/test_kvargs.c        | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index a28f76945..e616da351 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -44,6 +44,20 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 		    kvlist->pairs[i].value == NULL)
 			return -1;
 
+		/* Detect list [a,b] to skip comma delimiter in list. */
+		str = kvlist->pairs[i].value;
+		if (str[0] == '[') {
+			/* Find the end of the list. */
+			while (str[strlen(str) - 1] != ']') {
+				/* Restore the comma erased by strtok_r(). */
+				str[strlen(str)] = ',';
+				/* Parse until next comma. */
+				str = strtok_r(NULL, RTE_KVARGS_PAIRS_DELIM, &ctx1);
+				if (str == NULL)
+					return -1; /* no closing bracket */
+			}
+		}
+
 		kvlist->count++;
 		str = NULL;
 	}
diff --git a/test/test/test_kvargs.c b/test/test/test_kvargs.c
index e6738624e..a42056f36 100644
--- a/test/test/test_kvargs.c
+++ b/test/test/test_kvargs.c
@@ -137,6 +137,26 @@ static int test_valid_kvargs(void)
 	}
 	rte_kvargs_free(kvlist);
 
+	/* third test using list as value */
+	args = "foo=[0,1],check=value2";
+	valid_keys = valid_keys_list;
+	kvlist = rte_kvargs_parse(args, valid_keys);
+	if (kvlist == NULL) {
+		printf("rte_kvargs_parse() error");
+		goto fail;
+	}
+	if (strcmp(kvlist->pairs[0].value, "[0,1]") != 0) {
+		printf("wrong value %s", kvlist->pairs[0].value);
+		goto fail;
+	}
+	count = kvlist->count;
+	if (count != 2) {
+		printf("invalid count value %d\n", count);
+		rte_kvargs_free(kvlist);
+		goto fail;
+	}
+	rte_kvargs_free(kvlist);
+
 	return 0;
 
  fail:
@@ -162,6 +182,7 @@ static int test_invalid_kvargs(void)
 		"foo=1,,foo=2",    /* empty key/value */
 		"foo=1,foo",       /* no value */
 		"foo=1,=2",        /* no key */
+		"foo=[1,2",        /* no closing bracket in value */
 		",=",              /* also test with a smiley */
 		NULL };
 	const char **args;
-- 
2.19.0

[PATCH v3 2/4] ethdev: move representor parsing functions

[Thomas Monjalon]

The functions for representor devargs parsing were static
in the file rte_ethdev.c.
In order to reuse them in the file rte_class_eth.c,
they are moved to the files ethdev_private.c/.h.

A log is fixed by adding a missing line feed.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/ethdev_private.c | 82 ++++++++++++++++++++++++++++
 lib/librte_ethdev/ethdev_private.h |  6 +++
 lib/librte_ethdev/rte_ethdev.c     | 85 ------------------------------
 3 files changed, 88 insertions(+), 85 deletions(-)

diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c
index acc787dba..162a502fe 100644
--- a/lib/librte_ethdev/ethdev_private.c
+++ b/lib/librte_ethdev/ethdev_private.c
@@ -3,6 +3,7 @@
  */
 
 #include "rte_ethdev.h"
+#include "rte_ethdev_driver.h"
 #include "ethdev_private.h"
 
 uint16_t
@@ -37,3 +38,84 @@ eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
 	return NULL;
 }
 
+int
+rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
+	void *data)
+{
+	char *str_start;
+	int state;
+	int result;
+
+	if (*str != '[')
+		/* Single element, not a list */
+		return callback(str, data);
+
+	/* Sanity check, then strip the brackets */
+	str_start = &str[strlen(str) - 1];
+	if (*str_start != ']') {
+		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str);
+		return -EINVAL;
+	}
+	str++;
+	*str_start = '\0';
+
+	/* Process list elements */
+	state = 0;
+	while (1) {
+		if (state == 0) {
+			if (*str == '\0')
+				break;
+			if (*str != ',') {
+				str_start = str;
+				state = 1;
+			}
+		} else if (state == 1) {
+			if (*str == ',' || *str == '\0') {
+				if (str > str_start) {
+					/* Non-empty string fragment */
+					*str = '\0';
+					result = callback(str_start, data);
+					if (result < 0)
+						return result;
+				}
+				state = 0;
+			}
+		}
+		str++;
+	}
+	return 0;
+}
+
+static int
+rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
+	const uint16_t max_list)
+{
+	uint16_t lo, hi, val;
+	int result;
+
+	result = sscanf(str, "%hu-%hu", &lo, &hi);
+	if (result == 1) {
+		if (*len_list >= max_list)
+			return -ENOMEM;
+		list[(*len_list)++] = lo;
+	} else if (result == 2) {
+		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)
+			return -EINVAL;
+		for (val = lo; val <= hi; val++) {
+			if (*len_list >= max_list)
+				return -ENOMEM;
+			list[(*len_list)++] = val;
+		}
+	} else
+		return -EINVAL;
+	return 0;
+}
+
+int
+rte_eth_devargs_parse_representor_ports(char *str, void *data)
+{
+	struct rte_eth_devargs *eth_da = data;
+
+	return rte_eth_devargs_process_range(str, eth_da->representor_ports,
+		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
+}
diff --git a/lib/librte_ethdev/ethdev_private.h b/lib/librte_ethdev/ethdev_private.h
index e67cf6831..7b787bf97 100644
--- a/lib/librte_ethdev/ethdev_private.h
+++ b/lib/librte_ethdev/ethdev_private.h
@@ -25,6 +25,12 @@ struct rte_eth_dev *
 eth_find_device(const struct rte_eth_dev *_start, rte_eth_cmp_t cmp,
 		const void *data);
 
+/* Parse devargs value for representor parameter. */
+typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
+int rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
+	void *data);
+int rte_eth_devargs_parse_representor_ports(char *str, void *data);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 2b218d4a2..1c04f95ea 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4349,8 +4349,6 @@ rte_eth_switch_domain_free(uint16_t domain_id)
 	return 0;
 }
 
-typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
-
 static int
 rte_eth_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 {
@@ -4415,89 +4413,6 @@ rte_eth_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 	}
 }
 
-static int
-rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
-	void *data)
-{
-	char *str_start;
-	int state;
-	int result;
-
-	if (*str != '[')
-		/* Single element, not a list */
-		return callback(str, data);
-
-	/* Sanity check, then strip the brackets */
-	str_start = &str[strlen(str) - 1];
-	if (*str_start != ']') {
-		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'", str);
-		return -EINVAL;
-	}
-	str++;
-	*str_start = '\0';
-
-	/* Process list elements */
-	state = 0;
-	while (1) {
-		if (state == 0) {
-			if (*str == '\0')
-				break;
-			if (*str != ',') {
-				str_start = str;
-				state = 1;
-			}
-		} else if (state == 1) {
-			if (*str == ',' || *str == '\0') {
-				if (str > str_start) {
-					/* Non-empty string fragment */
-					*str = '\0';
-					result = callback(str_start, data);
-					if (result < 0)
-						return result;
-				}
-				state = 0;
-			}
-		}
-		str++;
-	}
-	return 0;
-}
-
-static int
-rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
-	const uint16_t max_list)
-{
-	uint16_t lo, hi, val;
-	int result;
-
-	result = sscanf(str, "%hu-%hu", &lo, &hi);
-	if (result == 1) {
-		if (*len_list >= max_list)
-			return -ENOMEM;
-		list[(*len_list)++] = lo;
-	} else if (result == 2) {
-		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)
-			return -EINVAL;
-		for (val = lo; val <= hi; val++) {
-			if (*len_list >= max_list)
-				return -ENOMEM;
-			list[(*len_list)++] = val;
-		}
-	} else
-		return -EINVAL;
-	return 0;
-}
-
-
-static int
-rte_eth_devargs_parse_representor_ports(char *str, void *data)
-{
-	struct rte_eth_devargs *eth_da = data;
-
-	return rte_eth_devargs_process_range(str, eth_da->representor_ports,
-		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
-}
-
 int __rte_experimental
 rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 {
-- 
2.19.0

[PATCH v3 3/4] ethdev: support representor id as iterator filter

[Thomas Monjalon]

The representor id is added in rte_eth_dev_data in order to be able
to match a port with its representor id in devargs.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/net/i40e/i40e_vf_representor.c   |  1 +
 drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
 drivers/net/mlx5/mlx5.c                  |  4 ++-
 lib/librte_ethdev/rte_class_eth.c        | 44 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_core.h      |  4 +++
 5 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
index 43fe00cca..a523e50da 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -504,6 +504,7 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 	}
 
 	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+	ethdev->data->representor_id = representor->vf_id;
 
 	/* Setting the number queues allocated to the VF */
 	ethdev->data->nb_rx_queues = vf->vsi->nb_qps;
diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c b/drivers/net/ixgbe/ixgbe_vf_representor.c
index eb9bbe5cb..917ee68fc 100644
--- a/drivers/net/ixgbe/ixgbe_vf_representor.c
+++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
@@ -192,6 +192,7 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
 		return -ENODEV;
 
 	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+	ethdev->data->representor_id = representor->vf_id;
 
 	/* Set representor device ops */
 	ethdev->dev_ops = &ixgbe_vf_representor_dev_ops;
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index b2be74b51..297cbffc0 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1084,8 +1084,10 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 		err = ENOMEM;
 		goto error;
 	}
-	if (priv->representor)
+	if (priv->representor) {
 		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
+		eth_dev->data->representor_id = priv->representor_id;
+	}
 	eth_dev->data->dev_private = priv;
 	priv->dev_data = eth_dev->data;
 	eth_dev->data->mac_addrs = priv->mac;
diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
index 58fed694b..fca7fe4d4 100644
--- a/lib/librte_ethdev/rte_class_eth.c
+++ b/lib/librte_ethdev/rte_class_eth.c
@@ -12,13 +12,16 @@
 
 #include "rte_ethdev.h"
 #include "rte_ethdev_core.h"
+#include "rte_ethdev_driver.h"
 #include "ethdev_private.h"
 
 enum eth_params {
+	RTE_ETH_PARAM_REPRESENTOR,
 	RTE_ETH_PARAM_MAX,
 };
 
 static const char * const eth_params_keys[] = {
+	[RTE_ETH_PARAM_REPRESENTOR] = "representor",
 	[RTE_ETH_PARAM_MAX] = NULL,
 };
 
@@ -33,10 +36,44 @@ struct eth_dev_match_arg {
 		.kvlist = (k), \
 	})
 
+static int
+eth_representor_cmp(const char *key __rte_unused,
+		const char *value, void *opaque)
+{
+	int ret;
+	char *values;
+	const struct rte_eth_dev_data *data = opaque;
+	struct rte_eth_devargs representors;
+	uint16_t index;
+
+	if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
+		return -1; /* not a representor port */
+
+	/* Parse devargs representor values. */
+	values = strdup(value);
+	if (values == NULL)
+		return -1;
+	memset(&representors, 0, sizeof(representors));
+	ret = rte_eth_devargs_parse_list(values,
+			rte_eth_devargs_parse_representor_ports,
+			&representors);
+	free(values);
+	if (ret != 0)
+		return -1; /* invalid devargs value */
+
+	/* Return 0 if representor id is matching one of the values. */
+	for (index = 0; index < representors.nb_representor_ports; index++)
+		if (data->representor_id ==
+				representors.representor_ports[index])
+			return 0;
+	return -1; /* no match */
+}
+
 static int
 eth_dev_match(const struct rte_eth_dev *edev,
 	      const void *_arg)
 {
+	int ret;
 	const struct eth_dev_match_arg *arg = _arg;
 	const struct rte_kvargs *kvlist = arg->kvlist;
 
@@ -47,6 +84,13 @@ eth_dev_match(const struct rte_eth_dev *edev,
 	if (kvlist == NULL)
 		/* Empty string matches everything. */
 		return 0;
+
+	ret = rte_kvargs_process(kvlist,
+			eth_params_keys[RTE_ETH_PARAM_REPRESENTOR],
+			eth_representor_cmp, edev->data);
+	if (ret != 0)
+		return -1;
+
 	return 0;
 }
 
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 9a020ce3b..8f03f83f6 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -625,6 +625,10 @@ struct rte_eth_dev_data {
 	struct rte_vlan_filter_conf vlan_filter_conf;
 			/**< VLAN filter configuration. */
 	struct rte_eth_dev_owner owner; /**< The port owner. */
+	uint16_t representor_id;
+			/**< Switch-specific identifier.
+			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
+			 */
 } __rte_cache_aligned;
 
 /**
-- 
2.19.0

[PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Thomas Monjalon]

The MAC addresses of a port can be matched with devargs.

As the conflict between rte_ether.h and netinet/ether.h is not resolved,
the MAC parsing is done with a rte_cmdline function.
As a result, cmdline library becomes a dependency of ethdev.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/Makefile                      |  1 +
 lib/librte_cmdline/meson.build    |  4 ++++
 lib/librte_ethdev/Makefile        |  2 +-
 lib/librte_ethdev/meson.build     |  2 +-
 lib/librte_ethdev/rte_class_eth.c | 36 +++++++++++++++++++++++++++++++
 lib/meson.build                   |  5 +++--
 6 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/lib/Makefile b/lib/Makefile
index 8c839425d..a264945d3 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -25,6 +25,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ethdev
 DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool librte_ring
 DEPDIRS-librte_ethdev += librte_mbuf
 DEPDIRS-librte_ethdev += librte_kvargs
+DEPDIRS-librte_ethdev += librte_cmdline
 DIRS-$(CONFIG_RTE_LIBRTE_BBDEV) += librte_bbdev
 DEPDIRS-librte_bbdev := librte_eal librte_mempool librte_mbuf
 DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += librte_cryptodev
diff --git a/lib/librte_cmdline/meson.build b/lib/librte_cmdline/meson.build
index 5741817ac..30498906c 100644
--- a/lib/librte_cmdline/meson.build
+++ b/lib/librte_cmdline/meson.build
@@ -1,6 +1,10 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2017 Intel Corporation
 
+# This library is processed before EAL
+includes = [global_inc]
+includes += include_directories('../librte_eal/common/include')
+
 version = 2
 sources = files('cmdline.c',
 	'cmdline_cirbuf.c',
diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
index e27bcd5ac..3e27ae466 100644
--- a/lib/librte_ethdev/Makefile
+++ b/lib/librte_ethdev/Makefile
@@ -12,7 +12,7 @@ CFLAGS += -DALLOW_EXPERIMENTAL_API
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
 LDLIBS += -lrte_net -lrte_eal -lrte_mempool -lrte_ring
-LDLIBS += -lrte_mbuf -lrte_kvargs
+LDLIBS += -lrte_mbuf -lrte_kvargs -lrte_cmdline
 
 EXPORT_MAP := rte_ethdev_version.map
 
diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
index 6783013fd..a4d850268 100644
--- a/lib/librte_ethdev/meson.build
+++ b/lib/librte_ethdev/meson.build
@@ -26,4 +26,4 @@ headers = files('rte_ethdev.h',
 	'rte_tm.h',
 	'rte_tm_driver.h')
 
-deps += ['net', 'kvargs']
+deps += ['net', 'kvargs', 'cmdline']
diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
index fca7fe4d4..16b47c3bc 100644
--- a/lib/librte_ethdev/rte_class_eth.c
+++ b/lib/librte_ethdev/rte_class_eth.c
@@ -4,6 +4,7 @@
 
 #include <string.h>
 
+#include <cmdline_parse_etheraddr.h>
 #include <rte_class.h>
 #include <rte_compat.h>
 #include <rte_errno.h>
@@ -16,11 +17,13 @@
 #include "ethdev_private.h"
 
 enum eth_params {
+	RTE_ETH_PARAM_MAC,
 	RTE_ETH_PARAM_REPRESENTOR,
 	RTE_ETH_PARAM_MAX,
 };
 
 static const char * const eth_params_keys[] = {
+	[RTE_ETH_PARAM_MAC] = "mac",
 	[RTE_ETH_PARAM_REPRESENTOR] = "representor",
 	[RTE_ETH_PARAM_MAX] = NULL,
 };
@@ -36,6 +39,33 @@ struct eth_dev_match_arg {
 		.kvlist = (k), \
 	})
 
+static int
+eth_mac_cmp(const char *key __rte_unused,
+		const char *value, void *opaque)
+{
+	int ret;
+	struct ether_addr mac;
+	const struct rte_eth_dev_data *data = opaque;
+	struct rte_eth_dev_info dev_info;
+	uint32_t index;
+
+	/* Parse devargs MAC address. */
+	/*
+	 * cannot use ether_aton_r(value, &mac)
+	 * because of include conflict with rte_ether.h
+	 */
+	ret = cmdline_parse_etheraddr(NULL, value, &mac, sizeof(mac));
+	if (ret < 0)
+		return -1; /* invalid devargs value */
+
+	/* Return 0 if devargs MAC is matching one of the device MACs. */
+	rte_eth_dev_info_get(data->port_id, &dev_info);
+	for (index = 0; index < dev_info.max_mac_addrs; index++)
+		if (is_same_ether_addr(&mac, &data->mac_addrs[index]))
+			return 0;
+	return -1; /* no match */
+}
+
 static int
 eth_representor_cmp(const char *key __rte_unused,
 		const char *value, void *opaque)
@@ -85,6 +115,12 @@ eth_dev_match(const struct rte_eth_dev *edev,
 		/* Empty string matches everything. */
 		return 0;
 
+	ret = rte_kvargs_process(kvlist,
+			eth_params_keys[RTE_ETH_PARAM_MAC],
+			eth_mac_cmp, edev->data);
+	if (ret != 0)
+		return -1;
+
 	ret = rte_kvargs_process(kvlist,
 			eth_params_keys[RTE_ETH_PARAM_REPRESENTOR],
 			eth_representor_cmp, edev->data);
diff --git a/lib/meson.build b/lib/meson.build
index 24351cc40..2b903fa37 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -9,13 +9,14 @@
 # given as a dep, no need to mention ring. This is especially true for the
 # core libs which are widely reused, so their deps are kept to a minimum.
 libraries = [ 'compat', # just a header, used for versioning
-	'kvargs',
+	'cmdline', # ethdev depends on cmdline for parsing functions
+	'kvargs', # eal depends on kvargs
 	'eal', 'ring', 'mempool', 'mbuf', 'net', 'ethdev', 'pci', # core
 	'metrics', # bitrate/latency stats depends on this
 	'hash',    # efd depends on this
 	'timer',   # eventdev depends on this
 	'acl', 'bbdev', 'bitratestats', 'cfgfile',
-	'cmdline', 'compressdev', 'cryptodev',
+	'compressdev', 'cryptodev',
 	'distributor', 'efd', 'eventdev',
 	'gro', 'gso', 'ip_frag', 'jobstats',
 	'kni', 'latencystats', 'lpm', 'member',
-- 
2.19.0

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Andrew Rybchenko]

On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> The MAC addresses of a port can be matched with devargs.
>
> As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> the MAC parsing is done with a rte_cmdline function.
> As a result, cmdline library becomes a dependency of ethdev.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

I'd like to share my thought about a new dependency.
Looking at cmdline I think that it is a bad and strange
dependency for kvargs. IMHO, even duplication of the
code to parse MAC address it less evil in this case.

May be it is possible to provide internal wrapper
which is implemented using ether_aton_r() and located
in a separate C file which does not include rte_ether.h etc?

Andrew.

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Thomas Monjalon]

22/10/2018 15:37, Andrew Rybchenko:
> On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > The MAC addresses of a port can be matched with devargs.
> >
> > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > the MAC parsing is done with a rte_cmdline function.
> > As a result, cmdline library becomes a dependency of ethdev.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> I'd like to share my thought about a new dependency.
> Looking at cmdline I think that it is a bad and strange
> dependency for kvargs. IMHO, even duplication of the
> code to parse MAC address it less evil in this case.

cmdline is not a dependency for kvargs.
I have added it as a dependency for ethdev.

> May be it is possible to provide internal wrapper
> which is implemented using ether_aton_r() and located
> in a separate C file which does not include rte_ether.h etc?

I raised the issue in technical board and it has been decided to fix the
conflict with libc in the next release (with Olivier's help).
So Bruce and me decided to use cmdline function in the meantime.

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Andrew Rybchenko]

On 10/22/18 5:02 PM, Thomas Monjalon wrote:
> 22/10/2018 15:37, Andrew Rybchenko:
>> On 10/22/18 4:15 PM, Thomas Monjalon wrote:
>>> The MAC addresses of a port can be matched with devargs.
>>>
>>> As the conflict between rte_ether.h and netinet/ether.h is not resolved,
>>> the MAC parsing is done with a rte_cmdline function.
>>> As a result, cmdline library becomes a dependency of ethdev.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>> I'd like to share my thought about a new dependency.
>> Looking at cmdline I think that it is a bad and strange
>> dependency for kvargs. IMHO, even duplication of the
>> code to parse MAC address it less evil in this case.
> cmdline is not a dependency for kvargs.
> I have added it as a dependency for ethdev.
>
>> May be it is possible to provide internal wrapper
>> which is implemented using ether_aton_r() and located
>> in a separate C file which does not include rte_ether.h etc?
> I raised the issue in technical board and it has been decided to fix the
> conflict with libc in the next release (with Olivier's help).
> So Bruce and me decided to use cmdline function in the meantime.

OK, I see. Thanks for explanations.

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, October 22, 2018 3:02 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; gaetan.rivet@6wind.com; ophirmu@mellanox.com; Yigit, Ferruh <ferruh.yigit@intel.com>; olivier.matz@6wind.com;
> Horton, Remy <remy.horton@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] ethdev: support MAC address as iterator filter
> 
> 22/10/2018 15:37, Andrew Rybchenko:
> > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > The MAC addresses of a port can be matched with devargs.
> > >
> > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > the MAC parsing is done with a rte_cmdline function.
> > > As a result, cmdline library becomes a dependency of ethdev.
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >
> > I'd like to share my thought about a new dependency.
> > Looking at cmdline I think that it is a bad and strange
> > dependency for kvargs. IMHO, even duplication of the
> > code to parse MAC address it less evil in this case.
> 
> cmdline is not a dependency for kvargs.
> I have added it as a dependency for ethdev.
> 
> > May be it is possible to provide internal wrapper
> > which is implemented using ether_aton_r() and located
> > in a separate C file which does not include rte_ether.h etc?
> 
> I raised the issue in technical board and it has been decided to fix the
> conflict with libc in the next release (with Olivier's help).
> So Bruce and me decided to use cmdline function in the meantime.

 As I can see, cmdline_parse_etheraddr() uses 
static struct ether_addr *
my_ether_aton(const char *a)
internally.
Why not to make it public, rename to rte_ethet_aton(),
and move into rte_net?
And use that one instead?
Later if/when we'll have name conflict with libc resolved,
It can become just a wrapper around ether_aton_r().
Konstantin 

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Thomas Monjalon]

22/10/2018 23:24, Ananyev, Konstantin:
> From: Thomas Monjalon
> > 22/10/2018 15:37, Andrew Rybchenko:
> > > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > > The MAC addresses of a port can be matched with devargs.
> > > >
> > > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > > the MAC parsing is done with a rte_cmdline function.
> > > > As a result, cmdline library becomes a dependency of ethdev.
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > I'd like to share my thought about a new dependency.
> > > Looking at cmdline I think that it is a bad and strange
> > > dependency for kvargs. IMHO, even duplication of the
> > > code to parse MAC address it less evil in this case.
> > 
> > cmdline is not a dependency for kvargs.
> > I have added it as a dependency for ethdev.
> > 
> > > May be it is possible to provide internal wrapper
> > > which is implemented using ether_aton_r() and located
> > > in a separate C file which does not include rte_ether.h etc?
> > 
> > I raised the issue in technical board and it has been decided to fix the
> > conflict with libc in the next release (with Olivier's help).
> > So Bruce and me decided to use cmdline function in the meantime.
> 
>  As I can see, cmdline_parse_etheraddr() uses 
> static struct ether_addr *
> my_ether_aton(const char *a)
> internally.
> Why not to make it public, rename to rte_ethet_aton(),
> and move into rte_net?
> And use that one instead?
> Later if/when we'll have name conflict with libc resolved,
> It can become just a wrapper around ether_aton_r().
> Konstantin

Several reasons:
	- It would be one more (useless) wrapper
	- cmdline_parse_etheraddr is already used in several places
	- ether_aton_r and my_ether_aton may have a different behaviour

When the libc conflict will be solved, I prefer replacing uses of
cmdline_parse_etheraddr one by one.

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Ananyev, Konstantin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, October 23, 2018 8:21 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org; gaetan.rivet@6wind.com; ophirmu@mellanox.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; olivier.matz@6wind.com; Horton, Remy <remy.horton@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] ethdev: support MAC address as iterator filter
> 
> 22/10/2018 23:24, Ananyev, Konstantin:
> > From: Thomas Monjalon
> > > 22/10/2018 15:37, Andrew Rybchenko:
> > > > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > > > The MAC addresses of a port can be matched with devargs.
> > > > >
> > > > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > > > the MAC parsing is done with a rte_cmdline function.
> > > > > As a result, cmdline library becomes a dependency of ethdev.
> > > > >
> > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > >
> > > > I'd like to share my thought about a new dependency.
> > > > Looking at cmdline I think that it is a bad and strange
> > > > dependency for kvargs. IMHO, even duplication of the
> > > > code to parse MAC address it less evil in this case.
> > >
> > > cmdline is not a dependency for kvargs.
> > > I have added it as a dependency for ethdev.
> > >
> > > > May be it is possible to provide internal wrapper
> > > > which is implemented using ether_aton_r() and located
> > > > in a separate C file which does not include rte_ether.h etc?
> > >
> > > I raised the issue in technical board and it has been decided to fix the
> > > conflict with libc in the next release (with Olivier's help).
> > > So Bruce and me decided to use cmdline function in the meantime.
> >
> >  As I can see, cmdline_parse_etheraddr() uses
> > static struct ether_addr *
> > my_ether_aton(const char *a)
> > internally.
> > Why not to make it public, rename to rte_ethet_aton(),
> > and move into rte_net?
> > And use that one instead?
> > Later if/when we'll have name conflict with libc resolved,
> > It can become just a wrapper around ether_aton_r().
> > Konstantin
> 
> Several reasons:
> 	- It would be one more (useless) wrapper

Well, it would be *when* will have libc naming conflict resolved.
Till that it would be pretty useful I think.

> 	- cmdline_parse_etheraddr is already used in several places


As I can see right now it is used just by bond PMD:
$ find lib drivers -type f | xargs grep -l cmdline_parse_etheraddr | grep -v librte_cmdline
drivers/net/bonding/rte_eth_bond_args.c

Again if that function is *really* used in several places to convert string to mac,
then I suppose it is an indication that rte_ether_aton() would be useful.
Without forcing cmdline dependency.

> 	- ether_aton_r and my_ether_aton may have a different behavior

Could you elaborate here?
What exactly would be different?
Both supposed to convert string to ether addr.
If our function can't do that properly, then I think it is a bug in our side.
If ether_aton_r() just supports extra formats(XXXX:XXXX:XXXX), then it
would be extension to current behavior.

> 
> When the libc conflict will be solved, I prefer replacing uses of
> cmdline_parse_etheraddr one by one.

We can do the same with rte_ether_aton() too, if we really want to.
 

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Thomas Monjalon]

23/10/2018 10:33, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 22/10/2018 23:24, Ananyev, Konstantin:
> > > From: Thomas Monjalon
> > > > 22/10/2018 15:37, Andrew Rybchenko:
> > > > > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > > > > The MAC addresses of a port can be matched with devargs.
> > > > > >
> > > > > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > > > > the MAC parsing is done with a rte_cmdline function.
> > > > > > As a result, cmdline library becomes a dependency of ethdev.
> > > > > >
> > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > >
> > > > > I'd like to share my thought about a new dependency.
> > > > > Looking at cmdline I think that it is a bad and strange
> > > > > dependency for kvargs. IMHO, even duplication of the
> > > > > code to parse MAC address it less evil in this case.
> > > >
> > > > cmdline is not a dependency for kvargs.
> > > > I have added it as a dependency for ethdev.
> > > >
> > > > > May be it is possible to provide internal wrapper
> > > > > which is implemented using ether_aton_r() and located
> > > > > in a separate C file which does not include rte_ether.h etc?
> > > >
> > > > I raised the issue in technical board and it has been decided to fix the
> > > > conflict with libc in the next release (with Olivier's help).
> > > > So Bruce and me decided to use cmdline function in the meantime.
> > >
> > >  As I can see, cmdline_parse_etheraddr() uses
> > > static struct ether_addr *
> > > my_ether_aton(const char *a)
> > > internally.
> > > Why not to make it public, rename to rte_ethet_aton(),
> > > and move into rte_net?
> > > And use that one instead?
> > > Later if/when we'll have name conflict with libc resolved,
> > > It can become just a wrapper around ether_aton_r().
> > > Konstantin
> > 
> > Several reasons:
> > 	- It would be one more (useless) wrapper
> 
> Well, it would be *when* will have libc naming conflict resolved.
> Till that it would be pretty useful I think.

It is planned to be fixed in the next release.

> > 	- cmdline_parse_etheraddr is already used in several places
> 
> As I can see right now it is used just by bond PMD:
> $ find lib drivers -type f | xargs grep -l cmdline_parse_etheraddr | grep -v librte_cmdline
> drivers/net/bonding/rte_eth_bond_args.c

It is also used in examples:

	examples/bond/main.c
	examples/ethtool/ethtool-app/ethapp.c
	examples/l3fwd/main.c
	examples/performance-thread/l3fwd-thread/main.c

> Again if that function is *really* used in several places to convert string to mac,
> then I suppose it is an indication that rte_ether_aton() would be useful.
> Without forcing cmdline dependency.

I don't like wrappers and reinventing the wheel.
If we introduce this new wrapper, then we will have to deprecate it,
and break the API to remove it.

> > 	- ether_aton_r and my_ether_aton may have a different behavior
> 
> Could you elaborate here?
> What exactly would be different?

The error path might be slightly different,
and...

> Both supposed to convert string to ether addr.
> If our function can't do that properly, then I think it is a bug in our side.
> If ether_aton_r() just supports extra formats(XXXX:XXXX:XXXX), then it
> would be extension to current behavior.

... yes there is this extension.

> > When the libc conflict will be solved, I prefer replacing uses of
> > cmdline_parse_etheraddr one by one.
> 
> We can do the same with rte_ether_aton() too, if we really want to.

At the price of breaking the API again.

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Ananyev, Konstantin]

> 
> 23/10/2018 10:33, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 22/10/2018 23:24, Ananyev, Konstantin:
> > > > From: Thomas Monjalon
> > > > > 22/10/2018 15:37, Andrew Rybchenko:
> > > > > > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > > > > > The MAC addresses of a port can be matched with devargs.
> > > > > > >
> > > > > > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > > > > > the MAC parsing is done with a rte_cmdline function.
> > > > > > > As a result, cmdline library becomes a dependency of ethdev.
> > > > > > >
> > > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > >
> > > > > > I'd like to share my thought about a new dependency.
> > > > > > Looking at cmdline I think that it is a bad and strange
> > > > > > dependency for kvargs. IMHO, even duplication of the
> > > > > > code to parse MAC address it less evil in this case.
> > > > >
> > > > > cmdline is not a dependency for kvargs.
> > > > > I have added it as a dependency for ethdev.
> > > > >
> > > > > > May be it is possible to provide internal wrapper
> > > > > > which is implemented using ether_aton_r() and located
> > > > > > in a separate C file which does not include rte_ether.h etc?
> > > > >
> > > > > I raised the issue in technical board and it has been decided to fix the
> > > > > conflict with libc in the next release (with Olivier's help).
> > > > > So Bruce and me decided to use cmdline function in the meantime.
> > > >
> > > >  As I can see, cmdline_parse_etheraddr() uses
> > > > static struct ether_addr *
> > > > my_ether_aton(const char *a)
> > > > internally.
> > > > Why not to make it public, rename to rte_ethet_aton(),
> > > > and move into rte_net?
> > > > And use that one instead?
> > > > Later if/when we'll have name conflict with libc resolved,
> > > > It can become just a wrapper around ether_aton_r().
> > > > Konstantin
> > >
> > > Several reasons:
> > > 	- It would be one more (useless) wrapper
> >
> > Well, it would be *when* will have libc naming conflict resolved.
> > Till that it would be pretty useful I think.
> 
> It is planned to be fixed in the next release.
> 
> > > 	- cmdline_parse_etheraddr is already used in several places
> >
> > As I can see right now it is used just by bond PMD:
> > $ find lib drivers -type f | xargs grep -l cmdline_parse_etheraddr | grep -v librte_cmdline
> > drivers/net/bonding/rte_eth_bond_args.c
> 
> It is also used in examples:
> 
> 	examples/bond/main.c
> 	examples/ethtool/ethtool-app/ethapp.c
> 	examples/l3fwd/main.c
> 	examples/performance-thread/l3fwd-thread/main.c
> 
> > Again if that function is *really* used in several places to convert string to mac,
> > then I suppose it is an indication that rte_ether_aton() would be useful.
> > Without forcing cmdline dependency.
> 
> I don't like wrappers and reinventing the wheel.
> If we introduce this new wrapper, then we will have to deprecate it,
> and break the API to remove it.
> 
> > > 	- ether_aton_r and my_ether_aton may have a different behavior
> >
> > Could you elaborate here?
> > What exactly would be different?
> 
> The error path might be slightly different,
> and...
> 
> > Both supposed to convert string to ether addr.
> > If our function can't do that properly, then I think it is a bug in our side.
> > If ether_aton_r() just supports extra formats(XXXX:XXXX:XXXX), then it
> > would be extension to current behavior.
> 
> ... yes there is this extension.
> 
> > > When the libc conflict will be solved, I prefer replacing uses of
> > > cmdline_parse_etheraddr one by one.
> >
> > We can do the same with rte_ether_aton() too, if we really want to.
> 
> At the price of breaking the API again.

Probably you are right - extra dependency is a less evil here.
Konstantin 

Re: [PATCH v3 4/4] ethdev: support MAC address as iterator filter

[Andrew Rybchenko]

On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> The MAC addresses of a port can be matched with devargs.
>
> As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> the MAC parsing is done with a rte_cmdline function.
> As a result, cmdline library becomes a dependency of ethdev.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [PATCH v3 0/4] support more ethdev iterator filters

[Ferruh Yigit]

On 10/22/2018 2:15 PM, Thomas Monjalon wrote:
> The new ethdev iterator lacks the support of filtering
> by representor port id or by MAC address.
> 
> 
> Changes in v3:
>   - improve comment for representor_id
> 
> Changes in v2:
>   - fix list parsing in kvargs
>   - support mac= parameter
> 
> 
> Thomas Monjalon (4):
>   kvargs: support list value
>   ethdev: move representor parsing functions
>   ethdev: support representor id as iterator filter
>   ethdev: support MAC address as iterator filter

Series applied to dpdk-next-net/master, thanks.