glibc strerrorname_np

[Jonny Grant]

On 09/11/2021 12:30, Adhemerval Zanella wrote:
> 
> 
> On 08/11/2021 19:22, Jonny Grant wrote:
>>
>>
>> On 06/11/2021 12:51, Adhemerval Zanella wrote:
>>>
>>>
>>> On 05/11/2021 19:23, Jonny Grant wrote:
>>>>
>>>>
>>>> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>>>>
>>>>>
>>>>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>>>>>
>>>>>>
>>>>>> Hi Adhemerval
>>>>>>
>>>>>> Thank you for your reply. Personally I understood an ABI break would be the return type, the name, or the parameters. But the proposed change is not so. Changing to return a string, should be fine.
>>>>>
>>>>> It is still an ABI break, code that checks NULL for invalid input will
>>>>> stop to work.
>>>>>
>>>>>>
>>>>>> ie, in relation to strerror() C99 and POSIX.1-2008 require the return value to be non-NULL. (my view is it is always better not to return a NULL from such string functions that could then cause a SEGV.
>>>>>>
>>>>>> strerror(1000) returns a string "Unknown error 1000"
>>>>>>
>>>>>> Better to simply align with glibc strerror() approach?
>>>>>>
>>>>>> Feels like there is still time to change it, as it is _np. Aligning with strerror(), or just "" as you had mentioned seems reasonable.
>>>>>
>>>>> I give you that it is indeed a better return code, and it is not a matter
>>>>> of timing, but rather I don't think it really worth the ABI break and 
>>>>> the required code complexity to do so. 
>>>>>
>>>>> It would require:
>>>>>
>>>>>   1. Change the strerrorname_np to return "" on invalid code.
>>>>
>>>> Please find attached the patch.
>>>>
>>>>>   2. Keep the compat symbol that returns NULL and add a compat symbol.
>>>>>   3. Exports a new symbol with version on 2.35 with the new semantic
>>>>>      and update the ailist.
>>
>> Could you dircet me to the ailist file you mention please.
> 
> I spelled it wrong, it should be 'abilist':
> 
> $ find . -iname *.abilist
> ./sysdeps/unix/sysv/linux/x86_64/64/libBrokenLocale.abilist
> ./sysdeps/unix/sysv/linux/x86_64/64/libm.abilist
> ./sysdeps/unix/sysv/linux/x86_64/64/libutil.abilist
> [...]
> 
> You can check if the exported symbols are on par with the pre-defined
> ones with:
> 
> $ make check-abi
> 
> And you can update the abilist files with:
> 
> $ make update-abi
> 
> It will need to be properly exported on the 'Versions' file.
> 
> 
>>
>>>>
>>>> May I check, why would a new symbol be needed? I'd expect it is only a change to strerrorname_np and any test code you have that presently checks for NULL return.
>>>
>>> As I said before it is an ABI break, since users that check for invalid 
>>> errno against NULL will start to fail.  For such change we *do need* all 
>>> the trouble of adding a compat symbol with current semantic.
>> Many thanks for your reply. May I check, are even the _np functions set in stone once they are released? glibc strerrorname_np was released a year ago. My disto Ubuntu LTS doesn't yet have this new glibc release containing this function.
> 
> It is not a matter of time, neither glibc is tied to any distribution.  It is
> a matter or point release, for instance 2.34.  Once we export such symbol on
> a release, programs will be built against and depend of the symbol.

Ok I see.
It's also tricky as I'd prefer to not define _GNU_SOURCE

What do you think of the possiblity of adding something like strerrorname() directly without _GNU_SOURCE, and always returning a valid string, or empty string?

With a consistent return for an errnum of 0.
strerror_r(0, &b, 0); already returns "".
strerror(0) returns "Success"

So there is some inconsistency in the POSIX functions.




>>
>>>>
>>>>>   4. Update the documentation and sync with man-pages. 
>>>>
>>>> The man-page update is minor, I could handle that.
>>>>
>>>>
>>>>>> https://man7.org/linux/man-pages/man3/strerror.3.html
>>>>>>
>>>>>> It's common for some returns to change, eg glibc 2.13 changed strerror_r() behaviour to return the actual error code, as opposed to returning -1 and setting errno.
>>>>>
>>>>> And such change did got without burden and extra complexity. Just check
>>>>> the multiple preprocessor checks it requires to get the right definition
>>>>> depending of the system support on the misc/error.c (imported from gnulib).
>>>>>
>>>>
>>>> Ok, I think the change I propose does not affect the definition, as the function signature is the same. Maybe I misunderstand something.
>>>
>>> The ABI break is not only for function signature and input alignment/size,
>>> but also for function semantic.  Just check the fmemopen
>>> (fdb7d390dd0d96e4a8239c46f3aa64598b90842b), where we kept the old buggy 
>>> implementation since even when it is not POSIX compliant because we do
>>> not know if users do depend of such behavior.
>>>
>>
>> I was reading the fmemopen ticket you shared. May I ask - could you direct me to a "compat symbol" description? I did search and came across libc_hidden_proto and see it in use in /glibc/include/stdio.h the patch commit note says update fmemopen to the new POSIX spec.
> 
> To put simply, the 'compat symbol' is a GNU extension that creates a 
> symbol tied to an specific version where a linked program built against 
> an older glibc will continue to use the older version instead of the 
> default one.
> 
> There is some information on how it is created internally on
> 'include/shlib-compat.h'.
> 

I see. Thank you for explaining.

Kind regards
Jonny