Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Thomas Monjalon]

31/10/2018 18:19, Ferruh Yigit:
> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
> - GNU-specific version
> 
> Those two has different return types, so the exiting return type check
> is not correct for GNU-specific version.
> 
> And this is causing failure in errno_autotest unit test.
> 
> Adding different implementation for FreeBSD and Linux.
> 
> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> --- a/lib/librte_eal/common/eal_common_errno.c
> +++ b/lib/librte_eal/common/eal_common_errno.c
>  		default:
> +#ifdef RTE_EXEC_ENV_BSDAPP
>  			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>  				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>  						sep, errnum);
> +#else
> +			/*
> +			 * _GNU_SOURCE version, error string is not always
> +			 * strored in "ret" buffer, need to use return value
> +			 */
> +			ret = strerror_r(errnum, ret, RETVAL_SZ);
> +#endif

Why not use the return value in both cases?

Why not writing an error message in Linux case?

[dpdk-stable] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

rte_strerror uses strerror_r(), and strerror_r() has two version of it.
- XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
- GNU-specific version

Those two has different return types, so the exiting return type check
is not correct for GNU-specific version.

And this is causing failure in errno_autotest unit test.

Adding different implementation for FreeBSD and Linux.

Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_eal/common/eal_common_errno.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_errno.c b/lib/librte_eal/common/eal_common_errno.c
index 56b492f5f..fbbc71b0b 100644
--- a/lib/librte_eal/common/eal_common_errno.c
+++ b/lib/librte_eal/common/eal_common_errno.c
@@ -38,9 +38,17 @@ rte_strerror(int errnum)
 		case E_RTE_NO_CONFIG:
 			return "Missing rte_config structure";
 		default:
+#ifdef RTE_EXEC_ENV_BSDAPP
 			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
 				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
 						sep, errnum);
+#else
+			/*
+			 * _GNU_SOURCE version, error string is not always
+			 * strored in "ret" buffer, need to use return value
+			 */
+			ret = strerror_r(errnum, ret, RETVAL_SZ);
+#endif
 		}
 
 	return ret;
-- 
2.17.2

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 10/31/2018 5:16 PM, Thomas Monjalon wrote:
> 31/10/2018 18:19, Ferruh Yigit:
>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
>> - GNU-specific version
>>
>> Those two has different return types, so the exiting return type check
>> is not correct for GNU-specific version.
>>
>> And this is causing failure in errno_autotest unit test.
>>
>> Adding different implementation for FreeBSD and Linux.
>>
>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> --- a/lib/librte_eal/common/eal_common_errno.c
>> +++ b/lib/librte_eal/common/eal_common_errno.c
>>  		default:
>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>  			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>>  				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>>  						sep, errnum);
>> +#else
>> +			/*
>> +			 * _GNU_SOURCE version, error string is not always
>> +			 * strored in "ret" buffer, need to use return value
>> +			 */
>> +			ret = strerror_r(errnum, ret, RETVAL_SZ);
>> +#endif
> 
> Why not use the return value in both cases?
> 
> Why not writing an error message in Linux case?

"man strerror_r" has more details, but briefly,

The XSI-compliant strerror_r() function returns 0 on success. GNU one returns
the pointer to string.

