diff mbox series

[v2] ethdev: report error on name truncation

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

Checks

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

Commit Message

Nithin Dabilpuram Jan. 13, 2019, 3:38 p.m. UTC 
  Currently this api doesn't report error if name is
truncated and so user is not sure about uniqueness
of name. This change reports error to help user.

Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
---

v2:
Fix issue caused by rebase and also fix log message

 lib/librte_ethdev/rte_ethdev.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Wiles, Keith Jan. 13, 2019, 7:28 p.m. UTC | #1 
> On Jan 13, 2019, at 9:38 AM, Nithin Kumar Dabilpuram <ndabilpuram@marvell.com> wrote:
> 
> Currently this api doesn't report error if name is
> truncated and so user is not sure about uniqueness
> of name. This change reports error to help user.
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
> 
> v2:
> Fix issue caused by rebase and also fix log message
> 
> lib/librte_ethdev/rte_ethdev.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 9d5107d..47d4f4a 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3588,9 +3588,16 @@ 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), "eth_p%d_q%d_%s",
> +		      dev->data->port_id, queue_id, ring_name);
> +
> +	if (rc >= RTE_MEMZONE_NAMESIZE) {
> +		RTE_ETHDEV_LOG(ERR, "truncated name");
> +		rte_errno = ENAMETOOLONG;
> +		return NULL;
> +	}

I we are already returning an error here should the RTE_LOG be DEBUG and not ERR. Of course this does mean we would have to check return codes :-)
> 
> 	mz = rte_memzone_lookup(z_name);
> 	if (mz)
> -- 
> 2.8.4
> 

Regards,
Keith
Thomas Monjalon Jan. 13, 2019, 8:02 p.m. UTC | #2 
13/01/2019 20:28, Wiles, Keith:
> > On Jan 13, 2019, at 9:38 AM, Nithin Kumar Dabilpuram <ndabilpuram@marvell.com> wrote:
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > +	if (rc >= RTE_MEMZONE_NAMESIZE) {
> > +		RTE_ETHDEV_LOG(ERR, "truncated name");
> > +		rte_errno = ENAMETOOLONG;
> > +		return NULL;
> > +	}
> 
> I we are already returning an error here should the RTE_LOG be DEBUG
> and not ERR.
> Of course this does mean we would have to check return codes :-)

In the general case, we should always log the errors as RTE_LOG_ERR,
no matter it is handled and logged again at an upper level.
Don't you think so?
Wiles, Keith Jan. 13, 2019, 8:19 p.m. UTC | #3 
> On Jan 13, 2019, at 2:02 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> 13/01/2019 20:28, Wiles, Keith:
>>> On Jan 13, 2019, at 9:38 AM, Nithin Kumar Dabilpuram <ndabilpuram@marvell.com> wrote:
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> +	if (rc >= RTE_MEMZONE_NAMESIZE) {
>>> +		RTE_ETHDEV_LOG(ERR, "truncated name");
>>> +		rte_errno = ENAMETOOLONG;
>>> +		return NULL;
>>> +	}
>> 
>> I we are already returning an error here should the RTE_LOG be DEBUG
>> and not ERR.
>> Of course this does mean we would have to check return codes :-)
> 
> In the general case, we should always log the errors as RTE_LOG_ERR,
> no matter it is handled and logged again at an upper level.
> Don't you think so?

My only concern is cluttering up the console output and developers should be checking return codes, which I know we do not do sometimes in DPDK.
I think we need to do some cleaning up of DPDK and test return codes or make the function return void, but that is a different problem then this one.

If we are fine with this type of log style then we can leave it. To me is just seem redundant if we are returning a code the calling function should report the error. In some cases we will get two or more messages about the same problem depending on the call path.
> 
> 

