[dpdk-dev] 2nd parameter of driver init function can be NULL using latest code

[dpdk-dev] 2nd parameter of driver init function can be NULL using latest code

[Tetsuya Mukawa]

Hi,

It seems after applying below patch, 2nd parameter of PMD initialization
code can be NULL when vdev option is like below.

----------------
commit c07691ae10894bb6bf284fed75829b95844eacdb

devargs: remove limit on parameters length
----------------

Here is example vdev option

--vdev 'eth_name0'
(No option after driver name case)


It seems some PMDs assumes 2nd parameter will be always not NULL, even
if there is no option after driver name.

For example, before applying the patch, here is a log of bond PMD.

$ sudo ./x86_64-native-linuxapp-gcc/app/testpmd -c f -n 1 --vdev
'eth_bond0' -- -i
EAL: Detected lcore 0 as core 0 on socket 0
EAL: Detected lcore 1 as core 1 on socket 0
EAL: Detected lcore 2 as core 2 on socket 0
EAL: Detected lcore 3 as core 3 on socket 0
EAL: Detected lcore 4 as core 4 on socket 0
EAL: Detected lcore 5 as core 5 on socket 0
EAL: Detected lcore 6 as core 6 on socket 0
EAL: Detected lcore 7 as core 7 on socket 0
EAL: Support maximum 128 logical core(s) by configuration.
EAL: Detected 8 lcore(s)
EAL: VFIO modules not all loaded, skip VFIO support...
EAL: Setting up memory...
EAL: Ask a virtual area of 0x280000000 bytes
EAL: Virtual area found at 0x7ffd40000000 (size = 0x280000000)
EAL: Requesting 10 pages of size 1024MB from socket 0
EAL: TSC frequency is ~3991438 KHz
EAL: Master core 0 is ready (tid=f7fd6840)
EAL: Initializing pmd_bond for eth_bond0
EAL: Mode must be specified only once for bonded device eth_bond0
PMD: ENICPMD trace: rte_enic_pmd_init
EAL: Core 3 is ready (tid=f58e0700)
EAL: Core 2 is ready (tid=f60e1700)
EAL: Core 1 is ready (tid=f68e2700)
EAL: PCI device 0000:02:00.0 on NUMA socket -1
EAL:   probe driver: 8086:10b9 rte_em_pmd
EAL:   0000:02:00.0 not managed by UIO driver, skipping
EAL: Error - exiting with code: 1
  Cause: No probed ethernet device


After applying.

$ sudo ./x86_64-native-linuxapp-gcc/app/testpmd -c f -n 1 --vdev
'eth_bond0' -- -i
EAL: Detected lcore 0 as core 0 on socket 0
EAL: Detected lcore 1 as core 1 on socket 0
EAL: Detected lcore 2 as core 2 on socket 0
EAL: Detected lcore 3 as core 3 on socket 0
EAL: Detected lcore 4 as core 4 on socket 0
EAL: Detected lcore 5 as core 5 on socket 0
EAL: Detected lcore 6 as core 6 on socket 0
EAL: Detected lcore 7 as core 7 on socket 0
EAL: Support maximum 128 logical core(s) by configuration.
EAL: Detected 8 lcore(s)
EAL: VFIO modules not all loaded, skip VFIO support...
EAL: Setting up memory...
EAL: Ask a virtual area of 0x280000000 bytes
EAL: Virtual area found at 0x7ffd40000000 (size = 0x280000000)
EAL: Requesting 10 pages of size 1024MB from socket 0
EAL: TSC frequency is ~3991439 KHz
EAL: Master core 0 is ready (tid=f7fd6840)
EAL: Initializing pmd_bond for eth_bond0
$

It seems error is returned in PMD code.
I am not sure this is an issue. But just in case I report it, because
behavior is changed.

Thanks,
Tetsuya

[dpdk-dev] [PATCH] devargs: restore empty devargs as ""

[David Marchand]

Following commit c07691ae1089, an implicit change has been done in the devargs
api.
This triggers problem in virtual pmds that did not check for parameters validity
as it was implicitely valid.

Fix this by restoring the empty argument as "" and add a note in the api.
Restore associated tests.

