Re: [PATCH RFC v3 7/8] termios: change the generic cfsetspeed() to support arbitrary speeds
H. Peter Anvin
hpa@zytor.com
Fri May 9 15:43:55 GMT 2025
On May 9, 2025 6:35:38 AM PDT, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
>
>
>On 08/05/25 18:03, H. Peter Anvin wrote:
>> On May 8, 2025 11:38:19 AM PDT, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
>>>
>>>
>>> On 03/05/25 22:43, H. Peter Anvin wrote:
>>>> The generic implementaion of cfsetspeed() had an internal table of
>>>> permitted baud rates, which was enforced even on an implementation
>>>> supporting arbitrary baud rates. This was to be able to *also* accept
>>>> numeric constants as well as Bxxx values.
>>>>
>>>> This fundamentally makes no sense; not only does it go against the
>>>> documented behavior of cfsetspeed() which is to take the same input
>>>> as cfset[io]speed(), but it means cfsetspeed() is broken with regard
>>>> to a platform supporting arbitrary speeds.
>>>>
>>>> With Linux converted to arbitrary baud rates, the only remaining case
>>>> of non-arbitrary baud rates appears to be Hurd with USE_OLD_TTY, which
>>>> one can presume being a legacy case that few if any people care about,
>>>> and so simply strip out this code and make cfsetspeed() rely on
>>>> cfsetospeed() to validate acceptable speed constants.
>>>>
>>>> If a new platform is introduced which does not have arbitrary baud
>>>> rate support, using non-baud rate Bxxx constants (highly not
>>>> recommended; should be abstracted at the glibc level) but such
>>>> aliasing is desired, it should be supported by cfset[io]speed() as
>>>> well, and belongs in the platform-specific code.
>>>>
>>>> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
>>>> ---
>>>> termios/cfsetspeed.c | 138 ++-----------------------------------------
>>>> 1 file changed, 5 insertions(+), 133 deletions(-)
>>>>
>>>> diff --git a/termios/cfsetspeed.c b/termios/cfsetspeed.c
>>>> index 053e3945ea10..257c6387aaa0 100644
>>>> --- a/termios/cfsetspeed.c
>>>> +++ b/termios/cfsetspeed.c
>>>> @@ -19,143 +19,15 @@
>>>> #include <errno.h>
>>>> #include <stddef.h>
>>>>
>>>> -struct speed_struct
>>>> -{
>>>> - speed_t value;
>>>> - speed_t internal;
>>>> -};
>>>> -
>>>> -static const struct speed_struct speeds[] =
>>>> - {
>>>> -#ifdef B0
>>>> - { 0, B0 },
>>>> -#endif
>>>> -#ifdef B50
>>>> - { 50, B50 },
>>>> -#endif
>>>> -#ifdef B75
>>>> - { 75, B75 },
>>>> -#endif
>>>> -#ifdef B110
>>>> - { 110, B110 },
>>>> -#endif
>>>> -#ifdef B134
>>>> - { 134, B134 },
>>>> -#endif
>>>> -#ifdef B150
>>>> - { 150, B150 },
>>>> -#endif
>>>> -#ifdef B200
>>>> - { 200, B200 },
>>>> -#endif
>>>> -#ifdef B300
>>>> - { 300, B300 },
>>>> -#endif
>>>> -#ifdef B600
>>>> - { 600, B600 },
>>>> -#endif
>>>> -#ifdef B1200
>>>> - { 1200, B1200 },
>>>> -#endif
>>>> -#ifdef B1200
>>>> - { 1200, B1200 },
>>>> -#endif
>>>> -#ifdef B1800
>>>> - { 1800, B1800 },
>>>> -#endif
>>>> -#ifdef B2400
>>>> - { 2400, B2400 },
>>>> -#endif
>>>> -#ifdef B4800
>>>> - { 4800, B4800 },
>>>> -#endif
>>>> -#ifdef B9600
>>>> - { 9600, B9600 },
>>>> -#endif
>>>> -#ifdef B19200
>>>> - { 19200, B19200 },
>>>> -#endif
>>>> -#ifdef B38400
>>>> - { 38400, B38400 },
>>>> -#endif
>>>> -#ifdef B57600
>>>> - { 57600, B57600 },
>>>> -#endif
>>>> -#ifdef B76800
>>>> - { 76800, B76800 },
>>>> -#endif
>>>> -#ifdef B115200
>>>> - { 115200, B115200 },
>>>> -#endif
>>>> -#ifdef B153600
>>>> - { 153600, B153600 },
>>>> -#endif
>>>> -#ifdef B230400
>>>> - { 230400, B230400 },
>>>> -#endif
>>>> -#ifdef B307200
>>>> - { 307200, B307200 },
>>>> -#endif
>>>> -#ifdef B460800
>>>> - { 460800, B460800 },
>>>> -#endif
>>>> -#ifdef B500000
>>>> - { 500000, B500000 },
>>>> -#endif
>>>> -#ifdef B576000
>>>> - { 576000, B576000 },
>>>> -#endif
>>>> -#ifdef B921600
>>>> - { 921600, B921600 },
>>>> -#endif
>>>> -#ifdef B1000000
>>>> - { 1000000, B1000000 },
>>>> -#endif
>>>> -#ifdef B1152000
>>>> - { 1152000, B1152000 },
>>>> -#endif
>>>> -#ifdef B1500000
>>>> - { 1500000, B1500000 },
>>>> -#endif
>>>> -#ifdef B2000000
>>>> - { 2000000, B2000000 },
>>>> -#endif
>>>> -#ifdef B2500000
>>>> - { 2500000, B2500000 },
>>>> -#endif
>>>> -#ifdef B3000000
>>>> - { 3000000, B3000000 },
>>>> -#endif
>>>> -#ifdef B3500000
>>>> - { 3500000, B3500000 },
>>>> -#endif
>>>> -#ifdef B4000000
>>>> - { 4000000, B4000000 },
>>>> -#endif
>>>> - };
>>>> -
>>>> -
>>>> /* Set both the input and output baud rates stored in *TERMIOS_P to SPEED. */
>>>> int
>>>> cfsetspeed (struct termios *termios_p, speed_t speed)
>>>> {
>>>> - size_t cnt;
>>>> -
>>>> - for (cnt = 0; cnt < sizeof (speeds) / sizeof (speeds[0]); ++cnt)
>>>> - if (speed == speeds[cnt].internal)
>>>> - {
>>>> - cfsetispeed (termios_p, speed);
>>>> - cfsetospeed (termios_p, speed);
>>>> - return 0;
>>>> - }
>>>> - else if (speed == speeds[cnt].value)
>>>> - {
>>>> - cfsetispeed (termios_p, speeds[cnt].internal);
>>>> - cfsetospeed (termios_p, speeds[cnt].internal);
>>>> - return 0;
>>>> - }
>>>> + int rv;
>>>>
>>>> - __set_errno (EINVAL);
>>>> + rv = cfsetospeed (termios_p, speed);
>>>> + if (rv)
>>>> + return rv;
>>>>
>>>> - return -1;
>>>> + return cfsetispeed (termios_p, speed);
>>>
>>> I think it should be __cfsetospeed and __cfsetispeed to avoid PLT creation.
>>> This looks ok, although recently I am trying to avoid changing code that is
>>> not built by any target.
>>>
>>>> }
>>>
>>
>> I will respond to the rest later, but I'm glad I'm not the only one who apparently finds this insanely confusing, with the behavior created ad hoc all over the place.
>>
>> The short answer is that *at this point* in the patch series it is actually correct to use the non-underscore versions, but that is changed (including this file) in patch 8.
>
>It made sense 25 years ago when glibc project aimed to be an alternative
>libc for different system with different standards support (that's why
>we still have most Linux code in sysdeps/unix/sysv/linux). Nowadays,
>glibc only supports Linux and Hurd and all these generic interfaces
>are most of time useless and confusing.
>
>I will check if I have some bandwidth to just remove all the unused files,
>there is little point in keeping code that is not actively built. For
>a possible future ports, it would make more sense if they do implement
>the missing symbols explicit instead of relying on broken/incomplete ones.
That being said, I got the impression these files were in fact used by Hurd.
More information about the Libc-alpha
mailing list