Re: svn commit: r311952 - head/sys/ddb

[Bruce Evans]

On Mon, 16 Jan 2017, Julian Elischer wrote:


On 16/01/2017 11:53 AM, Bruce Evans wrote:

On Sun, 15 Jan 2017, Adrian Chadd wrote:


hah, i took this, then life got in the way. :)

Bruce - what do you think of
https://bz-attachments.freebsd.org/attachment.cgi?id=138881 ?


Also https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=184987 .

I think it is hard to read and review since it is not in the mail.
After fetching it and quoting it:


review it in the review page and just send a link in the email.. that's what 
it's for.


That's what is hard to read and harder to review.

Bruce
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r311952 - head/sys/ddb

[Julian Elischer]

On 16/01/2017 11:53 AM, Bruce Evans wrote:

On Sun, 15 Jan 2017, Adrian Chadd wrote:


hah, i took this, then life got in the way. :)

Bruce - what do you think of
https://bz-attachments.freebsd.org/attachment.cgi?id=138881 ?


Also https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=184987 .

I think it is hard to read and review since it is not in the mail.
After fetching it and quoting it:


review it in the review page and just send a link in the email.. 
that's what it's for.




X diff -r 91820d9948e0 -r 13f40d577646 sys/kern/tty_ttydisc.c
X --- a/sys/kern/tty_ttydisc.cFri Feb 22 09:45:32 2013 +0100
X +++ b/sys/kern/tty_ttydisc.cTue Dec 17 23:03:17 2013 +0100
X @@ -448,6 +448,9 @@
X  if (tp->t_flags & TF_ZOMBIE)
X  return (EIO);
X X +if (tp->t_termios.c_lflag & FLUSHO)
X +return (0);
X +

This seems to be far too simple.  As pointed out in the PR thread, it
is missing at least the clearing of uio_resid.

   (BTW, returns for TF_ZOMBIE and IO_NDELAY tend to have the opposite
   problem.  The tend to reduce uio_resid to count i/o that they have
   not done.  They should just return an error like the above return
   for TF_ZOMBIE does.  Upper layers are supposed to translate this
   error to success if any i/o has been done.  Upper layers often get
   this wrong, and my fixes currently do too much compensation in lower
   layers.)

The old tty driver has squillions of FLUSHO checks for output not 
involving
uio.  Mostly for echo of input.  However, any input (and other 
operations?)

normally turns of FLUSHO, so this is hard to see.

To see if this works compatibly now, try typing 111^O2.  ^O should be
echoed as ^O^R followed by reprinting the line according to the 
implicit

VREPRINT (^R).  This should cancel FLUSHO, so the 2 is just echoed on
the new line.  However ^O instead of 2 should just cancel FLUSHO. So
^O^O should be equivalent to ^R except it prints ^O before ^R.

It is wrong for FLUSHO to have any effect except in modes where 
VDISCARD

has an effect.  VDISCARD seems to be conditional on precisely
!EXTPROC && IEXTEN.  Its inactivity in other modes should be 
implemented
by forcing it off on certain mode changes.  The old tty driver 
forces it

off in squillions of places, perhaps including all mode changes for
simplicity.  There would be a problem if stty(1) could actually set it,
since then invalid settings like "raw flusho" would be allowed. 
Clearing

it after all mode changes would prevent stty actually setting it.


X  /*
X   * We don't need to check whether the process is the foreground
X   * process group or if we have a carrier. This is already done
X @@ -881,6 +884,14 @@
X X  /* Special control characters that are implementation 
dependent. */

