Re: [PATCH RFC v3 7/8] termios: change the generic cfsetspeed() to support arbitrary speeds

[H. Peter Anvin]

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.