1599 backspace should perform delete on console

Review Request #214 — Created Sept. 12, 2016 and submitted

tsoome
illumos-gate
1599
1583d14...
general

1599 backspace should perform delete on console

Tested the console and non-console with vim and few other apps.

  • 0
  • 0
  • 16
  • 0
  • 16
Description From Last Updated
tsoome
tsoome
tsoome
tsoome
tsoome
trisk
  1. Ship It!
  2. 
      
tsoome
tsoome
tsoome
jclulow
  1. Hi Toomas,

    I've taken a look and I have some feedback below. (NB: I also looked at the rendered diffs in the webrev, and in particular the table in termios.h(3HEAD) looks much better.)

    Cheers.

  2. usr/src/cmd/ttymon/stty.c (Diff revision 7)
     
     

    I think that this code should actually be down on L354, i.e. after the status version of this line, and guarded by the TERMIOS flag.

    The TERMIOS flag appears to be set in get_ttymode() iff we were able to get a complete struct termios back from the kernel via TCGETS. If that fails, we mock one up with an older ioctl request which seems to only set the first NCC entries in c_cc. Because erase2 is going on the end of this extended object, I think it needs to behind the TERMIOS flag as well.

    1. hm, for visual reasons I would keep erase and erase2 close - so when I am looking for erase, the eye will catch the erase2 as well, but indeed, it needs TRMIOS check there.

  3. usr/src/cmd/ttymon/stty.c (Diff revision 7)
     
     

    Should be behind the TERMIOS check a few lines down, after the "status" line.

    1. same as previous case, added the TERMIOS, but left close to erase.

  4. usr/src/cmd/ttymon/sttyparse.c (Diff revision 7)
     
     

    It looks like this ought to be an else if, like the other chained branches underneath?

    Also, I think this should be in the TERMIOS block after the "status" version of this check, around ~L135.

    1. yep, also added TERMIOS check for stty ek and stty sane.

  5. usr/src/man/man1/stty.1 (Diff revision 7)
     
     

    There ought to be a comma after "ERASE2".

  6. usr/src/man/man7i/termio.7i (Diff revision 7)
     
     

    Comma after "erase2" here.

  7. usr/src/man/man7i/termio.7i (Diff revision 7)
     
     

    I think this would be better as:

    None of ERASE, ERASE2, or WERASE will erase beyond the beginning of the line.
    
  8. usr/src/man/man7i/termio.7i (Diff revision 7)
     
     

    Comma after ERASE2.

  9. usr/src/man/man7i/termio.7i (Diff revision 7)
     
     

    Comma after erase2.

  10. usr/src/man/man7i/termio.7i (Diff revision 7)
     
     

    "another additional" is redundant; I would probably go for something like:

    (Control-h or ASCII BS) erases the preceding character, with behaviour identical to that of ERASE.
    
  11. usr/src/man/man7i/termio.7i (Diff revision 7)
     
     

    Comma after ERASE2.

  12. usr/src/man/man7i/termio.7i (Diff revision 7)
     
     

    Comma after ERASE2.

  13. usr/src/uts/common/io/tty_common.c (Diff revision 7)
     
     

    I think this comment is a bit out of sync with the value already -- I think it should be 18 control characters now?

  14. usr/src/uts/common/sys/termios.h (Diff revision 7)
     
     

    I think this #if check is redundant, right? You're already inside a check for the same condition that starts on L140 and extends down to L186.

    1. yep, you are correct indeed.

  15. usr/src/uts/common/sys/termios.h (Diff revision 7)
     
     

    This #if seems redundant as well.

  16. usr/src/uts/sun4u/blade/io/options.conf (Diff revision 7)
     
     

    I think that when SIGINFO/STATUS support was added, the SPARC options.conf files might not have been updated. It looks like you might need to add 14 to the array for STATUS before you append the new value for ERASE2?

  17. usr/src/uts/sun4u/opl/io/options.conf (Diff revision 7)
     
     

    See comment in options.conf for sun4u/blade.

  18. 
      
tsoome
jclulow
  1. Thanks for the changes! Looks good to me.

  2. 
      
tsoome
xenol
  1. Ship It!
  2. 
      
tsoome
jclulow
  1. Ship It!
  2. 
      
trisk
  1. Checked the design discussion thread and this looks great.

  2. 
      
trisk
  1. Ship It!
  2. 
      
tsoome
tsoome
Review request changed

Status: Closed (submitted)

Loading...