Re: [PATCH] Cygwin: console: Share readahead buffer within the same process.
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
Re: [PATCH v2] Cygwin: pty: Revise code waiting for forwarding again.
On Jan 27 13:22, Takashi Yano wrote: > On Mon, 27 Jan 2020 11:38:22 +0900 > Takashi Yano wrote: > > On Sun, 26 Jan 2020 22:33:19 +0900 > > Takashi Yano wrote: > > > On Sat, 25 Jan 2020 20:38:37 +0900 > > > Takashi Yano wrote: > > > > On Fri, 24 Jan 2020 12:07:30 +0100 > > > > Corinna Vinschen wrote: > > > > > Too bad. It's pretty strange that CreatePseudoConsole returns a > > > > > valid HPCON but then isn't ready to take input immediately. > > > > > > > > > > > I do not come up with other implementation so far. > > > > > > > > > > > > Let me consider a while. > > > > > > > > > > I wonder how others solve this problem. I see that the native OpenSSH > > > > > is using Sleeps, too, in their start_with_pty() function, calling > > > > > AttachConsole in a loop, but I'm not sure if these are related to > > > > > pseudo > > > > > console usage. The commit message don't explain anything there :( > > > > > > > > The essence of the difficulty is that we have to support both cygwin > > > > programs and native console apps. If we consider only of native console > > > > apps, any time we can use pseudo console. However, pseudo console is > > > > not transparent at all, so it cannot be used for cygwin programs. > > > > > > > > Therefore, current cygwin is switching handles to be used between > > > > named-pipe and pseudo console. > > > > > > > > However, because pseudo console has relatively long latency, if pipe > > > > is switched just after writing to pseudo console, the forwarding > > > > does not get in time. So the "wait" is needed before switching. > > > > > > > > I had tried WriteFile(), ReadFile() and DeviceIoControl() for > > > > HANDLE hConDrvReference, however, all atempts of them failed. > > > > > > After much struggle, I finally found a solution. > > > Please look at v3 patch. > > > > v3 patch does not seem to work as expected in Win10 1809. > > I will submit v4 patch. > > Sorry for again and again. This is tricky stuff. No worries at all. > I think v5 is more fundamental fix than v4. I didn't see a v5 yet. It's still in the works, I take it? Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
[PATCH v5] Cygwin: pty: Revise code waiting for forwarding again.
- After commit 6cc299f0e20e4b76f7dbab5ea8c296ffa4859b62, outputs of
cygwin programs which call both printf() and WriteConsole() are
frequently distorted. This patch fixes the issue.
---
winsup/cygwin/fhandler.h | 1 +
winsup/cygwin/fhandler_tty.cc | 37 +--
winsup/cygwin/tty.cc | 1 +
winsup/cygwin/tty.h | 1 +
4 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 35190c0fe..9ce321c8c 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2209,6 +2209,7 @@ class fhandler_pty_slave: public fhandler_pty_common
}
void setup_locale (void);
void set_freeconsole_on_close (bool val);
+ void wait_pcon_fwd (void);
};
#define __ptsname(buf, unit) __small_sprintf ((buf), "/dev/pty%d", (unit))
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 71a1f42ba..3e5017f19 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1109,7 +1109,7 @@ skip_console_setting:
}
else if ((fd == 1 || fd == 2) && !get_ttyp ()->switch_to_pcon_out)
{
- cygwait (get_ttyp ()->fwd_done, INFINITE);
+ wait_pcon_fwd ();
if (get_ttyp ()->pcon_pid == 0 ||
kill (get_ttyp ()->pcon_pid, 0) != 0)
get_ttyp ()->pcon_pid = myself->pid;
@@ -1152,7 +1152,7 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
}
if (get_ttyp ()->switch_to_pcon_out)
/* Wait for pty_master_fwd_thread() */
-cygwait (get_ttyp ()->fwd_done, INFINITE);
+wait_pcon_fwd ();
get_ttyp ()->pcon_pid = 0;
get_ttyp ()->switch_to_pcon_in = false;
get_ttyp ()->switch_to_pcon_out = false;
@@ -2680,6 +2680,16 @@ fhandler_pty_slave::set_freeconsole_on_close (bool val)
freeconsole_on_close = val;
}
+void
+fhandler_pty_slave::wait_pcon_fwd (void)
+{
+ acquire_output_mutex (INFINITE);
+ get_ttyp ()->pcon_last_time = GetTickCount ();
+ ResetEvent (get_ttyp ()->fwd_done);
+ release_output_mutex ();
+ cygwait (get_ttyp ()->fwd_done, INFINITE);
+}
+
void
fhandler_pty_slave::fixup_after_attach (bool native_maybe, int fd_set)
{
@@ -2727,7 +2737,7 @@ fhandler_pty_slave::fixup_after_attach (bool
native_maybe, int fd_set)
DWORD mode = ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT;
SetConsoleMode (get_output_handle (), mode);
if (!get_ttyp ()->switch_to_pcon_out)
- cygwait (get_ttyp ()->fwd_done, INFINITE);
+ wait_pcon_fwd ();
if (get_ttyp ()->pcon_pid == 0 ||
kill (get_ttyp ()->pcon_pid, 0) != 0)
get_ttyp ()->pcon_pid = myself->pid;
@@ -3009,14 +3019,29 @@ fhandler_pty_master::pty_master_fwd_thread ()
termios_printf ("Started.");
for (;;)
{
- if (::bytes_available (rlen, from_slave) && rlen == 0)
- SetEvent (get_ttyp ()->fwd_done);
+ if (get_pseudo_console ())
+ {
+ /* The forwarding in pseudo console sometimes stops for
+16-32 msec even if it alerady has data to transfer.
+If the time without transfer exceeds 32 msec, the
+forwarding has supposed to be finished. */
+ const int sleep_in_pcon = 16;
+ const int time_to_wait = sleep_in_pcon * 2 + 1/* margine */;
+ get_ttyp ()->pcon_last_time = GetTickCount ();
+ while (::bytes_available (rlen, from_slave) && rlen == 0)
+ {
+ acquire_output_mutex (INFINITE);
+ if (GetTickCount () - get_ttyp ()->pcon_last_time > time_to_wait)
+ SetEvent (get_ttyp ()->fwd_done);
+ release_output_mutex ();
+ Sleep (1);
+ }
+ }
if (!ReadFile (from_slave, outbuf, sizeof outbuf, &rlen, NULL))
{
termios_printf ("ReadFile for forwarding failed, %E");
break;
}
- ResetEvent (get_ttyp ()->fwd_done);
ssize_t wlen = rlen;
char *ptr = outbuf;
if (get_pseudo_console ())
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index ef9bbc1ff..a3d4a0fc8 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -246,6 +246,7 @@ tty::init ()
term_code_page = 0;
need_redraw_screen = false;
fwd_done = NULL;
+ pcon_last_time = 0;
}
HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index b291fd3c1..755897d7d 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -107,6 +107,7 @@ private:
UINT term_code_page;
bool need_redraw_screen;
HANDLE fwd_done;
+ DWORD pcon_last_time;
public:
HANDLE from_master () const { return _from_master; }
--
2.21.0
[PATCH v2] Cygwin: console: Share readahead buffer within the same process.
- 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 | 56 +++
winsup/cygwin/fhandler.h | 33 +-
winsup/cygwin/fhandler_console.cc | 40 +-
winsup/cygwin/fhandler_termios.cc | 35 +--
winsup/cygwin/fhandler_tty.cc | 2 +-
5 files changed, 111 insertions(+), 55 deletions(-)
diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index aeee8fe4d..4210510be 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -44,11 +44,11 @@ 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;
_refcnt = 0;
}
@@ -66,15 +66,15 @@ int
fhandler_base::put_readahead (char value)
{
char *newrabuf;
- if (raixput < rabuflen)
+ if (raixput () < rabuflen ())
/* Nothing to do */;
- else if ((newrabuf = (char *) realloc (rabuf, rabuflen += 32)))
-rabuf = newrabuf;
+ else if ((newrabuf = (char *) realloc (rabuf (), rabuflen () += 32)))
+rabuf () = newrabuf;
else
return 0;
- rabuf[raixput++] = value;
- ralen++;
+ rabuf ()[raixput ()++] = value;
+ ralen ()++;
return 1;
}
@@ -82,11 +82,11 @@ int
fhandler_base::get_readahead ()
{
int chret = -1;
- if (raixget < ralen)
-chret = ((unsigned char) rabuf[raixget++]) & 0xff;
+ if (raixget () < ralen ())
+chret = ((unsigned char) rabuf ()[raixget ()++]) & 0xff;
/* FIXME - not thread safe */
- if (raixget >= ralen)
-raixget = raixput = ralen = 0;
+ if (raixget () >= ralen ())
+raixget () = raixput () = ralen () = 0;
return chret;
}
@@ -94,10 +94,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 && raixget () < ralen ())
+chret = ((unsigned char) rabuf ()[raixget ()]) & 0xff;
+ else if (queryput && raixput () > 0)
+chret = ((unsigned char) rabuf ()[raixput () - 1]) & 0xff;
return chret;
}
@@ -105,7 +105,7 @@ void
fhandler_base::set_readahead_valid (int val, int ch)
{
if (!val)
-ralen = raixget = raixput = 0;
+ralen () = raixget () = raixput () = 0;
if (ch != -1)
put_readahead (ch);
}
@@ -1092,7 +1092,7 @@ fhandler_base::lseek (off_t offset, int whence)
if (whence != SEEK_CUR || offset != 0)
{
if (whence == SEEK_CUR)
- offset -= ralen - raixget;
+ offset -= ralen () - raixget ();
set_readahead_valid (0);
}
@@ -1142,7 +1142,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 -= ralen () - raixget ();
return res;
}
@@ -1565,23 +1565,23 @@ 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;
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..b4a5638fe 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -202,16 +202,21 @@ 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;
/* Used for advisory file locking. See flock.cc. */
int64_t unique_id;
@@ -315,7 +320,13 @@ class fhandler_base
ReleaseSemaphore (read_state, n, NULL);
}
- bool get_readahead_valid () { return raixget < ralen; }
+ virtual char *&rabuf () { return ra.rabuf; };
+ vir
[PATCH 0/3] Some O_PATH fixes
Ken Brown (3): Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag Cygwin: fhandler_disk_file::fstatvfs: refactor Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set winsup/cygwin/fhandler.h| 1 + winsup/cygwin/fhandler_disk_file.cc | 24 +--- winsup/cygwin/fhandler_fifo.cc | 8 3 files changed, 26 insertions(+), 7 deletions(-) -- 2.21.0
[PATCH 3/3] Cygwin: FIFO::fstatvfs: use our handle if O_PATH is set
If O_PATH is set, then the fhandler_fifo object has a handle that can
be used for getting the statvfs information. Use it by calling
fhandler_base::fstatvfs_by_handle. Before this change,
fhandler_disk_file::fstatfvs was called on a new fhandler_disk object,
which would then have to be opened.
---
winsup/cygwin/fhandler_fifo.cc | 8
1 file changed, 8 insertions(+)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index a338f12cc..ef568f6fe 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -906,6 +906,14 @@ errout:
int __reg2
fhandler_fifo::fstatvfs (struct statvfs *sfs)
{
+ if (get_flags () & O_PATH)
+/* We already have a handle. */
+{
+ HANDLE h = get_handle ();
+ if (h)
+ return fstatvfs_by_handle (h, sfs);
+}
+
fhandler_disk_file fh (pc);
fh.get_device () = FH_FS;
return fh.fstatvfs (sfs);
--
2.21.0
[PATCH 1/3] Cygwin: fhandler_base::fstat_fs: accomodate the O_PATH flag
Treat a special file opened with O_PATH the same as a regular file,
i.e., use its handle to get the stat information.
Before this change, fstat_fs opened the file a second time, with the
wrong flags and without closing the existing handle. A side effect
was to change the openflags of the file, possibly causing further
system calls to fail.
Currently this change only affects FIFOs, but it will affect
AF_LOCAL/AF_UNIX sockets too once they support O_PATH.
---
winsup/cygwin/fhandler_disk_file.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/winsup/cygwin/fhandler_disk_file.cc
b/winsup/cygwin/fhandler_disk_file.cc
index 32381a0b0..a1ab2bbdd 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -359,7 +359,7 @@ fhandler_base::fstat_fs (struct stat *buf)
if (get_stat_handle ())
{
- if (!nohandle () && !is_fs_special ())
+ if (!nohandle () && (!is_fs_special () || get_flags () & O_PATH))
res = pc.fs_is_nfs () ? fstat_by_nfs_ea (buf) : fstat_by_handle (buf);
if (res)
res = fstat_by_name (buf);
--
2.21.0
[PATCH 2/3] Cygwin: fhandler_disk_file::fstatvfs: refactor
Define a new method fhandler_base::fstatvfs_by_handle, extracted from
fhandler_disk_file::fstatvfs, which gets the statvfs information when
a handle is available.
This will be used in future commits for special files that have been
opened with O_PATH.
---
winsup/cygwin/fhandler.h| 1 +
winsup/cygwin/fhandler_disk_file.cc | 22 --
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index c0d56b4da..5fa720a83 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -381,6 +381,7 @@ private:
int __reg2 fstat_by_name (struct stat *buf);
public:
virtual int __reg2 fstatvfs (struct statvfs *buf);
+ int __reg2 fstatvfs_by_handle (HANDLE h, struct statvfs *buf);
int __reg2 utimens_fs (const struct timespec *);
virtual int __reg1 fchmod (mode_t mode);
virtual int __reg2 fchown (uid_t uid, gid_t gid);
diff --git a/winsup/cygwin/fhandler_disk_file.cc
b/winsup/cygwin/fhandler_disk_file.cc
index a1ab2bbdd..89e651029 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -600,9 +600,7 @@ int __reg2
fhandler_disk_file::fstatvfs (struct statvfs *sfs)
{
int ret = -1, opened = 0;
- NTSTATUS status;
IO_STATUS_BLOCK io;
- FILE_FS_FULL_SIZE_INFORMATION full_fsi;
/* We must not use the stat handle here, even if it exists. The handle
has been opened with FILE_OPEN_REPARSE_POINT, thus, in case of a volume
mount point, it points to the FS of the mount point, rather than to the
@@ -630,6 +628,22 @@ fhandler_disk_file::fstatvfs (struct statvfs *sfs)
}
}
+ ret = fstatvfs_by_handle (fh, sfs);
+out:
+ if (opened)
+NtClose (fh);
+ syscall_printf ("%d = fstatvfs(%s, %p)", ret, get_name (), sfs);
+ return ret;
+}
+
+int __reg2
+fhandler_base::fstatvfs_by_handle (HANDLE fh, struct statvfs *sfs)
+{
+ int ret = -1;
+ NTSTATUS status;
+ IO_STATUS_BLOCK io;
+ FILE_FS_FULL_SIZE_INFORMATION full_fsi;
+
sfs->f_files = ULONG_MAX;
sfs->f_ffree = ULONG_MAX;
sfs->f_favail = ULONG_MAX;
@@ -688,10 +702,6 @@ fhandler_disk_file::fstatvfs (struct statvfs *sfs)
debug_printf ("%y = NtQueryVolumeInformationFile"
"(%S, FileFsFullSizeInformation)",
status, pc.get_nt_native_path ());
-out:
- if (opened)
-NtClose (fh);
- syscall_printf ("%d = fstatvfs(%s, %p)", ret, get_name (), sfs);
return ret;
}
--
2.21.0