The XSI-compliant can return an empty buffer, GNU one always return a string,
either proper error string or "Unknown .." one.

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 10/31/2018 6:26 PM, Ferruh Yigit wrote:
> On 10/31/2018 5:16 PM, Thomas Monjalon wrote:
>> 31/10/2018 18:19, Ferruh Yigit:
>>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
>>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
>>> - GNU-specific version
>>>
>>> Those two has different return types, so the exiting return type check
>>> is not correct for GNU-specific version.
>>>
>>> And this is causing failure in errno_autotest unit test.
>>>
>>> Adding different implementation for FreeBSD and Linux.
>>>
>>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>> ---
>>> --- a/lib/librte_eal/common/eal_common_errno.c
>>> +++ b/lib/librte_eal/common/eal_common_errno.c
>>>  		default:
>>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>>  			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>>>  				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>>>  						sep, errnum);
>>> +#else
>>> +			/*
>>> +			 * _GNU_SOURCE version, error string is not always
>>> +			 * strored in "ret" buffer, need to use return value
>>> +			 */
>>> +			ret = strerror_r(errnum, ret, RETVAL_SZ);
>>> +#endif
>>
>> Why not use the return value in both cases?
>>
>> Why not writing an error message in Linux case?
> 
> "man strerror_r" has more details, but briefly,
> 
> The XSI-compliant strerror_r() function returns 0 on success. GNU one returns
> the pointer to string.
> 
> The XSI-compliant can return an empty buffer, GNU one always return a string,
> either proper error string or "Unknown .." one.

strerror_r() not portable. An alternative can be not using it at all...

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Thomas Monjalon]

31/10/2018 19:26, Ferruh Yigit:
> On 10/31/2018 6:26 PM, Ferruh Yigit wrote:
> > On 10/31/2018 5:16 PM, Thomas Monjalon wrote:
> >> 31/10/2018 18:19, Ferruh Yigit:
> >>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
> >>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
> >>> - GNU-specific version
> >>>
> >>> Those two has different return types, so the exiting return type check
> >>> is not correct for GNU-specific version.
> >>>
> >>> And this is causing failure in errno_autotest unit test.
> >>>
> >>> Adding different implementation for FreeBSD and Linux.
> >>>
> >>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>> ---
> >>> --- a/lib/librte_eal/common/eal_common_errno.c
> >>> +++ b/lib/librte_eal/common/eal_common_errno.c
> >>>  		default:
> >>> +#ifdef RTE_EXEC_ENV_BSDAPP
> >>>  			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
> >>>  				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
> >>>  						sep, errnum);
> >>> +#else
> >>> +			/*
> >>> +			 * _GNU_SOURCE version, error string is not always
> >>> +			 * strored in "ret" buffer, need to use return value
> >>> +			 */
> >>> +			ret = strerror_r(errnum, ret, RETVAL_SZ);
> >>> +#endif
> >>
> >> Why not use the return value in both cases?
> >>
> >> Why not writing an error message in Linux case?
> > 
> > "man strerror_r" has more details, but briefly,
> > 
> > The XSI-compliant strerror_r() function returns 0 on success. GNU one returns
> > the pointer to string.
> > 
> > The XSI-compliant can return an empty buffer, GNU one always return a string,
> > either proper error string or "Unknown .." one.

You say "GNU one always return a string"
The comment says:
_GNU_SOURCE version, error string is not always strored in "ret" buffer


> strerror_r() not portable. An alternative can be not using it at all...

It's fine to use it.

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 10/31/2018 6:43 PM, Thomas Monjalon wrote:
> 31/10/2018 19:26, Ferruh Yigit:
>> On 10/31/2018 6:26 PM, Ferruh Yigit wrote:
>>> On 10/31/2018 5:16 PM, Thomas Monjalon wrote:
>>>> 31/10/2018 18:19, Ferruh Yigit:
>>>>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
>>>>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
>>>>> - GNU-specific version
>>>>>
>>>>> Those two has different return types, so the exiting return type check
>>>>> is not correct for GNU-specific version.
>>>>>
>>>>> And this is causing failure in errno_autotest unit test.
>>>>>
>>>>> Adding different implementation for FreeBSD and Linux.
>>>>>
>>>>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>> ---
>>>>> --- a/lib/librte_eal/common/eal_common_errno.c
>>>>> +++ b/lib/librte_eal/common/eal_common_errno.c
>>>>>  		default:
>>>>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>>>>  			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>>>>>  				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>>>>>  						sep, errnum);
>>>>> +#else
>>>>> +			/*
>>>>> +			 * _GNU_SOURCE version, error string is not always
>>>>> +			 * strored in "ret" buffer, need to use return value
>>>>> +			 */
>>>>> +			ret = strerror_r(errnum, ret, RETVAL_SZ);
>>>>> +#endif
>>>>
>>>> Why not use the return value in both cases?
>>>>
>>>> Why not writing an error message in Linux case?
>>>
>>> "man strerror_r" has more details, but briefly,
>>>
>>> The XSI-compliant strerror_r() function returns 0 on success. GNU one returns
>>> the pointer to string.
>>>
>>> The XSI-compliant can return an empty buffer, GNU one always return a string,
>>> either proper error string or "Unknown .." one.
> 
> You say "GNU one always return a string"
> The comment says:
> _GNU_SOURCE version, error string is not always strored in "ret" buffer

