Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On Mar 1 15:38, Takashi Yano wrote: > Hi Hans, > > On Sat, 29 Feb 2020 19:10:02 +0100 > Hans-Bernhard Bröker wrote: > > One more important note: the current implementation has a potential > > buffer overrun issue, because it writes first, and only then checks > > whether that may have overrun the buffer. And the check itself is off > > by one, too: > > > > >wpbuf[wpixput++] = x; \ > > >if (wpixput > WPBUF_LEN) \ > > > wpixput--; \ > > > > That's why my latest code snippet does it differently: > > > > > if (ixput < WPBUF_LEN) > > >{ > > > buf[ixput++] = x; > > >} > > Indeed. You are right. Thanks for pointing out that. > Another similar problem exists in console code of escape > sequence handling, so I will submit a patch for that. > > As for wpbuf, please continue to fix. Yeah, a patch in `git format-patch' format would be most welcome. For a first-time contribution (and then never again) we also need a 2-clause BSD license waiver per https://cygwin.com/contrib.html Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH 0/4] Cygwin: console: Some fixes for console in xterm mode.
On Mar 2 10:12, Takashi Yano wrote: > Takashi Yano (4): > Cygwin: console: Revise the code to fix tab position. > Cygwin: console: Fix setting/unsetting xterm mode for input. > Cygwin: console: Prevent buffer overrun. > Cygwin: console: Add a workaround for "ESC 7" and "ESC 8". > > winsup/cygwin/fhandler.h | 1 + > winsup/cygwin/fhandler_console.cc | 91 ++- > 2 files changed, 55 insertions(+), 37 deletions(-) > > -- > 2.21.0 Pushed. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On Mar 2 18:03, Corinna Vinschen wrote: > On Mar 1 15:38, Takashi Yano wrote: > > Hi Hans, > > > > On Sat, 29 Feb 2020 19:10:02 +0100 > > Hans-Bernhard Bröker wrote: > > > One more important note: the current implementation has a potential > > > buffer overrun issue, because it writes first, and only then checks > > > whether that may have overrun the buffer. And the check itself is off > > > by one, too: > > > > > > >wpbuf[wpixput++] = x; \ > > > >if (wpixput > WPBUF_LEN) \ > > > > wpixput--; \ > > > > > > That's why my latest code snippet does it differently: > > > > > > > if (ixput < WPBUF_LEN) > > > >{ > > > > buf[ixput++] = x; > > > >} > > > > Indeed. You are right. Thanks for pointing out that. > > Another similar problem exists in console code of escape > > sequence handling, so I will submit a patch for that. > > > > As for wpbuf, please continue to fix. > > Yeah, a patch in `git format-patch' format would be most welcome. > > For a first-time contribution (and then never again) we also need > a 2-clause BSD license waiver per https://cygwin.com/contrib.html I pushed wpbuf_put as a simple inline function as a stop-gap measure so Cygwin builds on gcc 9.2.0. Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH v2 1/4] Cygwin: console: Add workaround for broken IL/DL in xterm mode.
On 02/03/2020 19:54, Corinna Vinschen wrote: On Mar 2 18:03, Corinna Vinschen wrote: On Mar 1 15:38, Takashi Yano wrote: Hi Hans, [...] I pushed wpbuf_put as a simple inline function as a stop-gap measure so Cygwin builds on gcc 9.2.0. \o/ https://ci.appveyor.com/project/cygwin/cygwin/builds/31190427
/proc/partitions: add some space to avoid ragged output format
From 2e33b27e7d4683062f21a3082ec634a440ff9387 Mon Sep 17 00:00:00 2001 From: Thomas Wolff Date: Mon, 2 Mar 2020 22:36:56 +0100 Subject: [PATCH] avoid ragged output of /proc/partitions --- winsup/cygwin/fhandler_proc.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/winsup/cygwin/fhandler_proc.cc b/winsup/cygwin/fhandler_proc.cc index 605a8443f..3373f3ef5 100644 --- a/winsup/cygwin/fhandler_proc.cc +++ b/winsup/cygwin/fhandler_proc.cc @@ -1491,7 +1491,7 @@ format_proc_partitions (void *, char *&destbuf) } if (!got_one) { - print ("major minor #blocks name win-mounts\n\n"); + print ("major minor #blocks name win-mounts\n\n"); got_one = true; } /* Fetch partition info for the entire disk to get its size. */ @@ -1514,7 +1514,7 @@ format_proc_partitions (void *, char *&destbuf) size = 0; } device dev (drive_num, 0); - bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s\n", + bufptr += __small_sprintf (bufptr, "%5d %5d %11U %s\n", dev.get_major (), dev.get_minor (), size >> 10, dev.name () + 5); /* Fetch drive layout info to get size of all partitions on the disk. */ @@ -1561,7 +1561,7 @@ format_proc_partitions (void *, char *&destbuf) continue; device dev (drive_num, part_num); - bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s", + bufptr += __small_sprintf (bufptr, "%5d %5d %11U %s", dev.get_major (), dev.get_minor (), size >> 10, dev.name () + 5); /* Check if the partition is mounted in Windows and, if so, -- 2.21.0
Copyright License waiver
To whom it may concern, I hereby grant anyone the rights to any use code I send here under the standard 2-clause BSD license statement as found, among other places, in winsup/cygwin/CONTRIBUTORS. Hans-Bernhard Bröker
[PATCH 0/1] Handle wpbuf in a more C++ style
Replace a relatively C-styled co-dependent pair of variables, a #define, and an inline function by a helper class containing their relation, because that is more in the C++ style. Hans-Bernhard Broeker (1): Collect handling of wpixput and wpbuf into a helper class. winsup/cygwin/fhandler_console.cc | 135 -- 1 file changed, 70 insertions(+), 65 deletions(-) -- 2.21.0
[PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.
Replace direct access to a pair of co-dependent variables by calls to methods of a class that encapsulates their relation. Also replace C #define by C++ class constant. --- winsup/cygwin/fhandler_console.cc | 135 -- 1 file changed, 70 insertions(+), 65 deletions(-) diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc index c5f269168..af2fb11a4 100644 --- a/winsup/cygwin/fhandler_console.cc +++ b/winsup/cygwin/fhandler_console.cc @@ -59,17 +59,22 @@ static struct fhandler_base::rabuf_t con_ra; /* Write pending buffer for ESC sequence handling in xterm compatible mode */ -#define WPBUF_LEN 256 -static unsigned char wpbuf[WPBUF_LEN]; -static int wpixput; static unsigned char last_char; -static inline void -wpbuf_put (unsigned char x) +// simple helper class to accumulate output in a buffer +// and send that to the console on request: +static class WritePendingBuf { - if (wpixput < WPBUF_LEN) -wpbuf[wpixput++] = x; -} +private: + static const size_t WPBUF_LEN = 256u; + unsigned char buf[WPBUF_LEN]; + size_t ixput; + +public: + inline void put(unsigned char x) { if (ixput < WPBUF_LEN) { buf[ixput++] = x; } }; + inline void empty() { ixput = 0u; }; + inline void sendOut(HANDLE &handle, DWORD *wn) { WriteConsoleA (handle, buf, ixput, wn, 0); }; +} wpbuf; static void beep () @@ -2030,10 +2035,10 @@ fhandler_console::char_command (char c) break; #endif case 'b': /* REP */ - wpbuf_put (c); + wpbuf.put (c); if (wincap.has_con_esc_rep ()) /* Just send the sequence */ - WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0); + wpbuf.sendOut( get_output_handle (), &wn); else if (last_char && last_char != '\n') for (int i = 0; i < con.args[0]; i++) WriteConsoleA (get_output_handle (), &last_char, 1, &wn, 0); @@ -2041,9 +2046,9 @@ fhandler_console::char_command (char c) case 'r': /* DECSTBM */ con.scroll_region.Top = con.args[0] ? con.args[0] - 1 : 0; con.scroll_region.Bottom = con.args[1] ? con.args[1] - 1 : -1; - wpbuf_put (c); + wpbuf.put (c); /* Just send the sequence */ - WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0); + wpbuf.sendOut( get_output_handle (), &wn); break; case 'L': /* IL */ if (wincap.has_con_broken_il_dl ()) @@ -2067,8 +2072,8 @@ fhandler_console::char_command (char c) y + 1 - con.b.srWindow.Top, srBottom + 1 - con.b.srWindow.Top); WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0); - wpbuf_put ('T'); - WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0); + wpbuf.put ('T'); + wpbuf.sendOut( get_output_handle (), &wn); __small_sprintf (buf, "\033[%d;%dr", srTop + 1 - con.b.srWindow.Top, srBottom + 1 - con.b.srWindow.Top); @@ -2079,9 +2084,9 @@ fhandler_console::char_command (char c) } else { - wpbuf_put (c); + wpbuf.put (c); /* Just send the sequence */ - WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0); + wpbuf.sendOut( get_output_handle (), &wn); } break; case 'M': /* DL */ @@ -2095,8 +2100,8 @@ fhandler_console::char_command (char c) y + 1 - con.b.srWindow.Top, srBottom + 1 - con.b.srWindow.Top); WriteConsoleA (get_output_handle (), buf, strlen (buf), &wn, 0); - wpbuf_put ('S'); - WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0); + wpbuf.put ('S'); + wpbuf.sendOut( get_output_handle (), &wn); __small_sprintf (buf, "\033[%d;%dr", srTop + 1 - con.b.srWindow.Top, srBottom + 1 - con.b.srWindow.Top); @@ -2107,13 +2112,13 @@ fhandler_console::char_command (char c) } else { - wpbuf_put (c); + wpbuf.put (c); /* Just send the sequence */ - WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0); + wpbuf.sendOut( get_output_handle (), &wn); } break; case 'J': /* ED */ - wpbuf_put (c); + wpbuf.put (c); if (con.args[0] == 3 && wincap.has_con_broken_csi3j ()) { /* Workaround for broken CSI3J in Win10 1809 */ CONSOLE_SCREEN_BUFFER_INFO sbi; @@ -2131,7 +2136,7 @@ fhandler_console::char_command (char c) } else /* Just send the sequence */ - WriteConsoleA (get_output_handle (), wpbuf, wpixput, &wn, 0); + w
Re: /proc/partitions: add some space to avoid ragged output format
Hi Thomas, In this industry, you plan ahead a bit further and longer: you need to go for at least a three digit increase (below) or more; legacy 8TB HDDs are cheap and common, 20TB are available, 100TB SSDs are available now (#blocks 9765625 - 11 digits), capacity is expanding *faster* than HDDs: https://www.tomshardware.com/news/100tb-ssd-nimbus-sata-flash,36687.html and speeds now exceed 6GB/s and 1M IO/s. We're also at 64C/128T 6GHz (overclocked) chips with 256MB L3 and L4 caches, 256GB memory, and over the next decade, feature sizes dropping by an order of magnitude from 14nm to 1.4nm, with corresponding increases, so maintainers should consider capacity increases when they look at code. To make this easier next time ;^> I'd define a couple of formats: #define PROC_PARTITION_HDR "%5s %5s %12s %s\n\n" #define PROC_PARTITION_FMT "%5d %5d %12U %s\n" or simplify the code further with: #define PROC_PARTITION_HDR "%5s %5s %12s %-6s %-s\n\n" #define PROC_PARTITION_FMT "%5d %5d %12U %-6s %-s\n" and sprintf the header into the buffer: - print ("major minor #blocks name win-mounts\n\n"); + bufptr += __small_sprintf (bufptr, PROC_PARTITION_HDR, + "major", "minor", "#blocks", + "name win-mounts\n\n"); *or* - print ("major minor #blocks name win-mounts\n\n"); + bufptr += __small_sprintf (bufptr, PROC_PARTITION_HDR, + "major", "minor", "#blocks", + "name", "win-mounts\n\n"); On 2020-03-02 14:39, Thomas Wolff wrote: --- winsup/cygwin/fhandler_proc.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/winsup/cygwin/fhandler_proc.cc b/winsup/cygwin/fhandler_proc.cc index 605a8443f..3373f3ef5 100644 --- a/winsup/cygwin/fhandler_proc.cc +++ b/winsup/cygwin/fhandler_proc.cc @@ -1491,7 +1491,7 @@ format_proc_partitions (void *, char *&destbuf) } if (!got_one) { - print ("major minor #blocks name win-mounts\n\n"); + print ("major minor#blocks name win-mounts\n\n"); got_one = true; } /* Fetch partition info for the entire disk to get its size. */ @@ -1514,7 +1514,7 @@ format_proc_partitions (void *, char *&destbuf) size = 0; } device dev (drive_num, 0); - bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s\n", + bufptr += __small_sprintf (bufptr, "%5d %5d %12U %s\n", dev.get_major (), dev.get_minor (), size >> 10, dev.name () + 5); /* Fetch drive layout info to get size of all partitions on the disk. */ @@ -1561,7 +1561,7 @@ format_proc_partitions (void *, char *&destbuf) continue; device dev (drive_num, part_num); - bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s", + bufptr += __small_sprintf (bufptr, "%5d %5d %12U %s", dev.get_major (), dev.get_minor (), size >> 10, dev.name () + 5); /* Check if the partition is mounted in Windows and, if so, -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised.
Re: [PATCH 1/1] Collect handling of wpixput and wpbuf into a helper class.
Hi Hans, Thanks for the patch. On Tue, 3 Mar 2020 00:07:25 +0100 Hans-Bernhard Bröker wrote: > + inline void sendOut(HANDLE &handle, DWORD *wn) { WriteConsoleA > (handle, buf, ixput, wn, 0); }; The second argument DWORD *wn of sendOut() is not used outside sendOut(), so it can be covered up like: inline void sendOut (HANDLE &handle) { DWORD wn; WriteConsoleA (handle, buf, ixput, &wn, 0); } -- Takashi Yano
Re: /proc/partitions: add some space to avoid ragged output format
Am 03.03.2020 um 00:26 schrieb Brian Inglis: Hi Thomas, In this industry, you plan ahead a bit further and longer: you need to go for at least a three digit increase (below) or more; legacy 8TB HDDs are cheap and common, 20TB are available, 100TB SSDs are available now (#blocks 9765625 - 11 digits), capacity is expanding *faster* than HDDs: https://www.tomshardware.com/news/100tb-ssd-nimbus-sata-flash,36687.html and speeds now exceed 6GB/s and 1M IO/s. We're also at 64C/128T 6GHz (overclocked) chips with 256MB L3 and L4 caches, 256GB memory, and over the next decade, feature sizes dropping by an order of magnitude from 14nm to 1.4nm, with corresponding increases, so maintainers should consider capacity increases when they look at code. To make this easier next time ;^> I'd define a couple of formats: #define PROC_PARTITION_HDR "%5s %5s %12s %s\n\n" #define PROC_PARTITION_FMT "%5d %5d %12U %s\n" or simplify the code further with: #define PROC_PARTITION_HDR "%5s %5s %12s %-6s %-s\n\n" #define PROC_PARTITION_FMT "%5d %5d %12U %-6s %-s\n" and sprintf the header into the buffer: - print ("major minor #blocks name win-mounts\n\n"); + bufptr += __small_sprintf (bufptr, PROC_PARTITION_HDR, + "major", "minor", "#blocks", + "name win-mounts\n\n"); *or* - print ("major minor #blocks name win-mounts\n\n"); + bufptr += __small_sprintf (bufptr, PROC_PARTITION_HDR, + "major", "minor", "#blocks", + "name", "win-mounts\n\n"); Hi Brian, yes, I thought about factoring out the format as well, but then only submitted a quick-and-dirty patch. If you're suggesting a more solid solution, would you submit your patch? Thomas On 2020-03-02 14:39, Thomas Wolff wrote: --- winsup/cygwin/fhandler_proc.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/winsup/cygwin/fhandler_proc.cc b/winsup/cygwin/fhandler_proc.cc index 605a8443f..3373f3ef5 100644 --- a/winsup/cygwin/fhandler_proc.cc +++ b/winsup/cygwin/fhandler_proc.cc @@ -1491,7 +1491,7 @@ format_proc_partitions (void *, char *&destbuf) } if (!got_one) { - print ("major minor #blocks name win-mounts\n\n"); + print ("major minor#blocks name win-mounts\n\n"); got_one = true; } /* Fetch partition info for the entire disk to get its size. */ @@ -1514,7 +1514,7 @@ format_proc_partitions (void *, char *&destbuf) size = 0; } device dev (drive_num, 0); - bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s\n", + bufptr += __small_sprintf (bufptr, "%5d %5d %12U %s\n", dev.get_major (), dev.get_minor (), size >> 10, dev.name () + 5); /* Fetch drive layout info to get size of all partitions on the disk. */ @@ -1561,7 +1561,7 @@ format_proc_partitions (void *, char *&destbuf) continue; device dev (drive_num, part_num); - bufptr += __small_sprintf (bufptr, "%5d %5d %9U %s", + bufptr += __small_sprintf (bufptr, "%5d %5d %12U %s", dev.get_major (), dev.get_minor (), size >> 10, dev.name () + 5); /* Check if the partition is mounted in Windows and, if so,