Regards,
Keith
Thomas Monjalon Jan. 13, 2019, 9:21 p.m. UTC | #4 
13/01/2019 21:19, Wiles, Keith:
> > On Jan 13, 2019, at 2:02 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> > 13/01/2019 20:28, Wiles, Keith:
> >>> On Jan 13, 2019, at 9:38 AM, Nithin Kumar Dabilpuram <ndabilpuram@marvell.com> wrote:
> >>> --- a/lib/librte_ethdev/rte_ethdev.c
> >>> +++ b/lib/librte_ethdev/rte_ethdev.c
> >>> +	if (rc >= RTE_MEMZONE_NAMESIZE) {
> >>> +		RTE_ETHDEV_LOG(ERR, "truncated name");
> >>> +		rte_errno = ENAMETOOLONG;
> >>> +		return NULL;
> >>> +	}
> >> 
> >> I we are already returning an error here should the RTE_LOG be DEBUG
> >> and not ERR.
> >> Of course this does mean we would have to check return codes :-)
> > 
> > In the general case, we should always log the errors as RTE_LOG_ERR,
> > no matter it is handled and logged again at an upper level.
> > Don't you think so?
> 
> My only concern is cluttering up the console output and developers should be checking return codes, which I know we do not do sometimes in DPDK.
> I think we need to do some cleaning up of DPDK and test return codes or make the function return void, but that is a different problem then this one.
> 
> If we are fine with this type of log style then we can leave it. To me is just seem redundant if we are returning a code the calling function should report the error. In some cases we will get two or more messages about the same problem depending on the call path.

The log can be different at different levels.
The deepest log will give details, while other logs will give context.

The more error logs we have, the better it may be for the user.
I understand the concern about keeping logs clean, but for errors,
I don't see possible redundancy as a spam.

Anyone else think differently?
Andrew Rybchenko Jan. 14, 2019, 7:32 a.m. UTC | #5 
On 1/14/19 12:21 AM, Thomas Monjalon wrote:
> 13/01/2019 21:19, Wiles, Keith:
>>> On Jan 13, 2019, at 2:02 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
>>> 13/01/2019 20:28, Wiles, Keith:
>>>>> On Jan 13, 2019, at 9:38 AM, Nithin Kumar Dabilpuram <ndabilpuram@marvell.com> wrote:
>>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>>> +	if (rc >= RTE_MEMZONE_NAMESIZE) {
>>>>> +		RTE_ETHDEV_LOG(ERR, "truncated name");
>>>>> +		rte_errno = ENAMETOOLONG;
>>>>> +		return NULL;
>>>>> +	}
>>>> I we are already returning an error here should the RTE_LOG be DEBUG
>>>> and not ERR.
>>>> Of course this does mean we would have to check return codes :-)
>>> In the general case, we should always log the errors as RTE_LOG_ERR,
>>> no matter it is handled and logged again at an upper level.
>>> Don't you think so?
>> My only concern is cluttering up the console output and developers should be checking return codes, which I know we do not do sometimes in DPDK.
>> I think we need to do some cleaning up of DPDK and test return codes or make the function return void, but that is a different problem then this one.
>>
>> If we are fine with this type of log style then we can leave it. To me is just seem redundant if we are returning a code the calling function should report the error. In some cases we will get two or more messages about the same problem depending on the call path.
> The log can be different at different levels.
> The deepest log will give details, while other logs will give context.
>
> The more error logs we have, the better it may be for the user.
> I understand the concern about keeping logs clean, but for errors,
> I don't see possible redundancy as a spam.
>
> Anyone else think differently?

Error logs are bad if an API assumes retries to be used to resolve it
(if buffer is insufficient, increase buffer and retry). But it is not 
the case
here. Theoretically it is possible here (provide shorter name), but very
unlikely. So, I'm OK with the added error log.
Thomas Monjalon Jan. 14, 2019, 2:30 p.m. UTC | #6 
13/01/2019 16:38, Nithin Kumar Dabilpuram:
> Currently this api doesn't report error if name is
> truncated and so user is not sure about uniqueness
> of name. This change reports error to help user.
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
> +	if (rc >= RTE_MEMZONE_NAMESIZE) {
> +		RTE_ETHDEV_LOG(ERR, "truncated name");

It would be better understandable from an user perspective by saying
"ring name too long".
And it would be even better with \n at the end ;)

> +		rte_errno = ENAMETOOLONG;
> +		return NULL;
> +	}
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 9d5107d..47d4f4a 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -3588,9 +3588,16 @@  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), "eth_p%d_q%d_%s",
+		      dev->data->port_id, queue_id, ring_name);
+
+	if (rc >= RTE_MEMZONE_NAMESIZE) {
+		RTE_ETHDEV_LOG(ERR, "truncated name");
+		rte_errno = ENAMETOOLONG;
+		return NULL;
+	}
 
 	mz = rte_memzone_lookup(z_name);
 	if (mz)