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

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

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