patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] net/mlx5: fix error number handling
@ 2018-06-04 17:37 Yongseok Koh
  2018-06-05  6:52 ` Nélio Laranjeiro
  2018-06-19 23:13 ` [dpdk-stable] [PATCH v2] " Yongseok Koh
  0 siblings, 2 replies; 13+ messages in thread
From: Yongseok Koh @ 2018-06-04 17:37 UTC (permalink / raw)
  To: adrien.mazarguil, nelio.laranjeiro; +Cc: dev, shahafs, Yongseok Koh, stable

rte_errno should be saved only if error has occurred because rte_errno
could have garbage value.

Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
Cc: stable@dpdk.org

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 994be05be..eaffe7495 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
 		/* The flow does not match. */
 		continue;
 	}
-	ret = rte_errno; /* Save rte_errno before cleanup. */
+	if (ret)
+		ret = rte_errno; /* Save rte_errno before cleanup. */
 	if (flow)
 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
 exit:
-- 
2.11.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-04 17:37 [dpdk-stable] [PATCH] net/mlx5: fix error number handling Yongseok Koh
@ 2018-06-05  6:52 ` Nélio Laranjeiro
  2018-06-05 21:36   ` Yongseok Koh
  2018-06-19 23:13 ` [dpdk-stable] [PATCH v2] " Yongseok Koh
  1 sibling, 1 reply; 13+ messages in thread
From: Nélio Laranjeiro @ 2018-06-05  6:52 UTC (permalink / raw)
  To: Yongseok Koh; +Cc: adrien.mazarguil, dev, shahafs, stable

On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> rte_errno should be saved only if error has occurred because rte_errno
> could have garbage value.
> 
> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 994be05be..eaffe7495 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
>  		/* The flow does not match. */
>  		continue;
>  	}
> -	ret = rte_errno; /* Save rte_errno before cleanup. */
> +	if (ret)
> +		ret = rte_errno; /* Save rte_errno before cleanup. */
>  	if (flow)
>  		mlx5_flow_list_destroy(dev, &priv->flows, flow);
>  exit:
> -- 
> 2.11.0

This patch is not enough, the returned value being -rte_errno if no
error is detected by the function it cannot set rte_errno nor return it.

Thanks,

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-05  6:52 ` Nélio Laranjeiro
@ 2018-06-05 21:36   ` Yongseok Koh
  2018-06-06  6:55     ` Nélio Laranjeiro
  0 siblings, 1 reply; 13+ messages in thread
From: Yongseok Koh @ 2018-06-05 21:36 UTC (permalink / raw)
  To: Nélio Laranjeiro
  Cc: Adrien Mazarguil, dev, Shahaf Shuler, dpdk stable, Xueming(Steven) Li

> On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> 
> On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
>> rte_errno should be saved only if error has occurred because rte_errno
>> could have garbage value.
>> 
>> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
>> Cc: stable@dpdk.org
>> 
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>> ---
>> drivers/net/mlx5/mlx5_flow.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
>> index 994be05be..eaffe7495 100644
>> --- a/drivers/net/mlx5/mlx5_flow.c
>> +++ b/drivers/net/mlx5/mlx5_flow.c
>> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
>> 		/* The flow does not match. */
>> 		continue;
>> 	}
>> -	ret = rte_errno; /* Save rte_errno before cleanup. */
>> +	if (ret)
>> +		ret = rte_errno; /* Save rte_errno before cleanup. */
>> 	if (flow)
>> 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
>> exit:
>> -- 
>> 2.11.0
> 
> This patch is not enough, the returned value being -rte_errno if no
> error is detected by the function it cannot set rte_errno nor return it.

We may need to refactor this kind of code (saving and restoring rte_errno). I
still don't understand why we should preserve rte_errno like this.

Even if this function returns success, there's no obligation to preserve
rte_errno in the function. Once it is called, the ownership of rte_errno belongs
to this function. I can't find how we define this per-lcore variable but, from
the man page of errno,

       The  <errno.h>  header  file  defines  the integer variable errno, which
       is set by system calls and some library functions in the event of an
       error to indicate what went wrong.  Its value is significant only when
       the return value of the call indicated an error (i.e., -1 from most
       system calls; -1 or NULL from most library  functions);
       a function that succeeds is allowed to change errno.

So, I still think an API can change rte_errno even if it succeeds, no need to
preserve it. If needed, the caller has to save it.