Yes, GNU one always return a char pointer to a string but that pointer may not
be in the "ret" buffer.

> 
> 
>> strerror_r() not portable. An alternative can be not using it at all...
> 
> It's fine to use it.
> 
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Thomas Monjalon]

01/11/2018 13:46, Ferruh Yigit:
> On 10/31/2018 6:43 PM, Thomas Monjalon wrote:
> > 31/10/2018 19:26, Ferruh Yigit:
> >> On 10/31/2018 6:26 PM, Ferruh Yigit wrote:
> >>> On 10/31/2018 5:16 PM, Thomas Monjalon wrote:
> >>>> 31/10/2018 18:19, Ferruh Yigit:
> >>>>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
> >>>>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
> >>>>> - GNU-specific version
> >>>>>
> >>>>> Those two has different return types, so the exiting return type check
> >>>>> is not correct for GNU-specific version.
> >>>>>
> >>>>> And this is causing failure in errno_autotest unit test.
> >>>>>
> >>>>> Adding different implementation for FreeBSD and Linux.
> >>>>>
> >>>>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>>> ---
> >>>>> --- a/lib/librte_eal/common/eal_common_errno.c
> >>>>> +++ b/lib/librte_eal/common/eal_common_errno.c
> >>>>>  		default:
> >>>>> +#ifdef RTE_EXEC_ENV_BSDAPP
> >>>>>  			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
> >>>>>  				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
> >>>>>  						sep, errnum);
> >>>>> +#else
> >>>>> +			/*
> >>>>> +			 * _GNU_SOURCE version, error string is not always
> >>>>> +			 * strored in "ret" buffer, need to use return value
> >>>>> +			 */
> >>>>> +			ret = strerror_r(errnum, ret, RETVAL_SZ);
> >>>>> +#endif
> >>>>
> >>>> Why not use the return value in both cases?
> >>>>
> >>>> Why not writing an error message in Linux case?
> >>>
> >>> "man strerror_r" has more details, but briefly,
> >>>
> >>> The XSI-compliant strerror_r() function returns 0 on success. GNU one returns
> >>> the pointer to string.
> >>>
> >>> The XSI-compliant can return an empty buffer, GNU one always return a string,
> >>> either proper error string or "Unknown .." one.
> > 
> > You say "GNU one always return a string"
> > The comment says:
> > _GNU_SOURCE version, error string is not always strored in "ret" buffer
> 
> Yes, GNU one always return a char pointer to a string but that pointer may not
> be in the "ret" buffer.

OK

So I suggest only 2 minor changes:
	- strored -> stored
	- add a comment to explain that the error message from return value is enough