X  if (CMP_FLAG(l, IEXTEN)) {
X +/* Discard (^O) */

Style bug.  This comment is missing a "." and does less than echo 
the code.
The code doesn't hard-code VDISCARD as ^O, and it is unclear whether 
the

comment applies to the previous or the next line (except it is further
from echoing the previous line).

X +if (CMP_CC(VDISCARD, c)) {
X +if (!(tp->t_termios.c_lflag & FLUSHO))

Style bug.  This file elsewhere always uses the funky style of explicit
comparison of (sets of ) flags with 0.

X +ttyoutq_write_nofrag(>t_outq, "^O", 2);

This also hard-codes ^O.  The old driver echos the actual discard 
character
using ttyecho(c, tp).  I don't know if the funky rules for quoting 
control
characters apply in this case, but ttyecho() has to be quite 
complicated to

handle general cases.

X +tp->t_termios.c_lflag ^= FLUSHO;

The old driver adds an implicit VREPRINT char here if the input 
queue is
nonempty.  This usually results in echoing ^O^R and reprinting the 
line.


X +return(0);
X +}
X +

Otherwise, the action here is identical with the old driver.

X  /* Accept the next character as literal. */
X  if (CMP_CC(VLNEXT, c)) {
X  if (CMP_FLAG(l, ECHO)) {

The old driver needs 23 lines mentioning FLUSHO to keep it mostly 
clear.

These are:
- 3 in compat code (still there)
- 3 here
- 1 clear whenever restarting output at the end of interrupt input 
(most

  cases interrupt input)
- 4 checks in ttyoutput() which is used manly for echoing.  1 at the
  beginning would be simpler, but the checks are more or less before
  each putc() and there are many inconsistences for counting characters
  and the column position from the complications
- 1 clear in ttioctl() in 1 case of turning 1 type of flow control 
back on.
  Buggy, since FLUSHO should not be affected by flow control except 
by the

  xon char being treated as an ordinary char for canceling VDISCARD.
- 1 clear in ttioctl() for the same flow control done by TIOCSTART
- 1 check for ordinary writes as in this patch
- 2 in comments
- 2 more checks in the loop for ordinary writes.  Some checks are 
necessary,
  but these are misplaced.  FLUSHO can 

Re: svn commit: r311952 - head/sys/ddb

[Bruce Evans]

On Sun, 15 Jan 2017, Adrian Chadd wrote:


hah, i took this, then life got in the way. :)

Bruce - what do you think of
https://bz-attachments.freebsd.org/attachment.cgi?id=138881 ?


Also https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=184987 .

I think it is hard to read and review since it is not in the mail.
After fetching it and quoting it:

X diff -r 91820d9948e0 -r 13f40d577646 sys/kern/tty_ttydisc.c
X --- a/sys/kern/tty_ttydisc.c  Fri Feb 22 09:45:32 2013 +0100
X +++ b/sys/kern/tty_ttydisc.c  Tue Dec 17 23:03:17 2013 +0100
X @@ -448,6 +448,9 @@
X   if (tp->t_flags & TF_ZOMBIE)
X   return (EIO);
X 
X +	if (tp->t_termios.c_lflag & FLUSHO)

X + return (0);
X +

This seems to be far too simple.  As pointed out in the PR thread, it
is missing at least the clearing of uio_resid.

   (BTW, returns for TF_ZOMBIE and IO_NDELAY tend to have the opposite
   problem.  The tend to reduce uio_resid to count i/o that they have
   not done.  They should just return an error like the above return
   for TF_ZOMBIE does.  Upper layers are supposed to translate this
   error to success if any i/o has been done.  Upper layers often get
   this wrong, and my fixes currently do too much compensation in lower
   layers.)

The old tty driver has squillions of FLUSHO checks for output not involving
uio.  Mostly for echo of input.  However, any input (and other operations?)
normally turns of FLUSHO, so this is hard to see.

To see if this works compatibly now, try typing 111^O2.  ^O should be
echoed as ^O^R followed by reprinting the line according to the implicit
VREPRINT (^R).  This should cancel FLUSHO, so the 2 is just echoed on
the new line.  However ^O instead of 2 should just cancel FLUSHO.  So
^O^O should be equivalent to ^R except it prints ^O before ^R.

It is wrong for FLUSHO to have any effect except in modes where VDISCARD
has an effect.  VDISCARD seems to be conditional on precisely
!EXTPROC && IEXTEN.  Its inactivity in other modes should be implemented
by forcing it off on certain mode changes.  The old tty driver forces it
off in squillions of places, perhaps including all mode changes for
simplicity.  There would be a problem if stty(1) could actually set it,
since then invalid settings like "raw flusho" would be allowed.  Clearing
it after all mode changes would prevent stty actually setting it.


X   /*
X* We don't need to check whether the process is the foreground
X* process group or if we have a carrier. This is already done
X @@ -881,6 +884,14 @@
X 
X  	/* Special control characters that are implementation dependent. */

X   if (CMP_FLAG(l, IEXTEN)) {
X + /* Discard (^O) */

Style bug.  This comment is missing a "." and does less than echo the code.
The code doesn't hard-code VDISCARD as ^O, and it is unclear whether the
comment applies to the previous or the next line (except it is further
from echoing the previous line).

X + if (CMP_CC(VDISCARD, c)) {
X + if (!(tp->t_termios.c_lflag & FLUSHO))

Style bug.  This file elsewhere always uses the funky style of explicit
comparison of (sets of ) flags with 0.

X + ttyoutq_write_nofrag(>t_outq, "^O", 2);

This also hard-codes ^O.  The old driver echos the actual discard character
using ttyecho(c, tp).  I don't know if the funky rules for quoting control
characters apply in this case, but ttyecho() has to be quite complicated to
handle general cases.

X + tp->t_termios.c_lflag ^= FLUSHO;

The old driver adds an implicit VREPRINT char here if the input queue is
nonempty.  This usually results in echoing ^O^R and reprinting the line.

X + return(0);
X + }
X +

Otherwise, the action here is identical with the old driver.

X   /* Accept the next character as literal. */
X   if (CMP_CC(VLNEXT, c)) {
X   if (CMP_FLAG(l, ECHO)) {

The old driver needs 23 lines mentioning FLUSHO to keep it mostly clear.
These are:
- 3 in compat code (still there)
- 3 here
- 1 clear whenever restarting output at the end of interrupt input (most
  cases interrupt input)
- 4 checks in ttyoutput() which is used manly for echoing.  1 at the
  beginning would be simpler, but the checks are more or less before
  each putc() and there are many inconsistences for counting characters
  and the column position from the complications
- 1 clear in ttioctl() in 1 case of turning 1 type of flow control back on.
  Buggy, since FLUSHO should not be affected by flow control except by the
  xon char being treated as an ordinary char for canceling VDISCARD.
- 1 clear in ttioctl() for the same flow control done by TIOCSTART
- 1 check for ordinary writes as in this patch
- 2 in comments
- 2 more checks in the loop for ordinary writes.  Some checks are necessary,
  but these are misplaced.  FLUSHO can change underneath when we unlock to
  do the i/o, so it must be re-checked when we 

Re: svn commit: r311952 - head/sys/ddb

[Bruce Evans]

On Sun, 15 Jan 2017, Slawa Olhovchenkov wrote:


On Sun, Jan 15, 2017 at 01:07:58PM +0800, Julian Elischer wrote:



the old DEC machines had (from memory) ^O  which dropped all pending
output.
Not so useful when there is 2MB of network buffers queued up to you,
but very useful on
and asr-33 teletype.


Now it useful too, on serial console.
^O processing be present years ago in FreeBSD, but removed at time of
tty code rewrite. As I know patch for restore exist, nobody
review/commit.


Urk.  I have fixes for hundreds of things broken in the rewrite, but didn't
notice this one.  The only fix I have near here is a partial one for funky
quoting of control characters according to ECHOK and ECHOKE.  ECHOK is a
POSIX feature and ECHOKE is a BSD feature.  The man page still docoments
the old behaviour.

Grepping for CCEQ (and its renaming to the worse name CMP_FLAG in the
rewrite) shows that the only other control character with lost support
is VDSUSP.

Zgrepping for VDISCARD and VDSUSP in /usr/share/man shows the following
bugs:
- VDISCARD and VDSUSP are listed but not described in any useful way in
  termios(4)
- VDSUSP (and dsusp) is listed but not described in any useful way in
  stty(1)
- VDISCARD (and discard) are not mentioned in stty(1).  However,
  'stty discard c' works, and stty's display of the discard character
  works.  stty's displays exactly the special characters that used to
  work.
- VDISCARD and VDSUSP are otherwise undocumented.
- FLUSHO in termios(4) and flusho in stty(1) are listed and described, but
  their close connection with VDISCARD is not mentioned (except stty says
  "being discarded").  FLUSHO is mostly an internal flag.  In the working
  version, It is normally toggled by VDISCARD but is cleared by other actions.
  It can also be controlled by an ioctl, and stty supports the ioctl.  But
  settings by stty are rarely useful since it is normally cleared by other
  The patch in the PR doesn't handle the other actions right.
  actions which stty can't control.

Partial fixes for ECHOKE and nearby things:

X Index: tty_ttydisc.c
X ===
X --- tty_ttydisc.c (revision 312117)
X +++ tty_ttydisc.c (working copy)
X @@ -798,6 +842,16 @@
X   }
X   } else {
X   /* Don't print spaces. */
X + /*
X +  * XXX broken if VERASE is disabled.  Can get here
X +  * then if ECHO is off and:
X +  * - for VERASE2 if that is not disabled
X +  * - for the buggy ECHOK/ECHOKE cases
X +  * - perhaps in other cases.
X +  * This routine is too generic.
X +  *
X +  * XXX the logic is not just non-printing of spaces.
X +  */
X   ttydisc_echo(tp, tp->t_termios.c_cc[VERASE], 0);
X   }
X   }
X @@ -993,7 +1049,30 @@
X   ttydisc_rubchar(tp);
X   return (0);
X   } else if (CMP_CC(VKILL, c)) {
X +#if 0
X + /*
X +  * XXX broken.  This doesn't even look at ECHOK or
X +  * ECHOKE (nothing does now), and it doesn't produce
X +  * the documented behaviour of ECHOK.  ECHOK exists
X +  * to be a non-erasing version of ECHOKE that uses
X +  * fewer terminal capabilities.  ECHOPRT is also
X +  * broken (never even looked at).
X +  */
X   while (ttydisc_rubchar(tp) == 0);
X +#else
X + if (CMP_FLAG(l, ECHOKE)) {
X + /* XXX too hard to fix. */
X + while (ttydisc_rubchar(tp) == 0)
X + ;
X + } else {
X + ttydisc_echo_force(tp, c, 0);
X + if (CMP_FLAG(l, ECHOK))
X + ttydisc_echo_force(tp, '\n', 0);
X + while (ttyinq_peekchar(>t_inq, ,
X + ) == 0)
X + ttyinq_unputchar(>t_inq);
X + }
X +#endif
X   return (0);
X   } else if (CMP_FLAG(l, IEXTEN)) {
X   if (CMP_CC(VWERASE, c)) {
X @@ -1021,12 +1100,13 @@
X 
X  parmrk:

X   if (CMP_FLAG(i, PARMRK)) {
X - /* Prepend 0xff 0x00 0x.. */
X + /* Prepend 0xff 0x00. */
X   ob[2] = c;
X   ol = 3;
X   quote = 1;
X   } else {
X - ob[0] = c;
X + /* Kill the character for read().  Keep it in c for echoing. */
X + ob[0] = '\0';
X   ol = 1;
X   }
X

Bruce
___
svn-src-head@freebsd.org 

Re: svn commit: r311952 - head/sys/ddb

[Slawa Olhovchenkov]

On Sun, Jan 15, 2017 at 10:46:35AM -0800, Adrian Chadd wrote:

> url? :)

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=184987
Assignee:   Adrian Chadd

> On 15 January 2017 at 06:45, Slawa Olhovchenkov  wrote:
> > On Sun, Jan 15, 2017 at 01:07:58PM +0800, Julian Elischer wrote:
> >
> >>
> >> the old DEC machines had (from memory) ^O  which dropped all pending
> >> output.
> >> Not so useful when there is 2MB of network buffers queued up to you,
> >> but very useful on
> >> and asr-33 teletype.
> >
> > Now it useful too, on serial console.
> > ^O processing be present years ago in FreeBSD, but removed at time of
> > tty code rewrite. As I know patch for restore exist, nobody
> > review/commit.
> >
> >
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r311952 - head/sys/ddb

[Adrian Chadd]

url? :)

-a


On 15 January 2017 at 06:45, Slawa Olhovchenkov  wrote:
> On Sun, Jan 15, 2017 at 01:07:58PM +0800, Julian Elischer wrote:
>
>>
>> the old DEC machines had (from memory) ^O  which dropped all pending
>> output.
>> Not so useful when there is 2MB of network buffers queued up to you,
>> but very useful on
>> and asr-33 teletype.
>
> Now it useful too, on serial console.
> ^O processing be present years ago in FreeBSD, but removed at time of
> tty code rewrite. As I know patch for restore exist, nobody
> review/commit.
>
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r311952 - head/sys/ddb

[Slawa Olhovchenkov]

On Sun, Jan 15, 2017 at 01:07:58PM +0800, Julian Elischer wrote:

> 
> the old DEC machines had (from memory) ^O  which dropped all pending 
> output.
> Not so useful when there is 2MB of network buffers queued up to you, 
> but very useful on
> and asr-33 teletype.

Now it useful too, on serial console.
^O processing be present years ago in FreeBSD, but removed at time of
tty code rewrite. As I know patch for restore exist, nobody
review/commit.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r311952 - head/sys/ddb

[Julian Elischer]

On 15/01/2017 6:06 AM, Mark Johnston wrote:

On Fri, Jan 13, 2017 at 02:51:04PM +1100, Bruce Evans wrote:

On Thu, 12 Jan 2017, Mark Johnston wrote:


Log:
  Enable the use of ^C and ^S/^Q in DDB.

  This lets one interrupt DDB's output, which is useful if paging is
  disabled and the output device is slow.

This is quite broken.  It removes the code that was necessary for avoiding
loss of input.

Hi Bruce,

I've reverted this for now. I haven't thought much about how
db_check_interrupt() might losslessly poll the console driver for
ctrl-C, but I do think it's a valuable feature to have - just this
morning I absent-mindedly dumped a 128K-entry KTR ring from a DDB prompt
after having set $lines = 0, and had to wait for quite a while to get
my prompt back.


Modified: head/sys/ddb/db_input.c
==
--- head/sys/ddb/db_input.c Thu Jan 12 00:09:31 2017(r311951)
+++ head/sys/ddb/db_input.c Thu Jan 12 00:22:36 2017(r311952)
@@ -63,7 +63,6 @@ static intdb_lhist_nlines;
#define BLANK   ' '
#define BACKUP  '\b'

-static int cnmaygetc(void);
static void db_delete(int n, int bwd);
static int  db_inputchar(int c);
static void db_putnchars(int c, int count);
@@ -291,12 +290,6 @@ db_inputchar(c)
return (0);
}