Thanks,
Yongseok


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-05 21:36   ` Yongseok Koh
@ 2018-06-06  6:55     ` Nélio Laranjeiro
  2018-06-06 18:39       ` Yongseok Koh
  0 siblings, 1 reply; 13+ messages in thread
From: Nélio Laranjeiro @ 2018-06-06  6:55 UTC (permalink / raw)
  To: Yongseok Koh
  Cc: Adrien Mazarguil, dev, Shahaf Shuler, dpdk stable, Xueming(Steven) Li

On Tue, Jun 05, 2018 at 09:36:32PM +0000, Yongseok Koh wrote:
> > On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> > 
> > On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> >> rte_errno should be saved only if error has occurred because rte_errno
> >> could have garbage value.
> >> 
> >> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> >> Cc: stable@dpdk.org
> >> 
> >> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> >> ---
> >> drivers/net/mlx5/mlx5_flow.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> >> index 994be05be..eaffe7495 100644
> >> --- a/drivers/net/mlx5/mlx5_flow.c
> >> +++ b/drivers/net/mlx5/mlx5_flow.c
> >> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> >> 		/* The flow does not match. */
> >> 		continue;
> >> 	}
> >> -	ret = rte_errno; /* Save rte_errno before cleanup. */
> >> +	if (ret)
> >> +		ret = rte_errno; /* Save rte_errno before cleanup. */
> >> 	if (flow)
> >> 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
> >> exit:
> >> -- 
> >> 2.11.0
> > 
> > This patch is not enough, the returned value being -rte_errno if no
> > error is detected by the function it cannot set rte_errno nor return it.
> 
> We may need to refactor this kind of code (saving and restoring rte_errno). I
> still don't understand why we should preserve rte_errno like this.
>
> Even if this function returns success, there's no obligation to preserve
> rte_errno in the function. Once it is called, the ownership of rte_errno belongs
> to this function.
>
> I can't find how we define this per-lcore variable but, from
> the man page of errno,
> 
>        The  <errno.h>  header  file  defines  the integer variable errno, which
>        is set by system calls and some library functions in the event of an
>        error to indicate what went wrong.  Its value is significant only when
>        the return value of the call indicated an error (i.e., -1 from most
>        system calls; -1 or NULL from most library  functions);
>        a function that succeeds is allowed to change errno.
>
> So, I still think an API can change rte_errno even if it succeeds, no need to
> preserve it. If needed, the caller has to save it.

Functions in this PMD are defined as is:

  * @return
  *   0 on success, a negative errno value otherwise and rte_errno is set.

Which means rte_errno is only modified in case of error.

This fix does not respect the documentation of the function or any other
function of the PMD which can return errors.  rte_errno is only set if
an error is encountered and contains only the error code of the first
error sub-sequent ones are considered consequences of the first one and
thus not preserved.

Not preserving the rte_errno in roll backs is equivalent to not setting
it at all as a function called by the rollback may also set it, example:

 {
    void * a;

    foo_do();
    a  = malloc(10);
    if (!a) {
    	rte_errno = ENOMEM;
	foo_undo();
	return -rte_errno;
    }
 }

If foo_undo() also encounter an error it will modify the rte_errno which
may have a value different from ENOMEM, for the callee won't be informed
the error is due to a memory issue and thus cannot make counter parts.
In such situation the rte_errno must be preserved to keep the ENOMEM
error code.
This is also the main reason almost all system function only update
errno when no error is encountered.

Regards,

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-06  6:55     ` Nélio Laranjeiro
@ 2018-06-06 18:39       ` Yongseok Koh
  2018-06-07  7:39         ` Nélio Laranjeiro
  0 siblings, 1 reply; 13+ messages in thread
From: Yongseok Koh @ 2018-06-06 18:39 UTC (permalink / raw)
  To: Nélio Laranjeiro
  Cc: Adrien Mazarguil, dev, Shahaf Shuler, dpdk stable, Xueming(Steven) Li

On Wed, Jun 06, 2018 at 08:55:01AM +0200, Nélio Laranjeiro wrote:
> On Tue, Jun 05, 2018 at 09:36:32PM +0000, Yongseok Koh wrote:
> > > On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> > > 
> > > On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> > >> rte_errno should be saved only if error has occurred because rte_errno
> > >> could have garbage value.
> > >> 
> > >> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> > >> Cc: stable@dpdk.org
> > >> 
> > >> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > >> ---
> > >> drivers/net/mlx5/mlx5_flow.c | 3 ++-
> > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> > >> index 994be05be..eaffe7495 100644
> > >> --- a/drivers/net/mlx5/mlx5_flow.c
> > >> +++ b/drivers/net/mlx5/mlx5_flow.c
> > >> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> > >> 		/* The flow does not match. */
> > >> 		continue;
> > >> 	}
> > >> -	ret = rte_errno; /* Save rte_errno before cleanup. */
> > >> +	if (ret)
> > >> +		ret = rte_errno; /* Save rte_errno before cleanup. */
> > >> 	if (flow)
> > >> 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
> > >> exit:
> > >> -- 
> > >> 2.11.0
> > > 
> > > This patch is not enough, the returned value being -rte_errno if no
> > > error is detected by the function it cannot set rte_errno nor return it.
> > 
> > We may need to refactor this kind of code (saving and restoring rte_errno). I
> > still don't understand why we should preserve rte_errno like this.
> >
> > Even if this function returns success, there's no obligation to preserve
> > rte_errno in the function. Once it is called, the ownership of rte_errno belongs
> > to this function.
> >
> > I can't find how we define this per-lcore variable but, from
> > the man page of errno,
> > 
> >        The  <errno.h>  header  file  defines  the integer variable errno, which
> >        is set by system calls and some library functions in the event of an
> >        error to indicate what went wrong.  Its value is significant only when
> >        the return value of the call indicated an error (i.e., -1 from most
> >        system calls; -1 or NULL from most library  functions);
> >        a function that succeeds is allowed to change errno.
> >
> > So, I still think an API can change rte_errno even if it succeeds, no need to
> > preserve it. If needed, the caller has to save it.
> 
> Functions in this PMD are defined as is:
> 
>   * @return
>   *   0 on success, a negative errno value otherwise and rte_errno is set.
> 
> Which means rte_errno is only modified in case of error.
> 
> This fix does not respect the documentation of the function or any other
> function of the PMD which can return errors.

That's logically a wrong interpretation. According to the description, if
returning error, rte_errno is set but the opposite isn't always true. Even if
rte_errno is set, it doesn't mean there's an error. So the description coincides
with that of errno. If you want to enforce preserving rte_errno in case of
success, you should amend the documentation.

