* bug#49524: 28.0.50; make-serial-process is not portable
@ 2021-07-11 15:24 Ken Brown
2021-07-11 16:24 ` Eli Zaretskii
0 siblings, 1 reply; 6+ messages in thread
From: Ken Brown @ 2021-07-11 15:24 UTC (permalink / raw)
To: 49524
Fmake_serial_process calls Fserial_process_configure, which calls
serial_configure, which calls cfsetspeed with the speed argument equal to the
numerical baud rate (e.g., 9600). But the documentation of cfsetspeed says that
the speed argument must be one of the Bnnn constants defined in termios.h (e.g.,
B9600). See, for example,
https://man7.org/linux/man-pages/man3/termios.3.html
This incorrect call of cfsetspeed happens to succeed on GNU/Linux because
glibc's cfsetspeed allows the argument to be the numerical baud rate, which it
converts to the appropriate Bnnn constant. But I don't think emacs should be
relying on this undocumented behavior. In particular, this doesn't work on
Cygwin. And it wouldn't even work on GNU/Linux if emacs used the cfsetspeed
replacement defined in sysdep.c instead of glibc's cfsetspeed.
I think the way to fix this is to imitate the glibc code that converts the baud
rate to a Bnnn constant, but maybe someone has a better idea.
By the way, I came across this issue while investigating the failure of
process-tests/fd-setsize-no-crash/make-serial-process on Cygwin.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#49524: 28.0.50; make-serial-process is not portable
2021-07-11 15:24 bug#49524: 28.0.50; make-serial-process is not portable Ken Brown
@ 2021-07-11 16:24 ` Eli Zaretskii
2021-07-12 13:27 ` Ken Brown
0 siblings, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2021-07-11 16:24 UTC (permalink / raw)
To: Ken Brown; +Cc: 49524
> From: Ken Brown <kbrown@cornell.edu>
> Date: Sun, 11 Jul 2021 11:24:58 -0400
>
> Fmake_serial_process calls Fserial_process_configure, which calls
> serial_configure, which calls cfsetspeed with the speed argument equal to the
> numerical baud rate (e.g., 9600). But the documentation of cfsetspeed says that
> the speed argument must be one of the Bnnn constants defined in termios.h (e.g.,
> B9600). See, for example,
>
> https://man7.org/linux/man-pages/man3/termios.3.html
>
> This incorrect call of cfsetspeed happens to succeed on GNU/Linux because
> glibc's cfsetspeed allows the argument to be the numerical baud rate, which it
> converts to the appropriate Bnnn constant. But I don't think emacs should be
> relying on this undocumented behavior. In particular, this doesn't work on
> Cygwin. And it wouldn't even work on GNU/Linux if emacs used the cfsetspeed
> replacement defined in sysdep.c instead of glibc's cfsetspeed.
>
> I think the way to fix this is to imitate the glibc code that converts the baud
> rate to a Bnnn constant, but maybe someone has a better idea.
Converting in sysdep.c:serial_configure sounds TRT to me.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#49524: 28.0.50; make-serial-process is not portable
2021-07-11 16:24 ` Eli Zaretskii
@ 2021-07-12 13:27 ` Ken Brown
2021-07-12 21:35 ` Ken Brown
0 siblings, 1 reply; 6+ messages in thread
From: Ken Brown @ 2021-07-12 13:27 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 49524
[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]
On 7/11/2021 12:24 PM, Eli Zaretskii wrote:
>> From: Ken Brown <kbrown@cornell.edu>
>> Date: Sun, 11 Jul 2021 11:24:58 -0400
>>
>> Fmake_serial_process calls Fserial_process_configure, which calls
>> serial_configure, which calls cfsetspeed with the speed argument equal to the
>> numerical baud rate (e.g., 9600). But the documentation of cfsetspeed says that
>> the speed argument must be one of the Bnnn constants defined in termios.h (e.g.,
>> B9600). See, for example,
>>
>> https://man7.org/linux/man-pages/man3/termios.3.html
>>
>> This incorrect call of cfsetspeed happens to succeed on GNU/Linux because
>> glibc's cfsetspeed allows the argument to be the numerical baud rate, which it
>> converts to the appropriate Bnnn constant. But I don't think emacs should be
>> relying on this undocumented behavior. In particular, this doesn't work on
>> Cygwin. And it wouldn't even work on GNU/Linux if emacs used the cfsetspeed
>> replacement defined in sysdep.c instead of glibc's cfsetspeed.
>>
>> I think the way to fix this is to imitate the glibc code that converts the baud
>> rate to a Bnnn constant, but maybe someone has a better idea.
>
> Converting in sysdep.c:serial_configure sounds TRT to me.
Patch attached.
Ken
[-- Attachment #2: 0001-Fix-portability-issue-with-make-serial-process.patch --]
[-- Type: text/plain, Size: 3559 bytes --]
From aeda8d9e4432e5fac627341968e84c98f4df70bd Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Mon, 12 Jul 2021 09:24:12 -0400
Subject: [PATCH] Fix portability issue with make-serial-process
* src/sysdep.c (struct speed_struct): New struct.
(speeds): New static array of struct speed_struct.
(convert_speed): New static function to convert a numerical baud
rate (e.g., 9600) to a Bnnn constant defined in termios.h (e.g.,
B9600).
(serial_configure): Use convert_speed to avoid cfsetspeed errors
on non-glibc platforms. (Bug#49524)
---
src/sysdep.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 133 insertions(+), 1 deletion(-)
diff --git a/src/sysdep.c b/src/sysdep.c
index b8ec22d9dd..8eaee22498 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -2744,6 +2744,138 @@ cfsetspeed (struct termios *termios_p, speed_t vitesse)
}
#endif
+/* The following is based on the glibc implementation of cfsetspeed. */
+
+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
+ };
+
+/* Convert a numerical speed (e.g., 9600) to a Bnnn constant (e.g.,
+ B9600); see bug#49524. */
+static speed_t
+convert_speed (speed_t speed)
+{
+ for (size_t i = 0; i < sizeof speeds / sizeof speeds[0]; i++)
+ {
+ if (speed == speeds[i].internal)
+ return speed;
+ else if (speed == speeds[i].value)
+ return speeds[i].internal;
+ }
+ return speed;
+}
+
/* For serial-process-configure */
void
serial_configure (struct Lisp_Process *p,
@@ -2775,7 +2907,7 @@ serial_configure (struct Lisp_Process *p,
else
tem = Fplist_get (p->childp, QCspeed);
CHECK_FIXNUM (tem);
- err = cfsetspeed (&attr, XFIXNUM (tem));
+ err = cfsetspeed (&attr, convert_speed (XFIXNUM (tem)));
if (err != 0)
report_file_error ("Failed cfsetspeed", tem);
childp2 = Fplist_put (childp2, QCspeed, tem);
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#49524: 28.0.50; make-serial-process is not portable
2021-07-12 13:27 ` Ken Brown
@ 2021-07-12 21:35 ` Ken Brown
2021-07-13 11:19 ` Eli Zaretskii
0 siblings, 1 reply; 6+ messages in thread
From: Ken Brown @ 2021-07-12 21:35 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 49524
On 7/12/2021 9:27 AM, Ken Brown wrote:
> On 7/11/2021 12:24 PM, Eli Zaretskii wrote:
>>> From: Ken Brown <kbrown@cornell.edu>
>>> Date: Sun, 11 Jul 2021 11:24:58 -0400
>>>
>>> Fmake_serial_process calls Fserial_process_configure, which calls
>>> serial_configure, which calls cfsetspeed with the speed argument equal to the
>>> numerical baud rate (e.g., 9600). But the documentation of cfsetspeed says that
>>> the speed argument must be one of the Bnnn constants defined in termios.h (e.g.,
>>> B9600). See, for example,
>>>
>>> https://man7.org/linux/man-pages/man3/termios.3.html
>>>
>>> This incorrect call of cfsetspeed happens to succeed on GNU/Linux because
>>> glibc's cfsetspeed allows the argument to be the numerical baud rate, which it
>>> converts to the appropriate Bnnn constant. But I don't think emacs should be
>>> relying on this undocumented behavior. In particular, this doesn't work on
>>> Cygwin. And it wouldn't even work on GNU/Linux if emacs used the cfsetspeed
>>> replacement defined in sysdep.c instead of glibc's cfsetspeed.
>>>
>>> I think the way to fix this is to imitate the glibc code that converts the baud
>>> rate to a Bnnn constant, but maybe someone has a better idea.
>>
>> Converting in sysdep.c:serial_configure sounds TRT to me.
>
> Patch attached.
BTW, we've decided to change Cygwin's cfsetspeed to be compatible with glibc's,
so the problem fixed by my patch won't exist on Cygwin going forward. And I
checked FreeBSD out of curiosity and found that there's no issue there either
because of the way they define the Bnnn constants:
#define B0 0
#define B50 50
#define B75 75
...
So I should probably remove the reference to non-glibc platforms in my commit
message.
Ken
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#49524: 28.0.50; make-serial-process is not portable
2021-07-12 21:35 ` Ken Brown
@ 2021-07-13 11:19 ` Eli Zaretskii
2021-07-13 13:11 ` Ken Brown
0 siblings, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2021-07-13 11:19 UTC (permalink / raw)
To: Ken Brown; +Cc: 49524
> From: Ken Brown <kbrown@cornell.edu>
> Cc: 49524@debbugs.gnu.org
> Date: Mon, 12 Jul 2021 17:35:40 -0400
>
> So I should probably remove the reference to non-glibc platforms in my commit
> message.
Yes, something to the effect of compliance with advertised APIs would
probably be better.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#49524: 28.0.50; make-serial-process is not portable
2021-07-13 11:19 ` Eli Zaretskii
@ 2021-07-13 13:11 ` Ken Brown
0 siblings, 0 replies; 6+ messages in thread
From: Ken Brown @ 2021-07-13 13:11 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 49524-done
On 7/13/2021 7:19 AM, Eli Zaretskii wrote:
>> From: Ken Brown <kbrown@cornell.edu>
>> Cc: 49524@debbugs.gnu.org
>> Date: Mon, 12 Jul 2021 17:35:40 -0400
>>
>> So I should probably remove the reference to non-glibc platforms in my commit
>> message.
>
> Yes, something to the effect of compliance with advertised APIs would
> probably be better.
Done. Closing.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-13 13:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-11 15:24 bug#49524: 28.0.50; make-serial-process is not portable Ken Brown
2021-07-11 16:24 ` Eli Zaretskii
2021-07-12 13:27 ` Ken Brown
2021-07-12 21:35 ` Ken Brown
2021-07-13 11:19 ` Eli Zaretskii
2021-07-13 13:11 ` Ken Brown
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).