[v2] ethdev: report error on name truncation
Checks
Commit Message
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
> 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
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?
> 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
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?
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.
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;
> + }
@@ -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)