> rte_errno is only set if an error is encountered and contains only the error
> code of the first error sub-sequent ones are considered consequences of the
> first one and thus not preserved.
> 
> Not preserving the rte_errno in roll backs is equivalent to not setting
> it at all as a function called by the rollback may also set it, example:
> 
>  {
>     void * a;
> 
>     foo_do();
>     a  = malloc(10);
>     if (!a) {
>     	rte_errno = ENOMEM;
> 	foo_undo();

This example is weird. You can simply set rte_errno after foo_undo() in this
case.

> 	return -rte_errno;
>     }
>  }
> 
> If foo_undo() also encounter an error it will modify the rte_errno which
> may have a value different from ENOMEM, for the callee won't be informed
> the error is due to a memory issue and thus cannot make counter parts.
> In such situation the rte_errno must be preserved to keep the ENOMEM
> error code.

I knew it. That's why rte_errno is saved before calling another API which may
change the rte_errno inside. But, we are talking about a case where an API
returns success. If caller is supposed to save rte_errno (when it's needed), why
does callee have to put some effort to preserve it even in case of success? If
rte_errno must be preserved even in case of success, we have to make a big
change to preserve rte_errno for cases where a void function is called (or cases
where we don't check its return value of non-void function).

> This is also the main reason almost all system function only update
> errno when no error is encountered.

'Almost' doesn't mean 'all", does it? It is true that such functions must update
errno when it returns error but it doesn't care about the value when it returns
success. Like the man page I attached above, the errno is significant only when
it returns an error. And "a function that succeeds is allowed to change errno."


So, the decision point is whether we want to preserve rte_errno in case of
success? My opinion is no.


Thanks,
Yongseok

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-06 18:39       ` Yongseok Koh
@ 2018-06-07  7:39         ` Nélio Laranjeiro
  2018-06-18 17:06           ` Yongseok Koh
  0 siblings, 1 reply; 13+ messages in thread
From: Nélio Laranjeiro @ 2018-06-07  7:39 UTC (permalink / raw)
  To: Yongseok Koh
  Cc: Adrien Mazarguil, dev, Shahaf Shuler, dpdk stable, Xueming(Steven) Li

On Wed, Jun 06, 2018 at 11:39:27AM -0700, Yongseok Koh wrote:
> On Wed, Jun 06, 2018 at 08:55:01AM +0200, Nélio Laranjeiro wrote:
> > On Tue, Jun 05, 2018 at 09:36:32PM +0000, Yongseok Koh wrote:
> > > > On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> > > > 
> > > > On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> > > >> rte_errno should be saved only if error has occurred because rte_errno
> > > >> could have garbage value.
> > > >> 
> > > >> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> > > >> Cc: stable@dpdk.org
> > > >> 
> > > >> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > > >> ---
> > > >> drivers/net/mlx5/mlx5_flow.c | 3 ++-
> > > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >> 
> > > >> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> > > >> index 994be05be..eaffe7495 100644
> > > >> --- a/drivers/net/mlx5/mlx5_flow.c
> > > >> +++ b/drivers/net/mlx5/mlx5_flow.c
> > > >> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> > > >> 		/* The flow does not match. */
> > > >> 		continue;
> > > >> 	}
> > > >> -	ret = rte_errno; /* Save rte_errno before cleanup. */
> > > >> +	if (ret)
> > > >> +		ret = rte_errno; /* Save rte_errno before cleanup. */
> > > >> 	if (flow)
> > > >> 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
> > > >> exit:
> > > >> -- 
> > > >> 2.11.0
> > > > 
> > > > This patch is not enough, the returned value being -rte_errno if no
> > > > error is detected by the function it cannot set rte_errno nor return it.
> > > 
> > > We may need to refactor this kind of code (saving and restoring rte_errno). I
> > > still don't understand why we should preserve rte_errno like this.
> > >
> > > Even if this function returns success, there's no obligation to preserve
> > > rte_errno in the function. Once it is called, the ownership of rte_errno belongs
> > > to this function.
> > >
> > > I can't find how we define this per-lcore variable but, from
> > > the man page of errno,
> > > 
> > >        The  <errno.h>  header  file  defines  the integer variable errno, which
> > >        is set by system calls and some library functions in the event of an
> > >        error to indicate what went wrong.  Its value is significant only when
> > >        the return value of the call indicated an error (i.e., -1 from most
> > >        system calls; -1 or NULL from most library  functions);
> > >        a function that succeeds is allowed to change errno.
> > >
> > > So, I still think an API can change rte_errno even if it succeeds, no need to
> > > preserve it. If needed, the caller has to save it.
> > 
> > Functions in this PMD are defined as is:
> > 
> >   * @return
> >   *   0 on success, a negative errno value otherwise and rte_errno is set.
> > 
> > Which means rte_errno is only modified in case of error.
> > 
> > This fix does not respect the documentation of the function or any other
> > function of the PMD which can return errors.
> 
> That's logically a wrong interpretation. According to the description, if
> returning error, rte_errno is set but the opposite isn't always true. Even if
> rte_errno is set, it doesn't mean there's an error. So the description coincides
> with that of errno. If you want to enforce preserving rte_errno in case of
> success, you should amend the documentation.
> 
> > rte_errno is only set if an error is encountered and contains only the error
> > code of the first error sub-sequent ones are considered consequences of the
> > first one and thus not preserved.
> > 
> > Not preserving the rte_errno in roll backs is equivalent to not setting
> > it at all as a function called by the rollback may also set it, example:
> > 
> >  {
> >     void * a;
> > 
> >     foo_do();
> >     a  = malloc(10);
> >     if (!a) {
> >     	rte_errno = ENOMEM;
> > 	foo_undo();
> 
> This example is weird. You can simply set rte_errno after foo_undo() in this
> case.
> 
> > 	return -rte_errno;
> >     }
> >  }
> > 
> > If foo_undo() also encounter an error it will modify the rte_errno which
> > may have a value different from ENOMEM, for the callee won't be informed
> > the error is due to a memory issue and thus cannot make counter parts.
> > In such situation the rte_errno must be preserved to keep the ENOMEM
> > error code.
> 
> I knew it. That's why rte_errno is saved before calling another API which may
> change the rte_errno inside. But, we are talking about a case where an API
> returns success. If caller is supposed to save rte_errno (when it's needed), why
> does callee have to put some effort to preserve it even in case of success? If
> rte_errno must be preserved even in case of success, we have to make a big
> change to preserve rte_errno for cases where a void function is called (or cases
> where we don't check its return value of non-void function).
>
> > This is also the main reason almost all system function only update
> > errno when no error is encountered.
> 
> 'Almost' doesn't mean 'all", does it? It is true that such functions must update
> errno when it returns error but it doesn't care about the value when it returns
> success. Like the man page I attached above, the errno is significant only when
> it returns an error. And "a function that succeeds is allowed to change errno."

It is "almost" because a system function touching the errno when the
function succeed it not common.  But as the man page says it is not
impossible.

> So, the decision point is whether we want to preserve rte_errno in case of
> success? My opinion is no.

I did not understood it was only a concern about the success of the
function, even it is better to avoid as most as possible a useless
store, in this specific case, as errno (rte_errno) has a garbage value,
I fully agree with you.

Regards,

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-07  7:39         ` Nélio Laranjeiro
@ 2018-06-18 17:06           ` Yongseok Koh
  2018-06-19 11:48             ` Nélio Laranjeiro
  0 siblings, 1 reply; 13+ messages in thread
From: Yongseok Koh @ 2018-06-18 17:06 UTC (permalink / raw)
  To: Nélio Laranjeiro
  Cc: Adrien Mazarguil, dev, Shahaf Shuler, dpdk stable, Xueming(Steven) Li


> On Jun 7, 2018, at 12:39 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> 
> On Wed, Jun 06, 2018 at 11:39:27AM -0700, Yongseok Koh wrote:
>> On Wed, Jun 06, 2018 at 08:55:01AM +0200, Nélio Laranjeiro wrote:
>>> On Tue, Jun 05, 2018 at 09:36:32PM +0000, Yongseok Koh wrote:
>>>>> On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
>>>>> 
>>>>> On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
>>>>>> rte_errno should be saved only if error has occurred because rte_errno
>>>>>> could have garbage value.
>>>>>> 
>>>>>> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
>>>>>> Cc: stable@dpdk.org
>>>>>> 
>>>>>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>>>>>> ---
>>>>>> drivers/net/mlx5/mlx5_flow.c | 3 ++-
>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
>>>>>> index 994be05be..eaffe7495 100644
>>>>>> --- a/drivers/net/mlx5/mlx5_flow.c
>>>>>> +++ b/drivers/net/mlx5/mlx5_flow.c
>>>>>> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
>>>>>> 		/* The flow does not match. */
>>>>>> 		continue;
>>>>>> 	}
>>>>>> -	ret = rte_errno; /* Save rte_errno before cleanup. */
>>>>>> +	if (ret)
>>>>>> +		ret = rte_errno; /* Save rte_errno before cleanup. */
>>>>>> 	if (flow)
>>>>>> 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
>>>>>> exit:
>>>>>> -- 
>>>>>> 2.11.0
>>>>> 
>>>>> This patch is not enough, the returned value being -rte_errno if no
>>>>> error is detected by the function it cannot set rte_errno nor return it.
>>>> 
>>>> We may need to refactor this kind of code (saving and restoring rte_errno). I
>>>> still don't understand why we should preserve rte_errno like this.
>>>> 
>>>> Even if this function returns success, there's no obligation to preserve
>>>> rte_errno in the function. Once it is called, the ownership of rte_errno belongs
>>>> to this function.
>>>> 
>>>> I can't find how we define this per-lcore variable but, from
>>>> the man page of errno,
>>>> 
>>>>       The  <errno.h>  header  file  defines  the integer variable errno, which
>>>>       is set by system calls and some library functions in the event of an
>>>>       error to indicate what went wrong.  Its value is significant only when
>>>>       the return value of the call indicated an error (i.e., -1 from most
>>>>       system calls; -1 or NULL from most library  functions);
>>>>       a function that succeeds is allowed to change errno.
>>>> 
>>>> So, I still think an API can change rte_errno even if it succeeds, no need to
>>>> preserve it. If needed, the caller has to save it.
>>> 
>>> Functions in this PMD are defined as is:
>>> 
>>>  * @return
>>>  *   0 on success, a negative errno value otherwise and rte_errno is set.
>>> 
>>> Which means rte_errno is only modified in case of error.
>>> 
>>> This fix does not respect the documentation of the function or any other
>>> function of the PMD which can return errors.
>> 
>> That's logically a wrong interpretation. According to the description, if
>> returning error, rte_errno is set but the opposite isn't always true. Even if
>> rte_errno is set, it doesn't mean there's an error. So the description coincides
>> with that of errno. If you want to enforce preserving rte_errno in case of
>> success, you should amend the documentation.
>> 
>>> rte_errno is only set if an error is encountered and contains only the error
>>> code of the first error sub-sequent ones are considered consequences of the
>>> first one and thus not preserved.
>>> 
>>> Not preserving the rte_errno in roll backs is equivalent to not setting
>>> it at all as a function called by the rollback may also set it, example:
>>> 
>>> {
>>>    void * a;
>>> 
>>>    foo_do();
>>>    a  = malloc(10);
>>>    if (!a) {
>>>    	rte_errno = ENOMEM;
>>> 	foo_undo();
>> 
>> This example is weird. You can simply set rte_errno after foo_undo() in this
>> case.
>> 
>>> 	return -rte_errno;
>>>    }
>>> }
>>> 
>>> If foo_undo() also encounter an error it will modify the rte_errno which
>>> may have a value different from ENOMEM, for the callee won't be informed
>>> the error is due to a memory issue and thus cannot make counter parts.
>>> In such situation the rte_errno must be preserved to keep the ENOMEM
>>> error code.
>> 
>> I knew it. That's why rte_errno is saved before calling another API which may
>> change the rte_errno inside. But, we are talking about a case where an API
>> returns success. If caller is supposed to save rte_errno (when it's needed), why
>> does callee have to put some effort to preserve it even in case of success? If
>> rte_errno must be preserved even in case of success, we have to make a big
>> change to preserve rte_errno for cases where a void function is called (or cases
>> where we don't check its return value of non-void function).
>> 
>>> This is also the main reason almost all system function only update
>>> errno when no error is encountered.
>> 
>> 'Almost' doesn't mean 'all", does it? It is true that such functions must update
>> errno when it returns error but it doesn't care about the value when it returns
>> success. Like the man page I attached above, the errno is significant only when
>> it returns an error. And "a function that succeeds is allowed to change errno."
> 
> It is "almost" because a system function touching the errno when the
> function succeed it not common.  But as the man page says it is not
> impossible.
> 
>> So, the decision point is whether we want to preserve rte_errno in case of
>> success? My opinion is no.
> 
> I did not understood it was only a concern about the success of the
> function, even it is better to avoid as most as possible a useless
> store, in this specific case, as errno (rte_errno) has a garbage value,
> I fully agree with you.

Nelio,

Do you still want me to make any change for this patch?
Let me know if any.

Thanks,
Yongseok

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-18 17:06           ` Yongseok Koh
@ 2018-06-19 11:48             ` Nélio Laranjeiro
  2018-06-19 23:00               ` Yongseok Koh
  0 siblings, 1 reply; 13+ messages in thread
From: Nélio Laranjeiro @ 2018-06-19 11:48 UTC (permalink / raw)
  To: Yongseok Koh
  Cc: Adrien Mazarguil, dev, Shahaf Shuler, dpdk stable, Xueming(Steven) Li

On Mon, Jun 18, 2018 at 05:06:41PM +0000, Yongseok Koh wrote:
> 
> > On Jun 7, 2018, at 12:39 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> > 
> > On Wed, Jun 06, 2018 at 11:39:27AM -0700, Yongseok Koh wrote:
> >> On Wed, Jun 06, 2018 at 08:55:01AM +0200, Nélio Laranjeiro wrote:
> >>> On Tue, Jun 05, 2018 at 09:36:32PM +0000, Yongseok Koh wrote:
> >>>>> On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> >>>>> 
> >>>>> On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> >>>>>> rte_errno should be saved only if error has occurred because rte_errno
> >>>>>> could have garbage value.
> >>>>>> 
> >>>>>> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> >>>>>> Cc: stable@dpdk.org
> >>>>>> 
> >>>>>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> >>>>>> ---
> >>>>>> drivers/net/mlx5/mlx5_flow.c | 3 ++-
> >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> >>>>>> index 994be05be..eaffe7495 100644
> >>>>>> --- a/drivers/net/mlx5/mlx5_flow.c
> >>>>>> +++ b/drivers/net/mlx5/mlx5_flow.c
> >>>>>> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> >>>>>> 		/* The flow does not match. */
> >>>>>> 		continue;
> >>>>>> 	}
> >>>>>> -	ret = rte_errno; /* Save rte_errno before cleanup. */
> >>>>>> +	if (ret)
> >>>>>> +		ret = rte_errno; /* Save rte_errno before cleanup. */
> >>>>>> 	if (flow)
> >>>>>> 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
> >>>>>> exit:
> >>>>>> -- 
> >>>>>> 2.11.0
> >>>>> 
> >>>>> This patch is not enough, the returned value being -rte_errno if no
> >>>>> error is detected by the function it cannot set rte_errno nor return it.
> >>>> 
> >>>> We may need to refactor this kind of code (saving and restoring rte_errno). I
> >>>> still don't understand why we should preserve rte_errno like this.
> >>>> 
> >>>> Even if this function returns success, there's no obligation to preserve
> >>>> rte_errno in the function. Once it is called, the ownership of rte_errno belongs
> >>>> to this function.
> >>>> 
> >>>> I can't find how we define this per-lcore variable but, from
> >>>> the man page of errno,
> >>>> 
> >>>>       The  <errno.h>  header  file  defines  the integer variable errno, which
> >>>>       is set by system calls and some library functions in the event of an
> >>>>       error to indicate what went wrong.  Its value is significant only when
> >>>>       the return value of the call indicated an error (i.e., -1 from most
> >>>>       system calls; -1 or NULL from most library  functions);
> >>>>       a function that succeeds is allowed to change errno.
> >>>> 
> >>>> So, I still think an API can change rte_errno even if it succeeds, no need to
> >>>> preserve it. If needed, the caller has to save it.
> >>> 
> >>> Functions in this PMD are defined as is:
> >>> 
> >>>  * @return
> >>>  *   0 on success, a negative errno value otherwise and rte_errno is set.
> >>> 
> >>> Which means rte_errno is only modified in case of error.
> >>> 
> >>> This fix does not respect the documentation of the function or any other
> >>> function of the PMD which can return errors.
> >> 
> >> That's logically a wrong interpretation. According to the description, if
> >> returning error, rte_errno is set but the opposite isn't always true. Even if
> >> rte_errno is set, it doesn't mean there's an error. So the description coincides
> >> with that of errno. If you want to enforce preserving rte_errno in case of
> >> success, you should amend the documentation.
> >> 
> >>> rte_errno is only set if an error is encountered and contains only the error
> >>> code of the first error sub-sequent ones are considered consequences of the
> >>> first one and thus not preserved.
> >>> 
> >>> Not preserving the rte_errno in roll backs is equivalent to not setting
> >>> it at all as a function called by the rollback may also set it, example:
> >>> 
> >>> {
> >>>    void * a;
> >>> 
> >>>    foo_do();
> >>>    a  = malloc(10);
> >>>    if (!a) {
> >>>    	rte_errno = ENOMEM;
> >>> 	foo_undo();
> >> 
> >> This example is weird. You can simply set rte_errno after foo_undo() in this
> >> case.
> >> 
> >>> 	return -rte_errno;
> >>>    }
> >>> }
> >>> 
> >>> If foo_undo() also encounter an error it will modify the rte_errno which
> >>> may have a value different from ENOMEM, for the callee won't be informed
> >>> the error is due to a memory issue and thus cannot make counter parts.
> >>> In such situation the rte_errno must be preserved to keep the ENOMEM
> >>> error code.
> >> 
> >> I knew it. That's why rte_errno is saved before calling another API which may
> >> change the rte_errno inside. But, we are talking about a case where an API
> >> returns success. If caller is supposed to save rte_errno (when it's needed), why
> >> does callee have to put some effort to preserve it even in case of success? If
> >> rte_errno must be preserved even in case of success, we have to make a big
> >> change to preserve rte_errno for cases where a void function is called (or cases
> >> where we don't check its return value of non-void function).
> >> 
> >>> This is also the main reason almost all system function only update
> >>> errno when no error is encountered.
> >> 
> >> 'Almost' doesn't mean 'all", does it? It is true that such functions must update
> >> errno when it returns error but it doesn't care about the value when it returns
> >> success. Like the man page I attached above, the errno is significant only when
> >> it returns an error. And "a function that succeeds is allowed to change errno."
> > 
> > It is "almost" because a system function touching the errno when the
> > function succeed it not common.  But as the man page says it is not
> > impossible.
> > 
> >> So, the decision point is whether we want to preserve rte_errno in case of
> >> success? My opinion is no.
> > 
> > I did not understood it was only a concern about the success of the
> > function, even it is better to avoid as most as possible a useless
> > store, in this specific case, as errno (rte_errno) has a garbage value,
> > I fully agree with you.
> 
> Nelio,
> 
> Do you still want me to make any change for this patch?
> Let me know if any.

With your modification the function documentation is no more accurate as
rte_errno is always set.

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-19 11:48             ` Nélio Laranjeiro
@ 2018-06-19 23:00               ` Yongseok Koh
  2018-06-20  7:05                 ` Nélio Laranjeiro
  0 siblings, 1 reply; 13+ messages in thread
From: Yongseok Koh @ 2018-06-19 23:00 UTC (permalink / raw)
  To: Nélio Laranjeiro
  Cc: Adrien Mazarguil, dev, Shahaf Shuler, dpdk stable, Xueming(Steven) Li


> On Jun 19, 2018, at 4:48 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> 
> On Mon, Jun 18, 2018 at 05:06:41PM +0000, Yongseok Koh wrote:
>> 
>>> On Jun 7, 2018, at 12:39 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
>>> 
>>> On Wed, Jun 06, 2018 at 11:39:27AM -0700, Yongseok Koh wrote:
>>>> On Wed, Jun 06, 2018 at 08:55:01AM +0200, Nélio Laranjeiro wrote:
>>>>> On Tue, Jun 05, 2018 at 09:36:32PM +0000, Yongseok Koh wrote:
>>>>>>> On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
>>>>>>> 
>>>>>>> On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
>>>>>>>> rte_errno should be saved only if error has occurred because rte_errno
>>>>>>>> could have garbage value.
>>>>>>>> 
>>>>>>>> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>> 
>>>>>>>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>>>>>>>> ---
>>>>>>>> drivers/net/mlx5/mlx5_flow.c | 3 ++-
>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>> 
>>>>>>>> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
>>>>>>>> index 994be05be..eaffe7495 100644
>>>>>>>> --- a/drivers/net/mlx5/mlx5_flow.c
>>>>>>>> +++ b/drivers/net/mlx5/mlx5_flow.c
>>>>>>>> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
>>>>>>>> 		/* The flow does not match. */
>>>>>>>> 		continue;
>>>>>>>> 	}
>>>>>>>> -	ret = rte_errno; /* Save rte_errno before cleanup. */
>>>>>>>> +	if (ret)
>>>>>>>> +		ret = rte_errno; /* Save rte_errno before cleanup. */
>>>>>>>> 	if (flow)
>>>>>>>> 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
>>>>>>>> exit:
>>>>>>>> -- 
>>>>>>>> 2.11.0
>>>>>>> 
>>>>>>> This patch is not enough, the returned value being -rte_errno if no
>>>>>>> error is detected by the function it cannot set rte_errno nor return it.
>>>>>> 
>>>>>> We may need to refactor this kind of code (saving and restoring rte_errno). I
>>>>>> still don't understand why we should preserve rte_errno like this.
>>>>>> 
>>>>>> Even if this function returns success, there's no obligation to preserve
>>>>>> rte_errno in the function. Once it is called, the ownership of rte_errno belongs
>>>>>> to this function.
>>>>>> 
>>>>>> I can't find how we define this per-lcore variable but, from
>>>>>> the man page of errno,
>>>>>> 
>>>>>>      The  <errno.h>  header  file  defines  the integer variable errno, which
>>>>>>      is set by system calls and some library functions in the event of an
>>>>>>      error to indicate what went wrong.  Its value is significant only when
>>>>>>      the return value of the call indicated an error (i.e., -1 from most
>>>>>>      system calls; -1 or NULL from most library  functions);
>>>>>>      a function that succeeds is allowed to change errno.
>>>>>> 
>>>>>> So, I still think an API can change rte_errno even if it succeeds, no need to
>>>>>> preserve it. If needed, the caller has to save it.
>>>>> 
>>>>> Functions in this PMD are defined as is:
>>>>> 
>>>>> * @return
>>>>> *   0 on success, a negative errno value otherwise and rte_errno is set.
>>>>> 
>>>>> Which means rte_errno is only modified in case of error.
>>>>> 
>>>>> This fix does not respect the documentation of the function or any other
>>>>> function of the PMD which can return errors.
>>>> 
>>>> That's logically a wrong interpretation. According to the description, if
>>>> returning error, rte_errno is set but the opposite isn't always true. Even if
>>>> rte_errno is set, it doesn't mean there's an error. So the description coincides
>>>> with that of errno. If you want to enforce preserving rte_errno in case of
>>>> success, you should amend the documentation.
>>>> 
>>>>> rte_errno is only set if an error is encountered and contains only the error
>>>>> code of the first error sub-sequent ones are considered consequences of the
>>>>> first one and thus not preserved.
>>>>> 
>>>>> Not preserving the rte_errno in roll backs is equivalent to not setting
>>>>> it at all as a function called by the rollback may also set it, example:
>>>>> 
>>>>> {
>>>>>   void * a;
>>>>> 
>>>>>   foo_do();
>>>>>   a  = malloc(10);
>>>>>   if (!a) {
>>>>>   	rte_errno = ENOMEM;
>>>>> 	foo_undo();
>>>> 
>>>> This example is weird. You can simply set rte_errno after foo_undo() in this
>>>> case.
>>>> 
>>>>> 	return -rte_errno;
>>>>>   }
>>>>> }
>>>>> 
>>>>> If foo_undo() also encounter an error it will modify the rte_errno which
>>>>> may have a value different from ENOMEM, for the callee won't be informed
>>>>> the error is due to a memory issue and thus cannot make counter parts.
>>>>> In such situation the rte_errno must be preserved to keep the ENOMEM
>>>>> error code.
>>>> 
>>>> I knew it. That's why rte_errno is saved before calling another API which may
>>>> change the rte_errno inside. But, we are talking about a case where an API
>>>> returns success. If caller is supposed to save rte_errno (when it's needed), why
>>>> does callee have to put some effort to preserve it even in case of success? If
>>>> rte_errno must be preserved even in case of success, we have to make a big
>>>> change to preserve rte_errno for cases where a void function is called (or cases
>>>> where we don't check its return value of non-void function).
>>>> 
>>>>> This is also the main reason almost all system function only update
>>>>> errno when no error is encountered.
>>>> 
>>>> 'Almost' doesn't mean 'all", does it? It is true that such functions must update
>>>> errno when it returns error but it doesn't care about the value when it returns
>>>> success. Like the man page I attached above, the errno is significant only when
>>>> it returns an error. And "a function that succeeds is allowed to change errno."
>>> 
>>> It is "almost" because a system function touching the errno when the
>>> function succeed it not common.  But as the man page says it is not
>>> impossible.
>>> 
>>>> So, the decision point is whether we want to preserve rte_errno in case of
>>>> success? My opinion is no.
>>> 
>>> I did not understood it was only a concern about the success of the
>>> function, even it is better to avoid as most as possible a useless
>>> store, in this specific case, as errno (rte_errno) has a garbage value,
>>> I fully agree with you.
>> 
>> Nelio,
>> 
>> Do you still want me to make any change for this patch?
>> Let me know if any.
> 
> With your modification the function documentation is no more accurate as
> rte_errno is always set.

I still don't agree with that but will send out v2. It's not a big deal.

Thanks,
Yongseok


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-stable] [PATCH v2] net/mlx5: fix error number handling
  2018-06-04 17:37 [dpdk-stable] [PATCH] net/mlx5: fix error number handling Yongseok Koh
  2018-06-05  6:52 ` Nélio Laranjeiro
@ 2018-06-19 23:13 ` Yongseok Koh
  2018-06-20  7:02   ` Nélio Laranjeiro
  1 sibling, 1 reply; 13+ messages in thread
From: Yongseok Koh @ 2018-06-19 23:13 UTC (permalink / raw)
  To: adrien.mazarguil, nelio.laranjeiro; +Cc: dev, shahafs, Yongseok Koh, stable

rte_errno should be saved only if error has occurred because rte_errno
could have garbage value.

Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
Cc: stable@dpdk.org

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 994be05be..45207d70e 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -3561,15 +3561,17 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
 		/* The flow does not match. */
 		continue;
 	}
-	ret = rte_errno; /* Save rte_errno before cleanup. */
 	if (flow)
 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
 exit:
+	if (ret)
+		ret = rte_errno; /* Save rte_errno before cleanup. */
 	for (i = 0; i != hash_rxq_init_n; ++i) {
 		if (parser.queue[i].ibv_attr)
 			rte_free(parser.queue[i].ibv_attr);
 	}
-	rte_errno = ret; /* Restore rte_errno. */
+	if (ret)
+		rte_errno = ret; /* Restore rte_errno. */
 	return -rte_errno;
 }
 
-- 
2.11.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix error number handling
  2018-06-19 23:13 ` [dpdk-stable] [PATCH v2] " Yongseok Koh
@ 2018-06-20  7:02   ` Nélio Laranjeiro
  2018-06-21 10:57     ` Shahaf Shuler
  0 siblings, 1 reply; 13+ messages in thread
From: Nélio Laranjeiro @ 2018-06-20  7:02 UTC (permalink / raw)
  To: Yongseok Koh; +Cc: adrien.mazarguil, dev, shahafs, stable

On Tue, Jun 19, 2018 at 04:13:13PM -0700, Yongseok Koh wrote:
> rte_errno should be saved only if error has occurred because rte_errno
> could have garbage value.
> 
> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>

Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

> ---
>  drivers/net/mlx5/mlx5_flow.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 994be05be..45207d70e 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -3561,15 +3561,17 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
>  		/* The flow does not match. */
>  		continue;
>  	}
> -	ret = rte_errno; /* Save rte_errno before cleanup. */
>  	if (flow)
>  		mlx5_flow_list_destroy(dev, &priv->flows, flow);
>  exit:
> +	if (ret)
> +		ret = rte_errno; /* Save rte_errno before cleanup. */
>  	for (i = 0; i != hash_rxq_init_n; ++i) {
>  		if (parser.queue[i].ibv_attr)
>  			rte_free(parser.queue[i].ibv_attr);
>  	}
> -	rte_errno = ret; /* Restore rte_errno. */
> +	if (ret)
> +		rte_errno = ret; /* Restore rte_errno. */
>  	return -rte_errno;
>  }
>  
> -- 
> 2.11.0

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
  2018-06-19 23:00               ` Yongseok Koh
@ 2018-06-20  7:05                 ` Nélio Laranjeiro
  0 siblings, 0 replies; 13+ messages in thread
From: Nélio Laranjeiro @ 2018-06-20  7:05 UTC (permalink / raw)
  To: Yongseok Koh
  Cc: Adrien Mazarguil, dev, Shahaf Shuler, dpdk stable, Xueming(Steven) Li

On Tue, Jun 19, 2018 at 11:00:25PM +0000, Yongseok Koh wrote:
>[...]
> >>> I did not understood it was only a concern about the success of the
> >>> function, even it is better to avoid as most as possible a useless
> >>> store, in this specific case, as errno (rte_errno) has a garbage value,
> >>> I fully agree with you.
> >> 
> >> Nelio,
> >> 
> >> Do you still want me to make any change for this patch?
> >> Let me know if any.
> > 
> > With your modification the function documentation is no more accurate as
> > rte_errno is always set.
> 
> I still don't agree with that but will send out v2. It's not a big deal.

What I meant is, you could have only changed the function documentation.

 @return
   0 on success with rte_errno always set, negative errno otherwise.

letting the function documentation saying rte_errno is only modified in
case of error whereas it is not is a bug or in the documentation or in
the code, but as a function must respect its documentation, it would
have raised a bug in the code itself.

Regards,

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix error number handling
  2018-06-20  7:02   ` Nélio Laranjeiro
@ 2018-06-21 10:57     ` Shahaf Shuler
  0 siblings, 0 replies; 13+ messages in thread
From: Shahaf Shuler @ 2018-06-21 10:57 UTC (permalink / raw)
  To: Nélio Laranjeiro, Yongseok Koh; +Cc: Adrien Mazarguil, dev, stable

Wednesday, June 20, 2018 10:03 AM, Nélio Laranjeiro:
> Subject: Re: [PATCH v2] net/mlx5: fix error number handling
> 
> On Tue, Jun 19, 2018 at 04:13:13PM -0700, Yongseok Koh wrote:
> > rte_errno should be saved only if error has occurred because rte_errno
> > could have garbage value.
> >
> > Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> 
> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

Applied to next-net-mlx, thanks. 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-06-21 10:57 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-04 17:37 [dpdk-stable] [PATCH] net/mlx5: fix error number handling Yongseok Koh
2018-06-05  6:52 ` Nélio Laranjeiro
2018-06-05 21:36   ` Yongseok Koh
2018-06-06  6:55     ` Nélio Laranjeiro
2018-06-06 18:39       ` Yongseok Koh
2018-06-07  7:39         ` Nélio Laranjeiro
2018-06-18 17:06           ` Yongseok Koh
2018-06-19 11:48             ` Nélio Laranjeiro
2018-06-19 23:00               ` Yongseok Koh
2018-06-20  7:05                 ` Nélio Laranjeiro
2018-06-19 23:13 ` [dpdk-stable] [PATCH v2] " Yongseok Koh
2018-06-20  7:02   ` Nélio Laranjeiro
2018-06-21 10:57     ` Shahaf Shuler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).