-static int
-cnmaygetc()
-{
-   return (-1);
-}
-

BSD never had a usable console API (function) for checking for input.
cncheckc() is the correct name for such a function.  The actual
cncheckc() returns any input that it finds.  This is not the main
problem here.  The input will have to be read here anyway to determine
what it is.  The problems are that there is no console API at all for
ungetting characters in the input stream, and this function doesn't
even use a stub cnungetc(), but drops the input on the floor.  It does
document this behaviour in a comment, but doesn't say that it is wrong.


int
db_readline(lstart, lsize)
char *  lstart;
@@ -350,7 +343,7 @@ db_check_interrupt(void)
{
int c;

-   c = cnmaygetc();
+   c = cncheckc();
switch (c) {
case -1:/* no character */
return;

The stub function always returns -1, so db_check_interrupt() is turned into
a no-op.

db_check_interrupt() now eats the first byte of any input on every call.


@@ -361,7 +354,7 @@ db_check_interrupt(void)

case CTRL('s'):
do {
-   c = cnmaygetc();
+   c = cncheckc();
if (c == CTRL('c'))
db_error((char *)0);
} while (c != CTRL('q'));

This is now a bad implementation of xon/xoff.  It doesn't support ixany,
but busy-waits for ^Q or ^C using cncheckc().  It should at least
busy-wait using cngetc(), since that might be do a smarter busy wait.
cngetc() is actually only slightly smarter -- it uses busy-waiting too,
but with cpu_spinwait() in the loop.  This cpu_spinwait() makes little
difference since cncheckc() tends to be a heavyweight function.  Only
cpu_spinwait()s in the innermost loops of cncheckc(), if any, are likely
to have much effect.

Multiple consoles complicate this a bit.

db_check_interrupt() is called after every db_putc() of a newline.  So
the breakage is mainly for type-ahead while writing many lines.

I'll note that type-ahead interacts rather poorly with DDB's behaviour
of repeating the previous command upon reading an empty input line:
entering a carriage return at any point while "show ktr" is working will
cause it to be started again once it's finished.


the old DEC machines had (from memory) ^O  which dropped all pending 
output.
Not so useful when there is 2MB of network buffers queued up to you, 
but very useful on

and asr-33 teletype.




Control processing belongs in the lowest level of console drivers, and
at least the xon/xoff part is very easy to do.  This makes it work for
contexts like boot messages -- this can only be controlled now by booting
with -p, which gives something like an automatic ^S after every line,
but the control characters for this mode are unusual and there is no
way to get back to -p mode after turning it off or not starting with it.

[...]



___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r311952 - head/sys/ddb

[Mark Johnston]

On Fri, Jan 13, 2017 at 02:51:04PM +1100, Bruce Evans wrote:
> On Thu, 12 Jan 2017, Mark Johnston wrote:
> 
> > Log:
> >  Enable the use of ^C and ^S/^Q in DDB.
> >
> >  This lets one interrupt DDB's output, which is useful if paging is
> >  disabled and the output device is slow.
> 
> This is quite broken.  It removes the code that was necessary for avoiding
> loss of input.

Hi Bruce,

I've reverted this for now. I haven't thought much about how
db_check_interrupt() might losslessly poll the console driver for
ctrl-C, but I do think it's a valuable feature to have - just this
morning I absent-mindedly dumped a 128K-entry KTR ring from a DDB prompt
after having set $lines = 0, and had to wait for quite a while to get
my prompt back.

> 
> > Modified: head/sys/ddb/db_input.c
> > ==
> > --- head/sys/ddb/db_input.c Thu Jan 12 00:09:31 2017(r311951)
> > +++ head/sys/ddb/db_input.c Thu Jan 12 00:22:36 2017(r311952)
> > @@ -63,7 +63,6 @@ static intdb_lhist_nlines;
> > #define BLANK   ' '
> > #define BACKUP  '\b'
> >
> > -static int cnmaygetc(void);
> > static void db_delete(int n, int bwd);
> > static int  db_inputchar(int c);
> > static void db_putnchars(int c, int count);
> > @@ -291,12 +290,6 @@ db_inputchar(c)
> > return (0);
> > }
> >
> > -static int
> > -cnmaygetc()
> > -{
> > -   return (-1);
> > -}
> > -
> 
> BSD never had a usable console API (function) for checking for input.
> cncheckc() is the correct name for such a function.  The actual
> cncheckc() returns any input that it finds.  This is not the main
> problem here.  The input will have to be read here anyway to determine
> what it is.  The problems are that there is no console API at all for
> ungetting characters in the input stream, and this function doesn't
> even use a stub cnungetc(), but drops the input on the floor.  It does
> document this behaviour in a comment, but doesn't say that it is wrong.
> 
> > int
> > db_readline(lstart, lsize)
> > char *  lstart;
> > @@ -350,7 +343,7 @@ db_check_interrupt(void)
> > {
> > int c;
> >
> > -   c = cnmaygetc();
> > +   c = cncheckc();
> > switch (c) {
> > case -1:/* no character */
> > return;
> 
> The stub function always returns -1, so db_check_interrupt() is turned into
> a no-op.
> 
> db_check_interrupt() now eats the first byte of any input on every call.
> 
> > @@ -361,7 +354,7 @@ db_check_interrupt(void)
> >
> > case CTRL('s'):
> > do {
> > -   c = cnmaygetc();
> > +   c = cncheckc();
> > if (c == CTRL('c'))
> > db_error((char *)0);
> > } while (c != CTRL('q'));
> 
> This is now a bad implementation of xon/xoff.  It doesn't support ixany,
> but busy-waits for ^Q or ^C using cncheckc().  It should at least
> busy-wait using cngetc(), since that might be do a smarter busy wait.
> cngetc() is actually only slightly smarter -- it uses busy-waiting too,
> but with cpu_spinwait() in the loop.  This cpu_spinwait() makes little
> difference since cncheckc() tends to be a heavyweight function.  Only
> cpu_spinwait()s in the innermost loops of cncheckc(), if any, are likely
> to have much effect.
> 
> Multiple consoles complicate this a bit.
> 
> db_check_interrupt() is called after every db_putc() of a newline.  So
> the breakage is mainly for type-ahead while writing many lines.

I'll note that type-ahead interacts rather poorly with DDB's behaviour
of repeating the previous command upon reading an empty input line:
entering a carriage return at any point while "show ktr" is working will
cause it to be started again once it's finished.

> 
> Control processing belongs in the lowest level of console drivers, and
> at least the xon/xoff part is very easy to do.  This makes it work for
> contexts like boot messages -- this can only be controlled now by booting
> with -p, which gives something like an automatic ^S after every line,
> but the control characters for this mode are unusual and there is no
> way to get back to -p mode after turning it off or not starting with it.
> 
> [...]
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r311952 - head/sys/ddb

[Bruce Evans]

On Thu, 12 Jan 2017, Mark Johnston wrote:


Log:
 Enable the use of ^C and ^S/^Q in DDB.

 This lets one interrupt DDB's output, which is useful if paging is
 disabled and the output device is slow.


This is quite broken.  It removes the code that was necessary for avoiding
loss of input.


Modified: head/sys/ddb/db_input.c
==
--- head/sys/ddb/db_input.c Thu Jan 12 00:09:31 2017(r311951)
+++ head/sys/ddb/db_input.c Thu Jan 12 00:22:36 2017(r311952)
@@ -63,7 +63,6 @@ static intdb_lhist_nlines;
#define BLANK   ' '
#define BACKUP  '\b'

-static int cnmaygetc(void);
static void db_delete(int n, int bwd);
static int  db_inputchar(int c);
static void db_putnchars(int c, int count);
@@ -291,12 +290,6 @@ db_inputchar(c)
return (0);
}

-static int
-cnmaygetc()
-{
-   return (-1);
-}
-


BSD never had a usable console API (function) for checking for input.
cncheckc() is the correct name for such a function.  The actual
cncheckc() returns any input that it finds.  This is not the main
problem here.  The input will have to be read here anyway to determine
what it is.  The problems are that there is no console API at all for
ungetting characters in the input stream, and this function doesn't
even use a stub cnungetc(), but drops the input on the floor.  It does
document this behaviour in a comment, but doesn't say that it is wrong.


int
db_readline(lstart, lsize)
char *  lstart;
@@ -350,7 +343,7 @@ db_check_interrupt(void)
{
int c;

-   c = cnmaygetc();
+   c = cncheckc();
switch (c) {
case -1:/* no character */
return;


The stub function always returns -1, so db_check_interrupt() is turned into
a no-op.

db_check_interrupt() now eats the first byte of any input on every call.


@@ -361,7 +354,7 @@ db_check_interrupt(void)

case CTRL('s'):
do {
-   c = cnmaygetc();
+   c = cncheckc();
if (c == CTRL('c'))
db_error((char *)0);
} while (c != CTRL('q'));


This is now a bad implementation of xon/xoff.  It doesn't support ixany,
but busy-waits for ^Q or ^C using cncheckc().  It should at least
busy-wait using cngetc(), since that might be do a smarter busy wait.
cngetc() is actually only slightly smarter -- it uses busy-waiting too,
but with cpu_spinwait() in the loop.  This cpu_spinwait() makes little
difference since cncheckc() tends to be a heavyweight function.  Only
cpu_spinwait()s in the innermost loops of cncheckc(), if any, are likely
to have much effect.

Multiple consoles complicate this a bit.

db_check_interrupt() is called after every db_putc() of a newline.  So
the breakage is mainly for type-ahead while writing many lines.

Control processing belongs in the lowest level of console drivers, and
at least the xon/xoff part is very easy to do.  This makes it work for
contexts like boot messages -- this can only be controlled now by booting
with -p, which gives something like an automatic ^S after every line,
but the control characters for this mode are unusual and there is no
way to get back to -p mode after turning it off or not starting with it.

-p mode also crashes if you don't turn it off before the (syscons)
keyboard is attached, since attachment temporarily breaks the input needed
to control the mode.  ddb control should worry about this even more.
Multiple consoles complicate this more than a bit.

For console driver development/debugging, I needed xon-xoff type control
and more (discard) at the driver level, especially when i/o to one of
the multiple consoles is unreachable to avoid deadlock, or too active.
It was easy to write primitive xon/xoff but not to avoid deadlocks in
this.  Hackish code looks like this:
diff -u2 syscons.c~ syscons.c

X --- syscons.c~2016-09-01 19:20:38.263745000 +
X +++ syscons.c 2016-10-14 07:12:18.947461000 +
X ...
X +static void
X +ec_wait(void)

ec_wait() is called to give time to read transient output about certain
errors (mainly in near-deadlock conditions).  The error message is written
directly to the frame buffer and will be overwritten if the deadlock
condition clears enough to allow normal output.

X +{
X + int c;
X + int timeout;
X +
X + if (ec_kill_ec_wait)
X + return;
X + c = inb(0xe060);

0xe060 is an otherwise-unused serial port which I can write control characters
too.

X + if (ec_wait_verbose)
X + ec_puts("\r\nec: hit any key to abort wait of 10 seconds...");
X + for (timeout = 0; timeout < ec_wait_delay; timeout++) {
X + // if (cncheckc() != -1)
X + // break;

Early versions used simply cncheckc().  This checks all consoles, so can
deadlock too easily, and it doesn't keep the input streams separate enough
for easy control