Fixes: c07691ae1089 ("devargs: remove limit on parameters length")
Reported-by: Tetsuya Mukawa <mukawa@igel.co.jp>
Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 app/test/test_devargs.c                     |    2 +-
 lib/librte_eal/common/eal_common_devargs.c  |   11 +++++++----
 lib/librte_eal/common/include/rte_devargs.h |    2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index 08fb781..f7fc59c 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -107,7 +107,7 @@ test_devargs(void)
 		devargs->pci.addr.devid != 0 ||
 		devargs->pci.addr.function != 1)
 		goto fail;
-	if (devargs->args)
+	if (!devargs->args || strcmp(devargs->args, "") != 0)
 		goto fail;
 	free_devargs_list();
 
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 3aace08..eadd719 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -73,10 +73,13 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	if (sep != NULL) {
 		sep[0] = '\0';
 		devargs->args = strdup(sep + 1);
-		if (devargs->args == NULL) {
-			RTE_LOG(ERR, EAL, "cannot allocate for devargs args\n");
-			goto fail;
-		}
+	} else {
+		devargs->args = strdup("");
+	}
+
+	if (devargs->args == NULL) {
+		RTE_LOG(ERR, EAL, "cannot allocate for devargs args\n");
+		goto fail;
 	}
 
 	switch (devargs->type) {
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 996e180..6834333 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -88,7 +88,7 @@ struct rte_devargs {
 			char drv_name[32];
 		} virtual;
 	};
-	/** Arguments string as given by user. */
+	/** Arguments string as given by user or "" for no argument. */
 	char *args;
 };
 
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH] devargs: restore empty devargs as ""

[Tetsuya Mukawa]

On 2015/02/24 18:41, David Marchand wrote:
> Following commit c07691ae1089, an implicit change has been done in the devargs
> api.
> This triggers problem in virtual pmds that did not check for parameters validity
> as it was implicitely valid.
>
> Fix this by restoring the empty argument as "" and add a note in the api.
> Restore associated tests.
>
> Fixes: c07691ae1089 ("devargs: remove limit on parameters length")
> Reported-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  app/test/test_devargs.c                     |    2 +-
>  lib/librte_eal/common/eal_common_devargs.c  |   11 +++++++----
>  lib/librte_eal/common/include/rte_devargs.h |    2 +-
>  3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
> index 08fb781..f7fc59c 100644
> --- a/app/test/test_devargs.c
> +++ b/app/test/test_devargs.c
> @@ -107,7 +107,7 @@ test_devargs(void)
>  		devargs->pci.addr.devid != 0 ||
>  		devargs->pci.addr.function != 1)
>  		goto fail;
> -	if (devargs->args)
> +	if (!devargs->args || strcmp(devargs->args, "") != 0)
>  		goto fail;
>  	free_devargs_list();
>  
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 3aace08..eadd719 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -73,10 +73,13 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>  	if (sep != NULL) {
>  		sep[0] = '\0';
>  		devargs->args = strdup(sep + 1);
> -		if (devargs->args == NULL) {
> -			RTE_LOG(ERR, EAL, "cannot allocate for devargs args\n");
> -			goto fail;
> -		}
> +	} else {
> +		devargs->args = strdup("");
> +	}
> +
> +	if (devargs->args == NULL) {
> +		RTE_LOG(ERR, EAL, "cannot allocate for devargs args\n");
> +		goto fail;
>  	}
>  
>  	switch (devargs->type) {
> diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> index 996e180..6834333 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -88,7 +88,7 @@ struct rte_devargs {
>  			char drv_name[32];
>  		} virtual;
>  	};
> -	/** Arguments string as given by user. */
> +	/** Arguments string as given by user or "" for no argument. */
>  	char *args;
>  };
>  

Hi David,

I've confirmed this patch fixes the issue descried in above comment.

Thanks,
Tetsuya

Re: [dpdk-dev] [PATCH] devargs: restore empty devargs as ""

[Thomas Monjalon]

2015-02-24 19:16, Tetsuya Mukawa:
> On 2015/02/24 18:41, David Marchand wrote:
> > Following commit c07691ae1089, an implicit change has been done in the devargs
> > api.
> > This triggers problem in virtual pmds that did not check for parameters validity
> > as it was implicitely valid.
> >
> > Fix this by restoring the empty argument as "" and add a note in the api.
> > Restore associated tests.
> >
> > Fixes: c07691ae1089 ("devargs: remove limit on parameters length")
> > Reported-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> > Signed-off-by: David Marchand <david.marchand@6wind.com>
> 
> Hi David,
> 
> I've confirmed this patch fixes the issue descried in above comment.

Applied, thanks