> >> strerror_r() not portable. An alternative can be not using it at all...
> > 
> > It's fine to use it.

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 11/1/2018 1:40 PM, Thomas Monjalon wrote:
> 01/11/2018 13:46, Ferruh Yigit:
>> On 10/31/2018 6:43 PM, Thomas Monjalon wrote:
>>> 31/10/2018 19:26, Ferruh Yigit:
>>>> On 10/31/2018 6:26 PM, Ferruh Yigit wrote:
>>>>> On 10/31/2018 5:16 PM, Thomas Monjalon wrote:
>>>>>> 31/10/2018 18:19, Ferruh Yigit:
>>>>>>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
>>>>>>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
>>>>>>> - GNU-specific version
>>>>>>>
>>>>>>> Those two has different return types, so the exiting return type check
>>>>>>> is not correct for GNU-specific version.
>>>>>>>
>>>>>>> And this is causing failure in errno_autotest unit test.
>>>>>>>
>>>>>>> Adding different implementation for FreeBSD and Linux.
>>>>>>>
>>>>>>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>> ---
>>>>>>> --- a/lib/librte_eal/common/eal_common_errno.c
>>>>>>> +++ b/lib/librte_eal/common/eal_common_errno.c
>>>>>>>  		default:
>>>>>>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>>>>>>  			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>>>>>>>  				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>>>>>>>  						sep, errnum);
>>>>>>> +#else
>>>>>>> +			/*
>>>>>>> +			 * _GNU_SOURCE version, error string is not always
>>>>>>> +			 * strored in "ret" buffer, need to use return value
>>>>>>> +			 */
>>>>>>> +			ret = strerror_r(errnum, ret, RETVAL_SZ);
>>>>>>> +#endif
>>>>>>
>>>>>> Why not use the return value in both cases?
>>>>>>
>>>>>> Why not writing an error message in Linux case?
>>>>>
>>>>> "man strerror_r" has more details, but briefly,
>>>>>
>>>>> The XSI-compliant strerror_r() function returns 0 on success. GNU one returns
>>>>> the pointer to string.
>>>>>
>>>>> The XSI-compliant can return an empty buffer, GNU one always return a string,
>>>>> either proper error string or "Unknown .." one.
>>>
>>> You say "GNU one always return a string"
>>> The comment says:
>>> _GNU_SOURCE version, error string is not always strored in "ret" buffer
>>
>> Yes, GNU one always return a char pointer to a string but that pointer may not
>> be in the "ret" buffer.
> 
> OK
> 
> So I suggest only 2 minor changes:
> 	- strored -> stored
> 	- add a comment to explain that the error message from return value is enough

That is what I was trying to say in comment, we can't trust "ret" buffer but
need to use ret value which we can trust, I will re-visit the comment and fix typo

> 
>>>> strerror_r() not portable. An alternative can be not using it at all...
>>>
>>> It's fine to use it.
> 
> 
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Jerin Jacob]

-----Original Message-----
> Date: Wed, 31 Oct 2018 17:19:28 +0000
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Bruce Richardson <bruce.richardson@intel.com>
> CC: dev@dpdk.org, Ferruh Yigit <ferruh.yigit@intel.com>, stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] eal: fix API to get error string
> X-Mailer: git-send-email 2.17.2
> 
> External Email
> 
> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
> - GNU-specific version
> 
> Those two has different return types, so the exiting return type check
> is not correct for GNU-specific version.
> 
> And this is causing failure in errno_autotest unit test.
> 
> Adding different implementation for FreeBSD and Linux.
> 
> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  lib/librte_eal/common/eal_common_errno.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/librte_eal/common/eal_common_errno.c b/lib/librte_eal/common/eal_common_errno.c
> index 56b492f5f..fbbc71b0b 100644
> --- a/lib/librte_eal/common/eal_common_errno.c
> +++ b/lib/librte_eal/common/eal_common_errno.c
> @@ -38,9 +38,17 @@ rte_strerror(int errnum)
>                 case E_RTE_NO_CONFIG:
>                         return "Missing rte_config structure";
>                 default:
> +#ifdef RTE_EXEC_ENV_BSDAPP
>                         if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>                                 snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>                                                 sep, errnum);
> +#else
> +                       /*
> +                        * _GNU_SOURCE version, error string is not always
> +                        * strored in "ret" buffer, need to use return value
> +                        */
> +                       ret = strerror_r(errnum, ret, RETVAL_SZ);

