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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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
>>>>
>>