Re: [PATCH] Cygwin: console: Share readahead buffer within the same process.

[Corinna Vinschen]

Hi Takashi,

On Jan 25 18:45, Takashi Yano wrote:
> - The cause of the problem reported in
>   https://www.cygwin.com/ml/cygwin/2020-01/msg00220.html is that the
>   chars input before dup() cannot be read from the new file descriptor.
>   This is because the readahead buffer (rabuf) in the console is newly
>   created by dup(), and does not inherit from the parent. This patch
>   fixes the issue.
> ---
>  winsup/cygwin/fhandler.cc | 58 ---
>  winsup/cygwin/fhandler.h  | 24 -
>  winsup/cygwin/fhandler_console.cc | 16 -
>  winsup/cygwin/fhandler_termios.cc | 35 ++-
>  winsup/cygwin/fhandler_tty.cc |  2 +-
>  5 files changed, 80 insertions(+), 55 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
> index aeee8fe4d..ad4a7e61c 100644
> --- a/winsup/cygwin/fhandler.cc
> +++ b/winsup/cygwin/fhandler.cc
> @@ -44,11 +44,12 @@ void
>  fhandler_base::reset (const fhandler_base *from)
>  {
>pc << from->pc;
> -  rabuf = NULL;
> -  ralen = 0;
> -  raixget = 0;
> -  raixput = 0;
> -  rabuflen = 0;
> +  ra.rabuf = NULL;
> +  ra.ralen = 0;
> +  ra.raixget = 0;
> +  ra.raixput = 0;
> +  ra.rabuflen = 0;
> +  set_rabuf ();
>_refcnt = 0;
>  }
>  
> @@ -66,15 +67,15 @@ int
>  fhandler_base::put_readahead (char value)
>  {
>char *newrabuf;
> -  if (raixput < rabuflen)
> +  if (raptr->raixput < raptr->rabuflen)
>  /* Nothing to do */;

This adds extra pointer access to critical code paths, even if only
in O_TEXT scenarios.  May I suggest dropping the extra pointer and
converting readahead access to access methods, kind of like this:

  class fhandler {
char *&rabuf () { return ra.rabuf; }
int &rabuflen () { return ra.rabuflen; }
[...]

  class fhandler_console {
char *&rabuf () { return con_ra.rabuf; }
int &rabuflen () { return con_ra.rabuflen; }
[...]

and then use those accessor methods throughout:

> -  else if ((newrabuf = (char *) realloc (rabuf, rabuflen += 32)))
> -rabuf = newrabuf;

  else if ((newrabuf = (char *) realloc (rabuf (), rabuflen () += 32)))
rabuf () = newrabuf;

etc.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer


signature.asc
Description: PGP signature

[PATCH] Cygwin: console: Share readahead buffer within the same process.

[Takashi Yano]

- The cause of the problem reported in
  https://www.cygwin.com/ml/cygwin/2020-01/msg00220.html is that the
  chars input before dup() cannot be read from the new file descriptor.
  This is because the readahead buffer (rabuf) in the console is newly
  created by dup(), and does not inherit from the parent. This patch
  fixes the issue.
---
 winsup/cygwin/fhandler.cc | 58 ---
 winsup/cygwin/fhandler.h  | 24 -
 winsup/cygwin/fhandler_console.cc | 16 -
 winsup/cygwin/fhandler_termios.cc | 35 ++-
 winsup/cygwin/fhandler_tty.cc |  2 +-
 5 files changed, 80 insertions(+), 55 deletions(-)

diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index aeee8fe4d..ad4a7e61c 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -44,11 +44,12 @@ void
 fhandler_base::reset (const fhandler_base *from)
 {
   pc << from->pc;
-  rabuf = NULL;
-  ralen = 0;
-  raixget = 0;
-  raixput = 0;
-  rabuflen = 0;
+  ra.rabuf = NULL;
+  ra.ralen = 0;
+  ra.raixget = 0;
+  ra.raixput = 0;
+  ra.rabuflen = 0;
+  set_rabuf ();
   _refcnt = 0;
 }
 
@@ -66,15 +67,15 @@ int
 fhandler_base::put_readahead (char value)
 {
   char *newrabuf;
-  if (raixput < rabuflen)
+  if (raptr->raixput < raptr->rabuflen)
 /* Nothing to do */;
-  else if ((newrabuf = (char *) realloc (rabuf, rabuflen += 32)))
-rabuf = newrabuf;
+  else if ((newrabuf = (char *) realloc (raptr->rabuf, raptr->rabuflen += 32)))
+raptr->rabuf = newrabuf;
   else
 return 0;
 
-  rabuf[raixput++] = value;
-  ralen++;
+  raptr->rabuf[raptr->raixput++] = value;
+  raptr->ralen++;
   return 1;
 }
 
@@ -82,11 +83,11 @@ int
 fhandler_base::get_readahead ()
 {
   int chret = -1;
-  if (raixget < ralen)
-chret = ((unsigned char) rabuf[raixget++]) & 0xff;
+  if (raptr->raixget < raptr->ralen)
+chret = ((unsigned char) raptr->rabuf[raptr->raixget++]) & 0xff;
   /* FIXME - not thread safe */
-  if (raixget >= ralen)
-raixget = raixput = ralen = 0;
+  if (raptr->raixget >= raptr->ralen)
+raptr->raixget = raptr->raixput = raptr->ralen = 0;
   return chret;
 }
 
@@ -94,10 +95,10 @@ int
 fhandler_base::peek_readahead (int queryput)
 {
   int chret = -1;
-  if (!queryput && raixget < ralen)
-chret = ((unsigned char) rabuf[raixget]) & 0xff;
-  else if (queryput && raixput > 0)
-chret = ((unsigned char) rabuf[raixput - 1]) & 0xff;
+  if (!queryput && raptr->raixget < raptr->ralen)
+chret = ((unsigned char) raptr->rabuf[raptr->raixget]) & 0xff;
+  else if (queryput && raptr->raixput > 0)
+chret = ((unsigned char) raptr->rabuf[raptr->raixput - 1]) & 0xff;
   return chret;
 }
 
@@ -105,7 +106,7 @@ void
 fhandler_base::set_readahead_valid (int val, int ch)
 {
   if (!val)
-ralen = raixget = raixput = 0;
+raptr->ralen = raptr->raixget = raptr->raixput = 0;
   if (ch != -1)
 put_readahead (ch);
 }
@@ -1092,7 +1093,7 @@ fhandler_base::lseek (off_t offset, int whence)
   if (whence != SEEK_CUR || offset != 0)
 {
   if (whence == SEEK_CUR)
-   offset -= ralen - raixget;
+   offset -= raptr->ralen - raptr->raixget;
   set_readahead_valid (0);
 }
 
@@ -1142,7 +1143,7 @@ fhandler_base::lseek (off_t offset, int whence)
  readahead that we have to take into account when calculating
  the actual position for the application.  */
   if (whence == SEEK_CUR)
-res -= ralen - raixget;
+res -= raptr->ralen - raptr->raixget;
 
   return res;
 }
@@ -1565,23 +1566,24 @@ fhandler_base::fhandler_base () :
   ino (0),
   _refcnt (0),
   openflags (0),
-  rabuf (NULL),
-  ralen (0),
-  raixget (0),
-  raixput (0),
-  rabuflen (0),
   unique_id (0),
   archetype (NULL),
   usecount (0)
 {
+  ra.rabuf = NULL;
+  ra.ralen = 0;
+  ra.raixget = 0;
+  ra.raixput = 0;
+  ra.rabuflen = 0;
+  set_rabuf ();
   isclosed (false);
 }
 
 /* Normal I/O destructor */
 fhandler_base::~fhandler_base ()
 {
-  if (rabuf)
-free (rabuf);
+  if (ra.rabuf)
+free (ra.rabuf);
 }
 
 /**/
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index c0d56b4da..35190c0fe 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -202,16 +202,22 @@ class fhandler_base
 
   ino_t ino;   /* file ID or hashed filename, depends on FS. */
   LONG _refcnt;
+ public:
+  struct rabuf_t
+  {
+char *rabuf;   /* used for crlf conversion in text files */
+size_t ralen;
+size_t raixget;
+size_t raixput;
+size_t rabuflen;
+  };
 
  protected:
   /* File open flags from open () and fcntl () calls */
   int openflags;
 
-  char *rabuf; /* used for crlf conversion in text files */
-  size_t ralen;
-  size_t raixget;
-  size_t raixput;
-  size_t rabuflen;
+  struct rabuf_t ra;
+  struct rabuf_t *raptr;
 
   /* Used for advisory file locking.  See flock.cc.  */
   int64_t unique_id;
@@ -315,7 +321,8 @@ class fhandler_base