Probably this will fail in musl c version.
https://git.musl-libc.org/cgit/musl/tree/src/string/strerror_r.c

Another alternative of this patch.

http://patches.dpdk.org/patch/47706/

> +#endif
>                 }
> 
>         return ret;
> --
> 2.17.2
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 11/2/2018 9:51 AM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Wed, 31 Oct 2018 17:19:28 +0000
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Bruce Richardson <bruce.richardson@intel.com>
>> CC: dev@dpdk.org, Ferruh Yigit <ferruh.yigit@intel.com>, stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH] eal: fix API to get error string
>> X-Mailer: git-send-email 2.17.2
>>
>> External Email
>>
>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
>> - GNU-specific version
>>
>> Those two has different return types, so the exiting return type check
>> is not correct for GNU-specific version.
>>
>> And this is causing failure in errno_autotest unit test.
>>
>> Adding different implementation for FreeBSD and Linux.
>>
>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>>  lib/librte_eal/common/eal_common_errno.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/lib/librte_eal/common/eal_common_errno.c b/lib/librte_eal/common/eal_common_errno.c
>> index 56b492f5f..fbbc71b0b 100644
>> --- a/lib/librte_eal/common/eal_common_errno.c
>> +++ b/lib/librte_eal/common/eal_common_errno.c
>> @@ -38,9 +38,17 @@ rte_strerror(int errnum)
>>                 case E_RTE_NO_CONFIG:
>>                         return "Missing rte_config structure";
>>                 default:
>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>                         if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>>                                 snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>>                                                 sep, errnum);
>> +#else
>> +                       /*
>> +                        * _GNU_SOURCE version, error string is not always
>> +                        * strored in "ret" buffer, need to use return value
>> +                        */
>> +                       ret = strerror_r(errnum, ret, RETVAL_SZ);
> 
> Probably this will fail in musl c version.
> https://git.musl-libc.org/cgit/musl/tree/src/string/strerror_r.c

You are right, it will fail with musl. It may not be good idea to separate this
as BSD and Linux.

Instead of playing with strerror_r(), what about use strerror() and copy string
to RTE_PER_LCORE(retval)? I will send a patch for it.

> 
> Another alternative of this patch.
> 
> http://patches.dpdk.org/patch/47706/

I think this works, but I am not sure if it will have any side effect. And if we
want to add more functions to this file, that may be effected. I am more to fix
this locally in rte_strerror() function.

> 
>> +#endif
>>                 }
>>
>>         return ret;
>> --
>> 2.17.2
>>

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Jerin Jacob]

-----Original Message-----
> Date: Fri, 2 Nov 2018 15:39:04 +0000
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Bruce Richardson <bruce.richardson@intel.com>, "dev@dpdk.org"
>  <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH] eal: fix API to get error string
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.9.1
> 
> 
> On 11/2/2018 9:51 AM, Jerin Jacob wrote:
> > -----Original Message-----
> >> Date: Wed, 31 Oct 2018 17:19:28 +0000
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> To: Bruce Richardson <bruce.richardson@intel.com>
> >> CC: dev@dpdk.org, Ferruh Yigit <ferruh.yigit@intel.com>, stable@dpdk.org
> >> Subject: [dpdk-dev] [PATCH] eal: fix API to get error string
> >> X-Mailer: git-send-email 2.17.2
> >>
> >> External Email
> >>
> >> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
> >> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
> >> - GNU-specific version
> >>
> >> Those two has different return types, so the exiting return type check
> >> is not correct for GNU-specific version.
> >>
> >> And this is causing failure in errno_autotest unit test.
> >>
> >> Adding different implementation for FreeBSD and Linux.
> >>
> >> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> ---
> >>  lib/librte_eal/common/eal_common_errno.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/lib/librte_eal/common/eal_common_errno.c b/lib/librte_eal/common/eal_common_errno.c
> >> index 56b492f5f..fbbc71b0b 100644
> >> --- a/lib/librte_eal/common/eal_common_errno.c
> >> +++ b/lib/librte_eal/common/eal_common_errno.c
> >> @@ -38,9 +38,17 @@ rte_strerror(int errnum)
> >>                 case E_RTE_NO_CONFIG:
> >>                         return "Missing rte_config structure";
> >>                 default:
> >> +#ifdef RTE_EXEC_ENV_BSDAPP
> >>                         if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
> >>                                 snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
> >>                                                 sep, errnum);
> >> +#else
> >> +                       /*
> >> +                        * _GNU_SOURCE version, error string is not always
> >> +                        * strored in "ret" buffer, need to use return value
> >> +                        */
> >> +                       ret = strerror_r(errnum, ret, RETVAL_SZ);
> >
> > Probably this will fail in musl c version.
> > https://git.musl-libc.org/cgit/musl/tree/src/string/strerror_r.c
> 
> You are right, it will fail with musl. It may not be good idea to separate this
> as BSD and Linux.
> 
> Instead of playing with strerror_r(), what about use strerror() and copy string
> to RTE_PER_LCORE(retval)? I will send a patch for it.

