diff mbox series

ethdev: report error on name truncation

Message ID 20190107143951.30076-1-ndabilpuram@marvell.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: report error on name truncation |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Nithin Dabilpuram Jan. 7, 2019, 2:40 p.m. UTC 
  Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
---
 lib/librte_ethdev/rte_ethdev.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Andrew Rybchenko Jan. 7, 2019, 2:47 p.m. UTC | #1 
On 1/7/19 5:40 PM, Nithin Kumar Dabilpuram wrote:
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 9d5107d..bd45445 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3588,9 +3588,17 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
>   {
>   	char z_name[RTE_MEMZONE_NAMESIZE];
>   	const struct rte_memzone *mz;
> +	int rc;
>   
> -	snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
> -		 dev->data->port_id, queue_id, ring_name);
> +	rc = snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> +		      dev->device->driver->name, ring_name,
> +		      dev->data->port_id, queue_id);
> +
> +	if (rc >= RTE_MEMZONE_NAMESIZE) {
> +		RTE_LOG(ERR, EAL, "%s(): truncated name\n", __func__);
> +		rte_errno = ENAMETOOLONG;
> +		return NULL;
> +	}
>   
>   	mz = rte_memzone_lookup(z_name);
>   	if (mz)

It is good to report an error in the case of name truncation, but the patch
does more. It changes format of the memzone name, adds the driver name
in it (what is bad since testpmd has commands to find the memzone by name
and read descriptors (hack, but sometimes very useful)).
Also I'm not sure about function name in the log message. Other places
do not have it.
Thomas Monjalon Jan. 7, 2019, 2:50 p.m. UTC | #2 
Hi,

Please provide an explanation of the current behaviour.

07/01/2019 15:40, Nithin Kumar Dabilpuram:
> -	snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
> -		 dev->data->port_id, queue_id, ring_name);
> +	rc = snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> +		      dev->device->driver->name, ring_name,
> +		      dev->data->port_id, queue_id);

You should keep the same name format.
Please check this commit:
	http://git.dpdk.org/dpdk/commit/?id=5e046832
Stephen Hemminger Jan. 7, 2019, 3:53 p.m. UTC | #3 
On Mon, 7 Jan 2019 17:47:08 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 1/7/19 5:40 PM, Nithin Kumar Dabilpuram wrote:
> > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > ---
> >   lib/librte_ethdev/rte_ethdev.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index 9d5107d..bd45445 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -3588,9 +3588,17 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
> >   {
> >   	char z_name[RTE_MEMZONE_NAMESIZE];
> >   	const struct rte_memzone *mz;
> > +	int rc;
> >   
> > -	snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
> > -		 dev->data->port_id, queue_id, ring_name);
> > +	rc = snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> > +		      dev->device->driver->name, ring_name,
> > +		      dev->data->port_id, queue_id);
> > +
> > +	if (rc >= RTE_MEMZONE_NAMESIZE) {
> > +		RTE_LOG(ERR, EAL, "%s(): truncated name\n", __func__);
> > +		rte_errno = ENAMETOOLONG;
> > +		return NULL;
> > +	}
> >   
> >   	mz = rte_memzone_lookup(z_name);
> >   	if (mz)  
> 
> It is good to report an error in the case of name truncation, but the patch
> does more. It changes format of the memzone name, adds the driver name
> in it (what is bad since testpmd has commands to find the memzone by name
> and read descriptors (hack, but sometimes very useful)).
> Also I'm not sure about function name in the log message. Other places
> do not have it.
> 

