[Toybox] [PATCH] microcom, stty: Use TCSADRAIN to set tty device attribute
Don't flush the tty device input queue when setting device attribute.
Rationale:
* microcom: The tty device might already have a _good enough_ termios
to read data from. Flushing the input queue just to set the terminal
attribute would result in data loss in this case.
* stty: This command doesn't read nor write data to the tty device, so
flushing the input queue doesn't make sense here. The program
actually talking to the tty should decide if it wants the tty
flushed or not.
Note: other implementations of stty also uses TCSANOW (bsd) or
TCSADRAIN (coreutils), so I think this assumption is fine.
From 861774f11c49db0e9c59d54f84fbc71fc378544a Mon Sep 17 00:00:00 2001
From: Yi-Yo Chiang
Date: Tue, 21 May 2024 12:13:31 +0800
Subject: [PATCH] microcom,stty: Use TCSADRAIN to set tty device attribute
Don't flush the tty device input queue when setting device attribute.
Rationale:
* microcom: The tty device might already have a _good enough_ termios
to read data from. Flushing the input queue just to set the terminal
attribute would result in data loss in this case.
* stty: This command doesn't read nor write data to the tty device, so
flushing the input queue doesn't make sense here. The program
actually talking to the tty should decide if it wants the tty
flushed or not.
Note: other implementations of stty also uses TCSANOW (bsd) or
TCSADRAIN (coreutils), so I think this assumption is fine.
Change-Id: Ia42e65eaae9d70fdef6c2016469070e52071e9c3
---
toys/net/microcom.c | 4 ++--
toys/pending/stty.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/toys/net/microcom.c b/toys/net/microcom.c
index 49ccde0f..d06afb14 100644
--- a/toys/net/microcom.c
+++ b/toys/net/microcom.c
@@ -30,7 +30,7 @@ GLOBALS(
static void restore_states(int i)
{
if (TT.stok) tcsetattr(0, TCSAFLUSH, _stdin);
- tcsetattr(TT.fd, TCSAFLUSH, _fd);
+ tcsetattr(TT.fd, TCSADRAIN, _fd);
}
static void handle_esc(void)
@@ -107,7 +107,7 @@ void microcom_main(void)
memcpy(, _fd, sizeof(struct termios));
cfmakeraw();
xsetspeed(, TT.s);
- if (tcsetattr(TT.fd, TCSAFLUSH, )) perror_exit("set speed");
+ if (tcsetattr(TT.fd, TCSADRAIN, )) perror_exit("set speed");
if (!set_terminal(0, 1, 0, _stdin)) TT.stok++;
// ...and arrange to restore things, however we may exit.
sigatexit(restore_states);
diff --git a/toys/pending/stty.c b/toys/pending/stty.c
index 217c0c2b..25d2d9b4 100644
--- a/toys/pending/stty.c
+++ b/toys/pending/stty.c
@@ -406,7 +406,7 @@ void stty_main(void)
}
}
-tcsetattr(TT.fd, TCSAFLUSH, );
+tcsetattr(TT.fd, TCSADRAIN, );
xtcgetattr();
if (memcmp(, , sizeof(old)))
error_exit("unable to perform all requested operations on %s", TT.F);
--
2.45.0.rc1.225.g2a3ae87e7f-goog
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] microcom.c discarding data due to TCSAFLUSH
On Mon, May 20, 2024 at 10:43 AM Yi-Yo Chiang wrote: > > Found this out today when comparing captured logs. Every time I connect to a > pty the first few hundreds of bytes seem to always be missing. > I then traced to this line > https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L103 and > it's the tcsetattr(TCSAFLUSH) that is discarding buffered input data. > Something like this is happening: > > 1. (process 1) opens and write "" to the ptmx end. The data is not > immediately read by another process, so it's buffered somewhere. (in the pty > driver?) > 2. (process 2 / microcom) opens and flushes (TCSAFLUSH) the pts end, and then > start polling data. Receives "". Part of the buffered input data were > flushed and discarded. > > Man page says (https://man7.org/linux/man-pages/man3/termios.3.html): > >TCSANOW > the change occurs immediately. > >TCSADRAIN > the change occurs after all output written to fd has been > transmitted. This option should be used when changing > parameters that affect output. > >TCSAFLUSH > the change occurs after all output written to the object > referred by fd has been transmitted, and all input that > has been received but not read will be discarded before > the change is made. > > Is there any particular reason to use TCSAFLUSH here? > If not, can we change to TCSADRAIN or TCSANOW. I don't think there is good > reason to _discard received data_ just to set the terminal mode...? Is there > really a real world case that the device termios is so dirty that all data, > from before setting raw mode, must be discarded? not that i know of, but iirc that was my thinking here --- "i don't know what state this is in; let's just get rid of it". now you have a motivating example for not doing that :-) > I also tried to modify the microcom code to skip tcsetattr() if the device > termios is already equal to the mode we are setting it. > `if (old_termios != new_termios) tcsetattr(new_termios, TCSAFLUSH)` > However this doesn't work because microcom always tries to set the device > baud. For example a pty device might be configured to use buad 38400, but > microcom would want it to be 115200, thus flushing it's data. but pty doesn't > really care about the baud most of the time AFAIK, so flushing data in this > case just seems disruptive to the user experience. ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
[Toybox] microcom.c discarding data due to TCSAFLUSH
Found this out today when comparing captured logs. Every time I connect to a pty the first few hundreds of bytes seem to always be missing. I then traced to this line https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L103 and it's the tcsetattr(TCSAFLUSH) that is discarding buffered input data. Something like this is happening: 1. (process 1) opens and write "" to the ptmx end. The data is not immediately read by another process, so it's buffered somewhere. (in the pty driver?) 2. (process 2 / microcom) opens and flushes (TCSAFLUSH) the pts end, and then start polling data. Receives "". Part of the buffered input data were flushed and discarded. Man page says (https://man7.org/linux/man-pages/man3/termios.3.html): TCSANOW the change occurs immediately. TCSADRAIN the change occurs after all output written to fd has been transmitted. This option should be used when changing parameters that affect output. TCSAFLUSH the change occurs after all output written to the object referred by fd has been transmitted, and all input that has been received but not read will be discarded before the change is made. Is there any particular reason to use TCSAFLUSH here? If not, can we change to TCSADRAIN or TCSANOW. I don't think there is good reason to _discard received data_ just to set the terminal mode...? Is there really a real world case that the device termios is so dirty that all data, from before setting raw mode, must be discarded? I also tried to modify the microcom code to skip tcsetattr() if the device termios is already equal to the mode we are setting it. `if (old_termios != new_termios) tcsetattr(new_termios, TCSAFLUSH)` However this doesn't work because microcom always tries to set the device baud. For example a pty device might be configured to use buad 38400, but microcom would want it to be 115200, thus flushing it's data. but pty doesn't really care about the baud most of the time AFAIK, so flushing data in this case just seems disruptive to the user experience. ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
[Toybox] [PATCH] microcom: Don't crash if failed to open paste file
* When running the "paste" command and failed to open the paste file (such as file not found or permission error), don't crash the entire microcom program. Instead print the error message and give the user a chance to fix the problem. * If "paste" command is cancelled by "ESC" or an empty file name, clear the hanging "Filename:" prompt before returning to the main loop. -- Yi-yo Chiang Software Engineer yochi...@google.com 0001-microcom-Don-t-crash-if-failed-to-open-paste-file.patch Description: Binary data ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] xputs: Do flush
Thanks! Adding TOYFLAG_NOBUF worked.
I feel the same way about "manual flushing of the output buffer is a
terrible interface". I asked myself the question "Why am I manually
flushing so much? There must be a better way..." multiple times when I
wrote the other patch that does s/xprintf/dprintf/, s/xputs/xputsn/
On Mon, May 20, 2024 at 8:36 PM enh wrote:
> On Sun, May 19, 2024 at 10:56 PM Rob Landley wrote:
> >
> > On 5/18/24 21:53, Yi-Yo Chiang wrote:
> > > What I wanted to address with this patch are:
> > >
> > > 1. Fix this line of
> > > xputs()
> https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113
> > > The prompt text is not flushed immediately, so it is not shown to the
> user until
> > > the escape char is entered (which defeats the purpose of the prompt,
> that is to
> >
> > I agree you've identified two problems (unflushed prompt, comment not
> matching
> > code) that both need to be fixed. I'm just unhappy with the solutions,
> and am
> > concerned about a larger design problem.
> >
> > I implemented TOYFLAG_NOBUF and applied it to this command. The result
> compiles
> > but I'm not near serial hardware at the moment, does it fix the problem
> for you?
> >
> > Trying to fix it via micromanagement (adding more flushing and switching
> some
> > but not all output contexts in the same command between FILE * and file
> > descriptor) makes my head hurt...
> >
> > Adding flushing to xputs() is an Elliott question is because he's the
> one who
> > can presumably keep stdio buffer semantics in his head. They make zero
> sense to
> > me. I added a flush to xputsn() because in line buffering mode it was the
> > "dangerous" one that had no newline thus no flush, but then when we go
> to block
> > buffering mode xputs() needs a flush just like xputsn() would, and MAYBE
> it's
> > good to have the flush because in line buffer mode it would be a NOP?
> Except the
> > command selected block buffering mode precisely BECAUSE it didn't want
> to flush
> > each line, so why should xputs() do it when the command asked not to?
> And if
> > xputs() is doing it, why is xprintf() _not_ doing it? And if xprintf()
> _is_
> > doing it, then we're back to basically not buffering the output...
>
> this to me was exactly why it should be "everything flushes" or
> "nothing flushes". not "some subset of calls for some subset of
> inputs", because no-one can keep all the special cases in their heads.
> and "everything flushes" is problematic from a performance
> perspective, so "nothing flushes" wins by default. (but, yes, when we
> have our own kernel, have a time-based component to buffering layer's
> flushing is an interesting idea :-) )
>
> > > tell the user what the escape char is) and stdout is flushed by
> handle_esc.
> > > To fix this we either make xputs() flush automatically, or we just add
> a single
> > > line of manual flush() after xputs() in microcom.c.
> > > Either is fine with me.
> >
> > When I searched for the first xputs in microcom I got:
> >
> > xputsn("\r\n[b]reak, [p]aste file, [q]uit: ");
> > if (read(0, , 1)<1 || input == CTRL('D') || input == 'q') {
> >
> > Which is a separate function (the n version is no newline, it does not
> add the
> > newline the way libc puts() traditionally does), with its own flushing
> > semantics: xputsn() doesn't call xputs(), and neither calls or is called
> by
> > xprintf(). "Some functions flush, some functions don't" is a bit of a
> design
> > sharp edge.
>
> (yeah, exactly.)
>
> > The larger problem is manual flushing of the output buffer is a terrible
> > interface, and leads to missing error checking without which a command
> won't
> > reliably exit when its output terminal closes because the whole SIGPIPE
> thing
> > was its own can of worms that even bionic used to manually mess with.
> Which is
> > why I originally made toybox not ever do that (systemic fix) but I got
> > complaints about performance.
> >
> > Your other patch changes a bunch of xprintf() to dprintf() which is even
> _more_
> > fun because dprintf() writes directly to the file descriptor (bypassing
> the
> > buffer in the libc global FILE * instance "stdio"), which means in the
> absence
> > of manual flushing the dprintf() data will go out first and then the
> stdio data
> > will go out sometime later, in the wrong order. Mixing the two output
> formats
> > tends to invert the order that way, unless you disable output buffering.
>
Which is the reason I replaced those all with the "flush" functions
(xputsn) or direct fd-write functions (dprintf), so that their order won't
shuffle.
Anyway the problem is moot now that we have TOYFLAG_NOBUF.
> >
> > I like systematic solutions that mean the problem never comes up again.
> Elliott
> > has been advocating the whack-a-mole "fix them as we find them" approach
> here
> > which I am not comfortable with. I've been leaning towards adding a
> > TOYFLAG_NOBUF so we can select a third buffering type, and go back to
> "do not
> >
Re: [Toybox] [PATCH] xputs: Do flush
On Sun, May 19, 2024 at 10:56 PM Rob Landley wrote:
>
> On 5/18/24 21:53, Yi-Yo Chiang wrote:
> > What I wanted to address with this patch are:
> >
> > 1. Fix this line of
> > xputs()
> > https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113
> > The prompt text is not flushed immediately, so it is not shown to the user
> > until
> > the escape char is entered (which defeats the purpose of the prompt, that
> > is to
>
> I agree you've identified two problems (unflushed prompt, comment not matching
> code) that both need to be fixed. I'm just unhappy with the solutions, and am
> concerned about a larger design problem.
>
> I implemented TOYFLAG_NOBUF and applied it to this command. The result
> compiles
> but I'm not near serial hardware at the moment, does it fix the problem for
> you?
>
> Trying to fix it via micromanagement (adding more flushing and switching some
> but not all output contexts in the same command between FILE * and file
> descriptor) makes my head hurt...
>
> Adding flushing to xputs() is an Elliott question is because he's the one who
> can presumably keep stdio buffer semantics in his head. They make zero sense
> to
> me. I added a flush to xputsn() because in line buffering mode it was the
> "dangerous" one that had no newline thus no flush, but then when we go to
> block
> buffering mode xputs() needs a flush just like xputsn() would, and MAYBE it's
> good to have the flush because in line buffer mode it would be a NOP? Except
> the
> command selected block buffering mode precisely BECAUSE it didn't want to
> flush
> each line, so why should xputs() do it when the command asked not to? And if
> xputs() is doing it, why is xprintf() _not_ doing it? And if xprintf() _is_
> doing it, then we're back to basically not buffering the output...
this to me was exactly why it should be "everything flushes" or
"nothing flushes". not "some subset of calls for some subset of
inputs", because no-one can keep all the special cases in their heads.
and "everything flushes" is problematic from a performance
perspective, so "nothing flushes" wins by default. (but, yes, when we
have our own kernel, have a time-based component to buffering layer's
flushing is an interesting idea :-) )
> > tell the user what the escape char is) and stdout is flushed by handle_esc.
> > To fix this we either make xputs() flush automatically, or we just add a
> > single
> > line of manual flush() after xputs() in microcom.c.
> > Either is fine with me.
>
> When I searched for the first xputs in microcom I got:
>
> xputsn("\r\n[b]reak, [p]aste file, [q]uit: ");
> if (read(0, , 1)<1 || input == CTRL('D') || input == 'q') {
>
> Which is a separate function (the n version is no newline, it does not add the
> newline the way libc puts() traditionally does), with its own flushing
> semantics: xputsn() doesn't call xputs(), and neither calls or is called by
> xprintf(). "Some functions flush, some functions don't" is a bit of a design
> sharp edge.
(yeah, exactly.)
> The larger problem is manual flushing of the output buffer is a terrible
> interface, and leads to missing error checking without which a command won't
> reliably exit when its output terminal closes because the whole SIGPIPE thing
> was its own can of worms that even bionic used to manually mess with. Which is
> why I originally made toybox not ever do that (systemic fix) but I got
> complaints about performance.
>
> Your other patch changes a bunch of xprintf() to dprintf() which is even
> _more_
> fun because dprintf() writes directly to the file descriptor (bypassing the
> buffer in the libc global FILE * instance "stdio"), which means in the absence
> of manual flushing the dprintf() data will go out first and then the stdio
> data
> will go out sometime later, in the wrong order. Mixing the two output formats
> tends to invert the order that way, unless you disable output buffering.
>
> I like systematic solutions that mean the problem never comes up again.
> Elliott
> has been advocating the whack-a-mole "fix them as we find them" approach here
> which I am not comfortable with. I've been leaning towards adding a
> TOYFLAG_NOBUF so we can select a third buffering type, and go back to "do not
> buffer output at all, flush after every ANSI write function" for at least some
> commands I'd like to be able to reason about. Especially for commands like
> this
> where output performance really doesn't seem like it should be an issue.
+1 --- an inherently interactive program like this seems reasonable to
be unbuffered. (except for file transfer?)
> And OTHER implementations can't consistently get this right, which is why
> whether:
>
> for i in {1..100}; do echo -n .; sleep .1; done | less
>
> Produces any output before 10 seconds have elapsed is potluck, and varies from
> release to release of the same distro.
>
> Oh, and the gnu/crazies just came up with a fourth category of write as a
> gnu/extension: flush after NUL byte.
[Toybox] [PATCH] netcat: clarify documentation.
"Collate" means "sort", but -O is like -o other than buffering. --- toys/net/netcat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 0001-netcat-clarify-documentation.patch Description: Binary data ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