I thought we used strerror_r() to enable thread safety.IMO, strerror()
is not thread safe.

> 
> >
> > Another alternative of this patch.
> >
> > http://patches.dpdk.org/patch/47706/
> 
> I think this works, but I am not sure if it will have any side effect. And if we
> want to add more functions to this file, that may be effected. I am more to fix
> this locally in rte_strerror() function.

If we can then it is good.

> 
> >
> >> +#endif
> >>                 }
> >>
> >>         return ret;
> >> --
> >> 2.17.2
> >>
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 11/2/2018 3:45 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Fri, 2 Nov 2018 15:39:04 +0000
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: Bruce Richardson <bruce.richardson@intel.com>, "dev@dpdk.org"
>>  <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
>> Subject: Re: [dpdk-dev] [PATCH] eal: fix API to get error string
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.9.1
>>
>>
>> On 11/2/2018 9:51 AM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Wed, 31 Oct 2018 17:19:28 +0000
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> To: Bruce Richardson <bruce.richardson@intel.com>
>>>> CC: dev@dpdk.org, Ferruh Yigit <ferruh.yigit@intel.com>, stable@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH] eal: fix API to get error string
>>>> X-Mailer: git-send-email 2.17.2
>>>>
>>>> External Email
>>>>
>>>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
>>>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
>>>> - GNU-specific version
>>>>
>>>> Those two has different return types, so the exiting return type check
>>>> is not correct for GNU-specific version.
>>>>
>>>> And this is causing failure in errno_autotest unit test.
>>>>
>>>> Adding different implementation for FreeBSD and Linux.
>>>>
>>>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> ---
>>>>  lib/librte_eal/common/eal_common_errno.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/lib/librte_eal/common/eal_common_errno.c b/lib/librte_eal/common/eal_common_errno.c
>>>> index 56b492f5f..fbbc71b0b 100644
>>>> --- a/lib/librte_eal/common/eal_common_errno.c
>>>> +++ b/lib/librte_eal/common/eal_common_errno.c
>>>> @@ -38,9 +38,17 @@ rte_strerror(int errnum)
>>>>                 case E_RTE_NO_CONFIG:
>>>>                         return "Missing rte_config structure";
>>>>                 default:
>>>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>>>                         if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>>>>                                 snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>>>>                                                 sep, errnum);
>>>> +#else
>>>> +                       /*
>>>> +                        * _GNU_SOURCE version, error string is not always
>>>> +                        * strored in "ret" buffer, need to use return value
>>>> +                        */
>>>> +                       ret = strerror_r(errnum, ret, RETVAL_SZ);
>>>
>>> Probably this will fail in musl c version.
>>> https://git.musl-libc.org/cgit/musl/tree/src/string/strerror_r.c
>>
>> You are right, it will fail with musl. It may not be good idea to separate this
>> as BSD and Linux.
>>
>> Instead of playing with strerror_r(), what about use strerror() and copy string
>> to RTE_PER_LCORE(retval)? I will send a patch for it.
> 
> I thought we used strerror_r() to enable thread safety.IMO, strerror()
> is not thread safe.