Maybe MEMZONE_NAMESIZE should be big enough that this should never happen?
The size is arbitrary anyway.
Andrew Rybchenko Jan. 7, 2019, 4:17 p.m. UTC | #4 
On 1/7/19 6:53 PM, Stephen Hemminger wrote:
> On Mon, 7 Jan 2019 17:47:08 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 1/7/19 5:40 PM, Nithin Kumar Dabilpuram wrote:
>>> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
>>> ---
>>>    lib/librte_ethdev/rte_ethdev.c | 12 ++++++++++--
>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index 9d5107d..bd45445 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -3588,9 +3588,17 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
>>>    {
>>>    	char z_name[RTE_MEMZONE_NAMESIZE];
>>>    	const struct rte_memzone *mz;
>>> +	int rc;
>>>    
>>> -	snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
>>> -		 dev->data->port_id, queue_id, ring_name);
>>> +	rc = snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
>>> +		      dev->device->driver->name, ring_name,
>>> +		      dev->data->port_id, queue_id);
>>> +
>>> +	if (rc >= RTE_MEMZONE_NAMESIZE) {
>>> +		RTE_LOG(ERR, EAL, "%s(): truncated name\n", __func__);
>>> +		rte_errno = ENAMETOOLONG;
>>> +		return NULL;
>>> +	}
>>>    
>>>    	mz = rte_memzone_lookup(z_name);
>>>    	if (mz)
>> It is good to report an error in the case of name truncation, but the patch
>> does more. It changes format of the memzone name, adds the driver name
>> in it (what is bad since testpmd has commands to find the memzone by name
>> and read descriptors (hack, but sometimes very useful)).
>> Also I'm not sure about function name in the log message. Other places
>> do not have it.
>>
> Maybe MEMZONE_NAMESIZE should be big enough that this should never happen?
> The size is arbitrary anyway.

ring_name is provided by caller, so we can't guarantee it locally.
Nithin Dabilpuram Jan. 8, 2019, 5:32 a.m. UTC | #5 
Hi Thomas,

Sorry, missed to notice that format changed by your commit recently got overridden 
during some of our internal branch merging. I'll send a v2 with same format.

--
Thanks
Nithin



From: Thomas Monjalon <thomas@monjalon.net>
Sent: Monday, January 7, 2019 8:20 PM
To: Nithin Kumar Dabilpuram
Cc: Ferruh Yigit; Andrew Rybchenko; dev@dpdk.org; Jerin Jacob Kollanukkaran
Subject: [EXT] Re: [PATCH] ethdev: report error on name truncation
  

External Email

----------------------------------------------------------------------
Hi,

Please provide an explanation of the current behaviour.

07/01/2019 15:40, Nithin Kumar Dabilpuram:
> -     snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
> -              dev->data->port_id, queue_id, ring_name);
> +     rc = snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> +                   dev->device->driver->name, ring_name,
> +                   dev->data->port_id, queue_id);

You should keep the same name format.
Please check this commit:
        http://git.dpdk.org/dpdk/commit/?id=5e046832
Nithin Dabilpuram Jan. 8, 2019, 6:06 a.m. UTC | #6 
Sure. Format change was unintentional and happened during internal merge, I’ll fix that and remove function name and send V2.

--
Thanks
Nithin

From: Andrew Rybchenko <arybchenko@solarflare.com>
Sent: Monday, January 7, 2019 8:17 PM
To: Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Subject: [EXT] Re: [dpdk-dev] [PATCH] ethdev: report error on name truncation

External Email
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 9d5107d..bd45445 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -3588,9 +3588,17 @@  rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
 {
 	char z_name[RTE_MEMZONE_NAMESIZE];
 	const struct rte_memzone *mz;
+	int rc;
 
-	snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
-		 dev->data->port_id, queue_id, ring_name);
+	rc = snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
+		      dev->device->driver->name, ring_name,
+		      dev->data->port_id, queue_id);
+
+	if (rc >= RTE_MEMZONE_NAMESIZE) {
+		RTE_LOG(ERR, EAL, "%s(): truncated name\n", __func__);
+		rte_errno = ENAMETOOLONG;
+		return NULL;
+	}
 
 	mz = rte_memzone_lookup(z_name);
 	if (mz)