Good point, yes we need to use strerror_r()

> 
>>
>>>
>>> Another alternative of this patch.
>>>
>>> http://patches.dpdk.org/patch/47706/
>>
>> I think this works, but I am not sure if it will have any side effect. And if we
>> want to add more functions to this file, that may be effected. I am more to fix
>> this locally in rte_strerror() function.
> 
> If we can then it is good.
> 
>>
>>>
>>>> +#endif
>>>>                 }
>>>>
>>>>         return ret;
>>>> --
>>>> 2.17.2
>>>>
>>

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 11/1/2018 1:40 PM, Thomas Monjalon wrote:
> 01/11/2018 13:46, Ferruh Yigit:
>> On 10/31/2018 6:43 PM, Thomas Monjalon wrote:
>>> 31/10/2018 19:26, Ferruh Yigit:
>>>> On 10/31/2018 6:26 PM, Ferruh Yigit wrote:
>>>>> On 10/31/2018 5:16 PM, Thomas Monjalon wrote:
>>>>>> 31/10/2018 18:19, Ferruh Yigit:
>>>>>>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
>>>>>>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
>>>>>>> - GNU-specific version
>>>>>>>
>>>>>>> Those two has different return types, so the exiting return type check
>>>>>>> is not correct for GNU-specific version.
>>>>>>>
>>>>>>> And this is causing failure in errno_autotest unit test.
>>>>>>>
>>>>>>> Adding different implementation for FreeBSD and Linux.
>>>>>>>
>>>>>>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>> ---
>>>>>>> --- a/lib/librte_eal/common/eal_common_errno.c
>>>>>>> +++ b/lib/librte_eal/common/eal_common_errno.c
>>>>>>>  		default:
>>>>>>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>>>>>>  			if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>>>>>>>  				snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>>>>>>>  						sep, errnum);
>>>>>>> +#else
>>>>>>> +			/*
>>>>>>> +			 * _GNU_SOURCE version, error string is not always
>>>>>>> +			 * strored in "ret" buffer, need to use return value
>>>>>>> +			 */
>>>>>>> +			ret = strerror_r(errnum, ret, RETVAL_SZ);
>>>>>>> +#endif
>>>>>>
>>>>>> Why not use the return value in both cases?
>>>>>>
>>>>>> Why not writing an error message in Linux case?
>>>>>
>>>>> "man strerror_r" has more details, but briefly,
>>>>>
>>>>> The XSI-compliant strerror_r() function returns 0 on success. GNU one returns
>>>>> the pointer to string.
>>>>>
>>>>> The XSI-compliant can return an empty buffer, GNU one always return a string,
>>>>> either proper error string or "Unknown .." one.
>>>
>>> You say "GNU one always return a string"
>>> The comment says:
>>> _GNU_SOURCE version, error string is not always strored in "ret" buffer
>>
>> Yes, GNU one always return a char pointer to a string but that pointer may not
>> be in the "ret" buffer.
> 
> OK
> 
> So I suggest only 2 minor changes:
> 	- strored -> stored
> 	- add a comment to explain that the error message from return value is enough

For the case ret buffer is not filled, although returned pointer points to
immutable buffer and enough to return it, the expected API behavior can be to
fill the buffer, so I will fill the buffer and return it instead of comment.

> 
>>>> strerror_r() not portable. An alternative can be not using it at all...
>>>
>>> It's fine to use it.
> 
> 
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 11/2/2018 3:45 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Fri, 2 Nov 2018 15:39:04 +0000
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: Bruce Richardson <bruce.richardson@intel.com>, "dev@dpdk.org"
>>  <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
>> Subject: Re: [dpdk-dev] [PATCH] eal: fix API to get error string
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.9.1
>>
>>
>> On 11/2/2018 9:51 AM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Wed, 31 Oct 2018 17:19:28 +0000
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> To: Bruce Richardson <bruce.richardson@intel.com>
>>>> CC: dev@dpdk.org, Ferruh Yigit <ferruh.yigit@intel.com>, stable@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH] eal: fix API to get error string
>>>> X-Mailer: git-send-email 2.17.2
>>>>
>>>> External Email
>>>>
>>>> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
>>>> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
>>>> - GNU-specific version
>>>>
>>>> Those two has different return types, so the exiting return type check
>>>> is not correct for GNU-specific version.
>>>>
>>>> And this is causing failure in errno_autotest unit test.
>>>>
>>>> Adding different implementation for FreeBSD and Linux.
>>>>
>>>> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> ---
>>>>  lib/librte_eal/common/eal_common_errno.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/lib/librte_eal/common/eal_common_errno.c b/lib/librte_eal/common/eal_common_errno.c
>>>> index 56b492f5f..fbbc71b0b 100644
>>>> --- a/lib/librte_eal/common/eal_common_errno.c
>>>> +++ b/lib/librte_eal/common/eal_common_errno.c
>>>> @@ -38,9 +38,17 @@ rte_strerror(int errnum)
>>>>                 case E_RTE_NO_CONFIG:
>>>>                         return "Missing rte_config structure";
>>>>                 default:
>>>> +#ifdef RTE_EXEC_ENV_BSDAPP
>>>>                         if (strerror_r(errnum, ret, RETVAL_SZ) != 0)
>>>>                                 snprintf(ret, RETVAL_SZ, "Unknown error%s %d",
>>>>                                                 sep, errnum);
>>>> +#else
>>>> +                       /*
>>>> +                        * _GNU_SOURCE version, error string is not always
>>>> +                        * strored in "ret" buffer, need to use return value
>>>> +                        */
>>>> +                       ret = strerror_r(errnum, ret, RETVAL_SZ);
>>>
>>> Probably this will fail in musl c version.
>>> https://git.musl-libc.org/cgit/musl/tree/src/string/strerror_r.c
>>
>> You are right, it will fail with musl. It may not be good idea to separate this
>> as BSD and Linux.
>>
>> Instead of playing with strerror_r(), what about use strerror() and copy string
>> to RTE_PER_LCORE(retval)? I will send a patch for it.
> 
> I thought we used strerror_r() to enable thread safety.IMO, strerror()
> is not thread safe.
> 
>>
>>>
>>> Another alternative of this patch.
>>>
>>> http://patches.dpdk.org/patch/47706/
>>
>> I think this works, but I am not sure if it will have any side effect. And if we
>> want to add more functions to this file, that may be effected. I am more to fix
>> this locally in rte_strerror() function.
> 
> If we can then it is good.

I was hoping to use "#if !defined(_GNU_SOURCE)" to differentiate glibc and musl,
but we can't because gcc/clang adds this macro themselves.

+1 to your fix. We can dedicate this .c file only for this API at worst.

> 
>>
>>>
>>>> +#endif
>>>>                 }
>>>>
>>>>         return ret;
>>>> --
>>>> 2.17.2
>>>>
>>

Re: [dpdk-stable] [PATCH] eal: fix API to get error string

[Ferruh Yigit]

On 10/31/2018 5:19 PM, Ferruh Yigit wrote:
> rte_strerror uses strerror_r(), and strerror_r() has two version of it.
> - XSI-compliant version, (_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE
> - GNU-specific version
> 
> Those two has different return types, so the exiting return type check
> is not correct for GNU-specific version.
> 
> And this is causing failure in errno_autotest unit test.
> 
> Adding different implementation for FreeBSD and Linux.
> 
> Fixes: 016c32bd3e3d ("eal: cleanup strerror function")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

self Nack, in favor of http://patches.dpdk.org/patch/47706/