FAQ Proposed Updates Summary and Preview Diff
Just want to get feedback on how these FAQ changes should be packaged as patches
(separate, series, single) and whether some of the changes should not be applied
at all.
Summary
General:
change setup references to use the Cygwin Setup program;
change Win32 references to Windows;
reword net release or distribution references;
emphasize 64-bit Cygwin and setup-x86_64 over 32-bit;
change see to place links around available wording;
change for or where appropriate;
change bash .{ext1,ext2} usage to .ext1/.ext2;
trim trailing spaces highlighted by git diff.
Files:
winsup/doc/faq-api.xml: add to timezone FAQ "Why isn't time zone set correctly?"
winsup/doc/faq-programming.xml
winsup/doc/faq-setup.xml: update setup-x86/_64 --help options for
"Does the Cygwin Setup program accept command-line arguments?";
update compressed setup file types;
remove install everything instructions and provide total size info, time
required, and 32-bit Cygwin address space limitations;
winsup/doc/faq-using.xml
winsup/doc/faq-what.xml
Preview diff attached.
--
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.
[Data in IEC units and prefixes, physical quantities in SI.]
diff --git a/winsup/doc/faq-api.xml b/winsup/doc/faq-api.xml
index 313f15d37c..8b149cd76f 100644
--- a/winsup/doc/faq-api.xml
+++ b/winsup/doc/faq-api.xml
@@ -5,7 +5,7 @@
Cygwin API Questions
-
+
How does everything work?
@@ -18,7 +18,7 @@ Windows into the C library. Then your apps should (ideally)
run on POSIX
systems (Unix/Linux) and Windows with no changes at the source level.
The C library is in a DLL, which makes basic applications quite small.
-And it allows relatively easy upgrades to the Win32/POSIX translation
+And it allows relatively easy upgrades to the Windows/POSIX translation
layer, providing that DLL changes stay backward-compatible.
For a good overview of Cygwin, you may want to read the Cygwin
@@ -140,7 +140,7 @@ spawn family of calls if possible.
Here's how it works:
Parent initializes a space in the Cygwin process table for child.
-Parent creates child suspended using Win32 CreateProcess call, giving
+Parent creates child suspended using Windows CreateProcess call, giving
the same path it was invoked with itself. Parent calls setjmp to save
its own context and then sets a pointer to this in the Cygwin shared
memory area (shared among all Cygwin tasks). Parent fills in the child's
@@ -326,7 +326,7 @@ name under the API.
E.g., the POSIX select system call can wait on a standard file handles
and handles to sockets. The select call in Winsock can only wait on
sockets. Because of this, the Cygwin dll does a lot of nasty stuff behind
-the scenes, trying to persuade various Winsock/Win32 functions to do what
+the scenes, trying to persuade various Winsock/Windows functions to do what
a Unix select would do.
If you are porting an application which already uses Winsock, then
@@ -337,11 +337,11 @@ direct calls to Winsock functions. If you use Cygwin,
use the POSIX API.
-I don't want Unix sockets, how do I use normal Win32
winsock?
+I don't want Unix sockets, how do I use normal Windows
winsock?
You don't. Look for the Mingw-w64 project to port applications using
-native Win32/Winsock functions. Cross compilers packages to build Mingw-w64
+native Windows/Winsock functions. Cross compilers packages to build Mingw-w64
targets are available in the Cygwin distro.
@@ -385,13 +385,34 @@ Cygwin version number details, check out the
-Why isn't timezone set correctly?
+Why isn't time zone set correctly?
-(Please note: This section has not yet been
updated for the latest net release.)
-
-Did you explicitly call tzset() before checking the value of timezone?
+Did you explicitly call tzset() before checking the value of time zone?
If not, you must do so.
+Time zone settings are updated by changes to the tzdata package included in all
+Cygwin installations.
+Have you run the Cygwin Setup program recently to update at least the
+tzdata
+package to include the latest current daylight saving (summer time) rules
+for dates of changes, hour offsets from UTC of time zones, and the
+geographic regions to which those rules and offsets apply.
+
+These changes are decided on by politicians, and announced
+by government officials, sometimes with short or no notice, so
+tzdata
+package updates are released at least a few, and sometimes several,
+times a year.
+As details of changes are not known until they are announced publicly by
+officials, often in foreign languages, and those details then have to be
+noticed, possibly translated, passed to, and picked up by the official
+tzdata
+source package maintainers, subsequently released in an update to the
+tzdata
+source package, and then those changes have to be picked up on and applied
+to the Cygwin
+tzdata
+package, which
Re: [PATCH 00/12] FIFO: fix multiple reader support
Hi Ken, On Jul 16 12:19, Ken Brown via Cygwin-patches wrote: > There were several flaws in my previous attempt to add support for > explicitly opening a FIFO multiple times for reading. (By > "explicitly" I mean by calling open rather than by calling > fork/exec/dup.) See > > https://sourceware.org/pipermail/cygwin/2020-July/245456.html > > for one indication of problems > > The most important flaw was that I tried to use an indirect, > unreliable method for determining whether there are writers open. > This is fixed in the second patch of this series by adding a member > '_nwriters' to struct fifo_shmem_t, which counts the number of open > writers. > > We now have to give writers access to the shared memory as well as > readers, so that they can increment _nwriters in open/fork/exec/dup > and decrement it in close. > > The other patches contain miscellaneous fixes/improvements. > > Ken Brown (12): > Cygwin: FIFO: fix problems finding new owner > Cygwin: FIFO: keep a writer count in shared memory > Cygwin: fhandler_fifo::hit_eof: improve reliability > Cygwin: FIFO: reduce I/O interleaving > Cygwin: FIFO: improve taking ownership in fifo_reader_thread > Cygwin: FIFO: fix indentation > Cygwin: FIFO: make certain errors non-fatal > Cygwin: FIFO: add missing lock > Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily > Cygwin: FIFO: allow take_ownership to be interrupted > Cygwin: FIFO: clean up > Cygwin: FIFO: update commentary > > winsup/cygwin/fhandler.h | 55 +-- > winsup/cygwin/fhandler_fifo.cc | 725 ++--- > winsup/cygwin/select.cc| 14 +- > 3 files changed, 433 insertions(+), 361 deletions(-) LGTM, please push. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer
[PATCH 09/12] Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily
Don't set update_needed_evt if there's currently no owner. This will cause unnecessary churn once I'm the owner and am listening for connections. --- winsup/cygwin/fhandler_fifo.cc | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index b6e172ddc..fd1695f40 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -1186,8 +1186,11 @@ fhandler_fifo::take_ownership () return; } set_pending_owner (me); + /* Wake up my fifo_reader_thread. */ owner_needed (); - SetEvent (update_needed_evt); + if (get_owner ()) +/* Wake up owner's fifo_reader_thread. */ +SetEvent (update_needed_evt); owner_unlock (); /* The reader threads should now do the transfer. */ WaitForSingleObject (owner_found_evt, INFINITE); -- 2.27.0
[PATCH 01/12] Cygwin: FIFO: fix problems finding new owner
When the owning reader closes and there are still readers open, the
owner needs to wait for a new owner to be found before closing its
fifo_client handlers. This involves a loop in which dec_nreaders is
called at the beginning and inc_nreaders is called at the end. Any
other reader that tries to access shmem->_nreaders during this loop
will therefore get an inaccurate answer.
Fix this by adding an nreaders method and using it instead of
dec_nreaders and inc_nreaders. Also add nreaders_lock to control
access to the shmem->_nreaders.
Make various other changes to improve the reliability of finding a new
owner.
---
winsup/cygwin/fhandler.h | 8 +++-
winsup/cygwin/fhandler_fifo.cc | 86 +-
2 files changed, 61 insertions(+), 33 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 7a28adc16..cf6daea06 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1328,7 +1328,7 @@ class fifo_shmem_t
{
LONG _nreaders;
fifo_reader_id_t _owner, _prev_owner, _pending_owner;
- af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock;
+ af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock,
_nreaders_lock;
/* Info about shared memory block used for temporary storage of the
owner's fc_handler list. */
@@ -1336,6 +1336,7 @@ class fifo_shmem_t
_sh_fc_handler_updated;
public:
+ int nreaders () const { return (int) _nreaders; }
int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
@@ -1352,6 +1353,8 @@ public:
void reading_unlock () { _reading_lock.unlock (); }
void reader_opening_lock () { _reader_opening_lock.lock (); }
void reader_opening_unlock () { _reader_opening_lock.unlock (); }
+ void nreaders_lock () { _nreaders_lock.lock (); }
+ void nreaders_unlock () { _nreaders_lock.unlock (); }
int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n);
}
@@ -1420,8 +1423,11 @@ class fhandler_fifo: public fhandler_base
int reopen_shared_fc_handler ();
int remap_shared_fc_handler (size_t);
+ int nreaders () const { return shmem->nreaders (); }
int inc_nreaders () { return shmem->inc_nreaders (); }
int dec_nreaders () { return shmem->dec_nreaders (); }
+ void nreaders_lock () { shmem->nreaders_lock (); }
+ void nreaders_unlock () { shmem->nreaders_unlock (); }
fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
void set_prev_owner (fifo_reader_id_t fr_id)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 3d34cdfab..2d4f7a97e 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -371,6 +371,8 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
int
fhandler_fifo::update_my_handlers ()
{
+ int ret = 0;
+
close_all_handlers ();
fifo_reader_id_t prev = get_prev_owner ();
if (!prev)
@@ -387,7 +389,7 @@ fhandler_fifo::update_my_handlers ()
{
debug_printf ("Can't open process of previous owner, %E");
__seterrno ();
- return -1;
+ goto out;
}
for (int i = 0; i < get_shared_nhandlers (); i++)
@@ -402,11 +404,13 @@ fhandler_fifo::update_my_handlers ()
debug_printf ("Can't duplicate handle of previous owner, %E");
--nhandlers;
__seterrno ();
- return -1;
+ goto out;
}
fc.state = shared_fc_handler[i].state;
}
- return 0;
+out:
+ set_prev_owner (null_fr_id);
+ return ret;
}
int
@@ -1414,41 +1418,59 @@ fhandler_fifo::close ()
{
if (reader)
{
- /* If we're the owner, try to find a new owner. */
- bool find_new_owner = false;
+ /* If we're the owner, we can't close our fc_handlers if a new
+owner might need to duplicate them. */
+ bool close_fc_ok = false;
cancel_reader_thread ();
- owner_lock ();
- if (get_owner () == me)
+ nreaders_lock ();
+ if (dec_nreaders () == 0)
{
- find_new_owner = true;
+ close_fc_ok = true;
+ ResetEvent (read_ready);
+ ResetEvent (owner_needed_evt);
+ ResetEvent (owner_found_evt);
set_owner (null_fr_id);
- set_prev_owner (me);
- owner_needed ();
+ set_prev_owner (null_fr_id);
+ set_pending_owner (null_fr_id);
+ set_shared_nhandlers (0);
}
- owner_unlock ();
- if (dec_nreaders () == 0)
- ResetEvent (read_ready);
- else if (find_new_owner && !IsEventSignalled (owner_found_evt))
+ else
{
- bool found = false;
- do
- if (dec_nreaders () >= 0)
- {
- /* There's still another reader open. */
- if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0)
-
[PATCH 02/12] Cygwin: FIFO: keep a writer count in shared memory
When a reader opens, it needs to block if there are no writers open
(unless is is opened with O_NONBLOCK). This is easy for the first
reader to test, since it can just wait for a writer to signal that it
is open (via the write_ready event). But when a second reader wants
to open, all writers might have closed.
To check this, use a new '_nwriters' member of struct fifo_shmem_t,
which keeps track of the number of open writers. This should be more
reliable than the previous method.
Add nwriters_lock to control access to shmem->_nwriters, and remove
reader_opening_lock, which is no longer needed.
Previously only readers had access to the shared memory, but now
writers access it too so that they can increment _nwriters during
open/dup/fork/exec and decrement it during close.
Add an optional 'only_open' argument to create_shmem for use by
writers, which only open the shared memory rather than first trying to
create it. Since writers don't need to access the shared memory until
they have successfully connected to a pipe instance, they can safely
assume that a reader has already created the shared memory.
For debugging purposes, change create_shmem to return 1 instead of 0
when a reader successfully opens the shared memory after finding that
it had already been created.
Remove check_write_ready_evt, write_ready_ok_evt, and
check_write_ready(), which are no longer needed.
When opening a writer and looping to try to get a connection, recheck
read_ready at the top of the loop since the number of readers might
have changed.
To slightly speed up the process of opening the first reader, take
ownership immediately rather than waiting for the fifo_reader_thread
to handle it.
---
winsup/cygwin/fhandler.h | 27 ++--
winsup/cygwin/fhandler_fifo.cc | 263 ++---
2 files changed, 124 insertions(+), 166 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index cf6daea06..f034a110e 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1323,12 +1323,14 @@ struct fifo_reader_id_t
}
};
-/* Info needed by all readers of a FIFO, stored in named shared memory. */
+/* Info needed by all fhandlers for a given FIFO, stored in named
+ shared memory. This is mostly for readers, but writers need access
+ in order to update the count of open writers. */
class fifo_shmem_t
{
- LONG _nreaders;
+ LONG _nreaders, _nwriters;
fifo_reader_id_t _owner, _prev_owner, _pending_owner;
- af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock,
_nreaders_lock;
+ af_unix_spinlock_t _owner_lock, _reading_lock, _nreaders_lock,
_nwriters_lock;
/* Info about shared memory block used for temporary storage of the
owner's fc_handler list. */
@@ -1339,6 +1341,9 @@ public:
int nreaders () const { return (int) _nreaders; }
int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
+ int nwriters () const { return (int) _nwriters; }
+ int inc_nwriters () { return (int) InterlockedIncrement (&_nwriters); }
+ int dec_nwriters () { return (int) InterlockedDecrement (&_nwriters); }
fifo_reader_id_t get_owner () const { return _owner; }
void set_owner (fifo_reader_id_t fr_id) { _owner = fr_id; }
@@ -1351,10 +1356,10 @@ public:
void owner_unlock () { _owner_lock.unlock (); }
void reading_lock () { _reading_lock.lock (); }
void reading_unlock () { _reading_lock.unlock (); }
- void reader_opening_lock () { _reader_opening_lock.lock (); }
- void reader_opening_unlock () { _reader_opening_lock.unlock (); }
void nreaders_lock () { _nreaders_lock.lock (); }
void nreaders_unlock () { _nreaders_lock.unlock (); }
+ void nwriters_lock () { _nwriters_lock.lock (); }
+ void nwriters_unlock () { _nwriters_lock.unlock (); }
int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n);
}
@@ -1380,8 +1385,6 @@ class fhandler_fifo: public fhandler_base
HANDLE owner_needed_evt; /* The owner is closing. */
HANDLE owner_found_evt; /* A new owner has taken over. */
HANDLE update_needed_evt; /* shared_fc_handler needs updating. */
- HANDLE check_write_ready_evt; /* write_ready needs to be checked. */
- HANDLE write_ready_ok_evt;/* check_write_ready is done. */
/* Handles to non-shared events needed for fifo_reader_threads. */
HANDLE cancel_evt;/* Signal thread to terminate. */
@@ -1417,7 +1420,7 @@ class fhandler_fifo: public fhandler_base
void record_connection (fifo_client_handler&,
fifo_client_connect_state = fc_connected);
- int create_shmem ();
+ int create_shmem (bool only_open = false);
int reopen_shmem ();
int create_shared_fc_handler ();
int reopen_shared_fc_handler ();
@@ -1426,8 +1429,13 @@ class fhandler_fifo: public fhandler_base
int nreaders
[PATCH 04/12] Cygwin: FIFO: reduce I/O interleaving
Add a bool member 'last_read' to the fifo_client_handler structure,
which is set to true on a successful read. This is used by raw_read
as follows.
When raw_read is called, it first locates the writer (if any) for
which last_read is true. raw_read tries to read from that writer and
returns if there is input available. Otherwise, it proceeds to poll
all the writers, as before.
The effect of this is that if a writer writes some data that is only
partially read, the next attempt to read will continue to read from
the same writer. This should reduce the interleaving of output from
different writers.
---
winsup/cygwin/fhandler.h | 3 +-
winsup/cygwin/fhandler_fifo.cc | 55 +-
2 files changed, 50 insertions(+), 8 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index b5bfdd0b3..221c856e6 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1298,7 +1298,8 @@ struct fifo_client_handler
{
HANDLE h;
fifo_client_connect_state state;
- fifo_client_handler () : h (NULL), state (fc_unknown) {}
+ bool last_read; /* true if our last successful read was from this client. */
+ fifo_client_handler () : h (NULL), state (fc_unknown), last_read (false) {}
void close () { NtClose (h); }
fifo_client_connect_state set_state ();
};
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 3685cc0c2..afe21a468 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -404,6 +404,7 @@ fhandler_fifo::update_my_handlers ()
goto out;
}
fc.state = shared_fc_handler[i].state;
+ fc.last_read = shared_fc_handler[i].last_read;
}
out:
set_prev_owner (null_fr_id);
@@ -1200,15 +1201,56 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
/* No one else can take ownership while we hold the reading_lock. */
reading_lock ();
take_ownership ();
- /* Poll the connected clients for input. */
- int nconnected = 0;
+ /* Poll the connected clients for input. Make two passes. On
+the first pass, just try to read from the client from which
+we last read successfully. This should minimize
+interleaving of writes from different clients. */
fifo_client_lock ();
+ /* First pass. */
+ int j;
+ for (j = 0; j < nhandlers; j++)
+ if (fc_handler[j].last_read)
+ break;
+ if (j < nhandlers && fc_handler[j].state >= fc_closing)
+ {
+ NTSTATUS status;
+ IO_STATUS_BLOCK io;
+
+ status = NtReadFile (fc_handler[j].h, NULL, NULL, NULL,
+ , in_ptr, len, NULL, NULL);
+ switch (status)
+ {
+ case STATUS_SUCCESS:
+ case STATUS_BUFFER_OVERFLOW:
+ /* io.Information is supposedly valid in latter case. */
+ if (io.Information > 0)
+ {
+ len = io.Information;
+ fifo_client_unlock ();
+ reading_unlock ();
+ return;
+ }
+ break;
+ case STATUS_PIPE_EMPTY:
+ break;
+ case STATUS_PIPE_BROKEN:
+ fc_handler[j].state = fc_disconnected;
+ break;
+ default:
+ debug_printf ("NtReadFile status %y", status);
+ fc_handler[j].state = fc_error;
+ break;
+ }
+ fc_handler[j].last_read = false;
+ }
+
+ /* Second pass. */
+ int nconnected = 0;
for (int i = 0; i < nhandlers; i++)
if (fc_handler[i].state >= fc_closing)
{
NTSTATUS status;
IO_STATUS_BLOCK io;
- size_t nbytes = 0;
nconnected++;
status = NtReadFile (fc_handler[i].h, NULL, NULL, NULL,
@@ -1217,11 +1259,10 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
{
case STATUS_SUCCESS:
case STATUS_BUFFER_OVERFLOW:
- /* io.Information is supposedly valid. */
- nbytes = io.Information;
- if (nbytes > 0)
+ if (io.Information > 0)
{
- len = nbytes;
+ len = io.Information;
+ fc_handler[i].last_read = true;
fifo_client_unlock ();
reading_unlock ();
return;
--
2.27.0
[PATCH 07/12] Cygwin: FIFO: make certain errors non-fatal
If update_my_handlers fails to duplicate one or more handles, just
mark the corresponding handlers as being in an error state.
But if update_my_handlers is unable to open the process of the
previous owner, it's likely that something serious has gone wrong, so
we continue to make that a fatal error.
---
winsup/cygwin/fhandler_fifo.cc | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 69dda0811..91a276ee9 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -383,11 +383,7 @@ fhandler_fifo::update_my_handlers ()
else
prev_proc = OpenProcess (PROCESS_DUP_HANDLE, false, prev.winpid);
if (!prev_proc)
-{
- debug_printf ("Can't open process of previous owner, %E");
- __seterrno ();
- goto out;
-}
+api_fatal ("Can't open process of previous owner, %E");
for (int i = 0; i < get_shared_nhandlers (); i++)
{
@@ -399,14 +395,17 @@ fhandler_fifo::update_my_handlers ()
!close_on_exec (), DUPLICATE_SAME_ACCESS))
{
debug_printf ("Can't duplicate handle of previous owner, %E");
- --nhandlers;
__seterrno ();
- goto out;
+ fc.state = fc_error;
+ fc.last_read = false;
+ ret = -1;
+ }
+ else
+ {
+ fc.state = shared_fc_handler[i].state;
+ fc.last_read = shared_fc_handler[i].last_read;
}
- fc.state = shared_fc_handler[i].state;
- fc.last_read = shared_fc_handler[i].last_read;
}
-out:
set_prev_owner (null_fr_id);
return ret;
}
@@ -493,7 +492,7 @@ fhandler_fifo::fifo_reader_thread_func ()
set_owner (me);
set_pending_owner (null_fr_id);
if (update_my_handlers () < 0)
- api_fatal ("Can't update my handlers, %E");
+ debug_printf ("error updating my handlers, %E");
owner_found ();
owner_unlock ();
/* Fall through to owner_listen. */
--
2.27.0
[PATCH 12/12] Cygwin: FIFO: update commentary
--- winsup/cygwin/fhandler_fifo.cc | 21 + 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index 30486304f..e9d0187d4 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -52,10 +52,23 @@ which is mostly idle. The thread wakes up if that reader might need to take ownership. - There is a block of shared memory, accessible to all readers, - that contains information needed for the owner change process. - It also contains some locks to prevent races and deadlocks - between the various threads. + There is a block of named shared memory, accessible to all + fhandlers for a given FIFO. It keeps track of the number of open + readers and writers; it contains information needed for the owner + change process; and it contains some locks to prevent races and + deadlocks between the various threads. + + The shared memory is created by the first reader to open the + FIFO. It is opened by subsequent readers and by all writers. It + is destroyed by Windows when the last handle to it is closed. + + If a handle to it somehow remains open after all processes + holding file descriptors to the FIFO have closed, the shared + memory can persist and be reused with stale data by the next + process that opens the FIFO. So far I've seen this happen only + as a result of a bug in the code, but there are some debug_printf + statements in fhandler_fifo::open to help detect this if it + happens again. At this writing, I know of only one application (Midnight Commander when running under tcsh) that *explicitly* opens two -- 2.27.0
[PATCH 05/12] Cygwin: FIFO: improve taking ownership in fifo_reader_thread
When a reader takes ownership in fifo_reader_thread, it now goes
directly to the part of the main loop that listens for a connection.
Previously it went back to the beginning of the loop.
Also, if the reader has to delay taking ownership because the previous
owner has not finished updating the shared fifo_client handlers, it
now checks to see if cancel_evt has been set. Previously it might
have had to spin its wheels unnecessarily only to eventually find that
its thread had been canceled.
---
winsup/cygwin/fhandler_fifo.cc | 44 ++
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index afe21a468..1fb319fcf 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -462,12 +462,30 @@ fhandler_fifo::fifo_reader_thread_func ()
take_ownership = true;
else if (cur_owner != me)
idle = true;
- if (take_ownership)
+ else
+ /* I'm the owner. */
+ goto owner_listen;
+ if (idle)
+ {
+ owner_unlock ();
+ HANDLE w[2] = { owner_needed_evt, cancel_evt };
+ switch (WaitForMultipleObjects (2, w, false, INFINITE))
+ {
+ case WAIT_OBJECT_0:
+ continue;
+ case WAIT_OBJECT_0 + 1:
+ goto canceled;
+ default:
+ api_fatal ("WFMO failed, %E");
+ }
+ }
+ else if (take_ownership)
{
if (!shared_fc_handler_updated ())
{
owner_unlock ();
- yield ();
+ if (IsEventSignalled (cancel_evt))
+ goto canceled;
continue;
}
else
@@ -478,26 +496,11 @@ fhandler_fifo::fifo_reader_thread_func ()
api_fatal ("Can't update my handlers, %E");
owner_found ();
owner_unlock ();
- continue;
+ /* Fall through to owner_listen. */
}
}
- else if (idle)
- {
- owner_unlock ();
- HANDLE w[2] = { owner_needed_evt, cancel_evt };
- switch (WaitForMultipleObjects (2, w, false, INFINITE))
- {
- case WAIT_OBJECT_0:
- continue;
- case WAIT_OBJECT_0 + 1:
- goto canceled;
- default:
- api_fatal ("WFMO failed, %E");
- }
- }
- else
- {
- /* I'm the owner */
+
+owner_listen:
fifo_client_lock ();
cleanup_handlers ();
if (add_client_handler () < 0)
@@ -590,7 +593,6 @@ fhandler_fifo::fifo_reader_thread_func ()
fifo_client_unlock ();
if (cancel)
goto canceled;
- }
}
canceled:
if (conn_evt)
--
2.27.0
[PATCH 11/12] Cygwin: FIFO: clean up
Remove the fhandler_fifo::get_me method, which is no longer used.
Make the methods get_owner, set_owner, owner_lock, and owner_unlock
private.
---
winsup/cygwin/fhandler.h | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 0e0cfbd71..60bd27e00 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1437,6 +1437,8 @@ class fhandler_fifo: public fhandler_base
void nwriters_lock () { shmem->nwriters_lock (); }
void nwriters_unlock () { shmem->nwriters_unlock (); }
+ fifo_reader_id_t get_owner () const { return shmem->get_owner (); }
+ void set_owner (fifo_reader_id_t fr_id) { shmem->set_owner (fr_id); }
fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
void set_prev_owner (fifo_reader_id_t fr_id)
{ shmem->set_prev_owner (fr_id); }
@@ -1444,6 +1446,8 @@ class fhandler_fifo: public fhandler_base
{ return shmem->get_pending_owner (); }
void set_pending_owner (fifo_reader_id_t fr_id)
{ shmem->set_pending_owner (fr_id); }
+ void owner_lock () { shmem->owner_lock (); }
+ void owner_unlock () { shmem->owner_unlock (); }
void owner_needed ()
{
@@ -1483,12 +1487,6 @@ public:
void fifo_client_lock () { _fifo_client_lock.lock (); }
void fifo_client_unlock () { _fifo_client_lock.unlock (); }
- fifo_reader_id_t get_me () const { return me; }
- fifo_reader_id_t get_owner () const { return shmem->get_owner (); }
- void set_owner (fifo_reader_id_t fr_id) { shmem->set_owner (fr_id); }
- void owner_lock () { shmem->owner_lock (); }
- void owner_unlock () { shmem->owner_unlock (); }
-
DWORD take_ownership ();
void reading_lock () { shmem->reading_lock (); }
void reading_unlock () { shmem->reading_unlock (); }
--
2.27.0
[PATCH 00/12] FIFO: fix multiple reader support
There were several flaws in my previous attempt to add support for explicitly opening a FIFO multiple times for reading. (By "explicitly" I mean by calling open rather than by calling fork/exec/dup.) See https://sourceware.org/pipermail/cygwin/2020-July/245456.html for one indication of problems The most important flaw was that I tried to use an indirect, unreliable method for determining whether there are writers open. This is fixed in the second patch of this series by adding a member '_nwriters' to struct fifo_shmem_t, which counts the number of open writers. We now have to give writers access to the shared memory as well as readers, so that they can increment _nwriters in open/fork/exec/dup and decrement it in close. The other patches contain miscellaneous fixes/improvements. Ken Brown (12): Cygwin: FIFO: fix problems finding new owner Cygwin: FIFO: keep a writer count in shared memory Cygwin: fhandler_fifo::hit_eof: improve reliability Cygwin: FIFO: reduce I/O interleaving Cygwin: FIFO: improve taking ownership in fifo_reader_thread Cygwin: FIFO: fix indentation Cygwin: FIFO: make certain errors non-fatal Cygwin: FIFO: add missing lock Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily Cygwin: FIFO: allow take_ownership to be interrupted Cygwin: FIFO: clean up Cygwin: FIFO: update commentary winsup/cygwin/fhandler.h | 55 +-- winsup/cygwin/fhandler_fifo.cc | 725 ++--- winsup/cygwin/select.cc| 14 +- 3 files changed, 433 insertions(+), 361 deletions(-) -- 2.27.0
[PATCH 08/12] Cygwin: FIFO: add missing lock
--- winsup/cygwin/fhandler_fifo.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index 91a276ee9..b6e172ddc 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -422,7 +422,9 @@ fhandler_fifo::update_shared_handlers () set_shared_nhandlers (nhandlers); memcpy (shared_fc_handler, fc_handler, nhandlers * sizeof (fc_handler[0])); shared_fc_handler_updated (true); + owner_lock (); set_prev_owner (me); + owner_unlock (); return 0; } -- 2.27.0
[PATCH 03/12] Cygwin: fhandler_fifo::hit_eof: improve reliability
Use the writer count introduced in the previous commit to help detect
EOF. Drop the maybe_eof method, which is no longer needed.
---
winsup/cygwin/fhandler.h | 7 +++
winsup/cygwin/fhandler_fifo.cc | 26 ++
winsup/cygwin/select.cc| 3 +--
3 files changed, 6 insertions(+), 30 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index f034a110e..b5bfdd0b3 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1392,7 +1392,6 @@ class fhandler_fifo: public fhandler_base
UNICODE_STRING pipe_name;
WCHAR pipe_name_buf[CYGWIN_FIFO_PIPE_NAME_LEN + 1];
- bool _maybe_eof;
fifo_client_handler *fc_handler; /* Dynamically growing array. */
int shandlers; /* Size (capacity) of the array. */
int nhandlers; /* Number of elements in the array. */
@@ -1473,9 +1472,9 @@ class fhandler_fifo: public fhandler_base
public:
fhandler_fifo ();
- bool hit_eof ();
- bool maybe_eof () const { return _maybe_eof; }
- void maybe_eof (bool val) { _maybe_eof = val; }
+ /* Called if we appear to be at EOF after polling fc_handlers. */
+ bool hit_eof () const
+ { return !nwriters () && !IsEventSignalled (writer_opening); }
int get_nhandlers () const { return nhandlers; }
fifo_client_handler _fc_handler (int i) { return fc_handler[i]; }
PUNICODE_STRING get_pipe_name ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 26b24d019..3685cc0c2 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -87,7 +87,7 @@ fhandler_fifo::fhandler_fifo ():
fhandler_base (),
read_ready (NULL), write_ready (NULL), writer_opening (NULL),
owner_needed_evt (NULL), owner_found_evt (NULL), update_needed_evt (NULL),
- cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false),
+ cancel_evt (NULL), thr_sync_evt (NULL),
fc_handler (NULL), shandlers (0), nhandlers (0),
reader (false), writer (false), duplexer (false),
max_atomic_write (DEFAULT_PIPEBUFSIZE),
@@ -361,8 +361,6 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
fifo_client_connect_state s)
{
fc.state = s;
- maybe_eof (false);
- ResetEvent (writer_opening);
set_pipe_non_blocking (fc.h, true);
}
@@ -1173,25 +1171,6 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
return ret;
}
-/* A reader is at EOF if the pipe is empty and no writers are open.
- hit_eof is called by raw_read and select.cc:peek_fifo if it appears
- that we are at EOF after polling the fc_handlers. We recheck this
- in case a writer opened while we were polling. */
-bool
-fhandler_fifo::hit_eof ()
-{
- bool ret = maybe_eof () && !IsEventSignalled (writer_opening);
- if (ret)
-{
- yield ();
- /* Wait for the reader thread to finish recording any connection. */
- fifo_client_lock ();
- fifo_client_unlock ();
- ret = maybe_eof ();
-}
- return ret;
-}
-
/* Called from raw_read and select.cc:peek_fifo. */
void
fhandler_fifo::take_ownership ()
@@ -1261,9 +1240,8 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
break;
}
}
- maybe_eof (!nconnected && !IsEventSignalled (writer_opening));
fifo_client_unlock ();
- if (maybe_eof () && hit_eof ())
+ if (!nconnected && hit_eof ())
{
reading_unlock ();
len = 0;
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 9ae567c38..80d16f2a7 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -883,9 +883,8 @@ peek_fifo (select_record *s, bool from_select)
goto out;
}
}
- fh->maybe_eof (!nconnected);
fh->fifo_client_unlock ();
- if (fh->maybe_eof () && fh->hit_eof ())
+ if (!nconnected && fh->hit_eof ())
{
select_printf ("read: %s, saw EOF", fh->get_name ());
gotone += s->read_ready = true;
--
2.27.0
[PATCH 06/12] Cygwin: FIFO: fix indentation
---
winsup/cygwin/fhandler_fifo.cc | 168 -
1 file changed, 84 insertions(+), 84 deletions(-)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 1fb319fcf..69dda0811 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -501,98 +501,98 @@ fhandler_fifo::fifo_reader_thread_func ()
}
owner_listen:
- fifo_client_lock ();
- cleanup_handlers ();
- if (add_client_handler () < 0)
- api_fatal ("Can't add a client handler, %E");
-
- /* Listen for a writer to connect to the new client handler. */
- fifo_client_handler& fc = fc_handler[nhandlers - 1];
- fifo_client_unlock ();
- shared_fc_handler_updated (false);
- owner_unlock ();
- NTSTATUS status;
- IO_STATUS_BLOCK io;
- bool cancel = false;
- bool update = false;
+ fifo_client_lock ();
+ cleanup_handlers ();
+ if (add_client_handler () < 0)
+ api_fatal ("Can't add a client handler, %E");
- status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, ,
- FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
- if (status == STATUS_PENDING)
- {
- HANDLE w[3] = { conn_evt, update_needed_evt, cancel_evt };
- switch (WaitForMultipleObjects (3, w, false, INFINITE))
- {
- case WAIT_OBJECT_0:
- status = io.Status;
- debug_printf ("NtFsControlFile STATUS_PENDING, then %y",
- status);
- break;
- case WAIT_OBJECT_0 + 1:
- status = STATUS_WAIT_1;
- update = true;
- break;
- case WAIT_OBJECT_0 + 2:
- status = STATUS_THREAD_IS_TERMINATING;
- cancel = true;
- update = true;
- break;
- default:
- api_fatal ("WFMO failed, %E");
- }
- }
- else
- debug_printf ("NtFsControlFile status %y, no STATUS_PENDING",
- status);
- HANDLE ph = NULL;
- NTSTATUS status1;
+ /* Listen for a writer to connect to the new client handler. */
+ fifo_client_handler& fc = fc_handler[nhandlers - 1];
+ fifo_client_unlock ();
+ shared_fc_handler_updated (false);
+ owner_unlock ();
+ NTSTATUS status;
+ IO_STATUS_BLOCK io;
+ bool cancel = false;
+ bool update = false;
- fifo_client_lock ();
- switch (status)
+ status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, ,
+ FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
+ if (status == STATUS_PENDING)
+ {
+ HANDLE w[3] = { conn_evt, update_needed_evt, cancel_evt };
+ switch (WaitForMultipleObjects (3, w, false, INFINITE))
{
- case STATUS_SUCCESS:
- case STATUS_PIPE_CONNECTED:
- record_connection (fc);
+ case WAIT_OBJECT_0:
+ status = io.Status;
+ debug_printf ("NtFsControlFile STATUS_PENDING, then %y",
+ status);
break;
- case STATUS_PIPE_CLOSING:
- record_connection (fc, fc_closing);
+ case WAIT_OBJECT_0 + 1:
+ status = STATUS_WAIT_1;
+ update = true;
break;
- case STATUS_THREAD_IS_TERMINATING:
- case STATUS_WAIT_1:
- /* Try to connect a bogus client. Otherwise fc is still
-listening, and the next connection might not get recorded. */
- status1 = open_pipe (ph);
- WaitForSingleObject (conn_evt, INFINITE);
- if (NT_SUCCESS (status1))
- /* Bogus cilent connected. */
- delete_client_handler (nhandlers - 1);
- else
- /* Did a real client connect? */
- switch (io.Status)
- {
- case STATUS_SUCCESS:
- case STATUS_PIPE_CONNECTED:
- record_connection (fc);
- break;
- case STATUS_PIPE_CLOSING:
- record_connection (fc, fc_closing);
- break;
- default:
- debug_printf ("NtFsControlFile status %y after failing to
connect bogus client or real client", io.Status);
- fc.state = fc_unknown;
- break;
- }
+ case WAIT_OBJECT_0 + 2:
+ status = STATUS_THREAD_IS_TERMINATING;
+ cancel = true;
+ update = true;
break;
default:
- break;
+ api_fatal ("WFMO failed, %E");
}
- if (ph)
- NtClose (ph);
- if (update && update_shared_handlers () < 0)
-
[PATCH 10/12] Cygwin: FIFO: allow take_ownership to be interrupted
Use cygwait in take_ownership to allow interruption while waiting to
become owner. Return the cygwait return value or a suitable value to
indicate an error.
raw_read now checks the return value and acts accordingly.
---
winsup/cygwin/fhandler.h | 2 +-
winsup/cygwin/fhandler_fifo.cc | 54 ++
winsup/cygwin/select.cc| 11 ++-
3 files changed, 59 insertions(+), 8 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 221c856e6..0e0cfbd71 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1489,7 +1489,7 @@ public:
void owner_lock () { shmem->owner_lock (); }
void owner_unlock () { shmem->owner_unlock (); }
- void take_ownership ();
+ DWORD take_ownership ();
void reading_lock () { shmem->reading_lock (); }
void reading_unlock () { shmem->reading_unlock (); }
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index fd1695f40..30486304f 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1175,15 +1175,16 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
return ret;
}
-/* Called from raw_read and select.cc:peek_fifo. */
-void
+/* Called from raw_read and select.cc:peek_fifo. Return WAIT_OBJECT_0
+ on success. */
+DWORD
fhandler_fifo::take_ownership ()
{
owner_lock ();
if (get_owner () == me)
{
owner_unlock ();
- return;
+ return WAIT_OBJECT_0;
}
set_pending_owner (me);
/* Wake up my fifo_reader_thread. */
@@ -1192,8 +1193,19 @@ fhandler_fifo::take_ownership ()
/* Wake up owner's fifo_reader_thread. */
SetEvent (update_needed_evt);
owner_unlock ();
- /* The reader threads should now do the transfer. */
- WaitForSingleObject (owner_found_evt, INFINITE);
+ /* The reader threads should now do the transfer. */
+ DWORD waitret = cygwait (owner_found_evt, cw_cancel | cw_sig_eintr);
+ owner_lock ();
+ if (waitret == WAIT_OBJECT_0
+ && (get_owner () != me || get_pending_owner ()))
+{
+ /* Something went wrong. Return WAIT_TIMEOUT, which can't be
+returned by the above cygwait call. */
+ set_pending_owner (null_fr_id);
+ waitret = WAIT_TIMEOUT;
+}
+ owner_unlock ();
+ return waitret;
}
void __reg3
@@ -1206,7 +1218,37 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
{
/* No one else can take ownership while we hold the reading_lock. */
reading_lock ();
- take_ownership ();
+ switch (take_ownership ())
+ {
+ case WAIT_OBJECT_0:
+ break;
+ case WAIT_SIGNALED:
+ if (_my_tls.call_signal_handler ())
+ {
+ reading_unlock ();
+ continue;
+ }
+ else
+ {
+ set_errno (EINTR);
+ reading_unlock ();
+ goto errout;
+ }
+ break;
+ case WAIT_CANCELED:
+ reading_unlock ();
+ pthread::static_cancel_self ();
+ break;
+ case WAIT_TIMEOUT:
+ reading_unlock ();
+ debug_printf ("take_ownership returned an unexpected result; retry");
+ continue;
+ default:
+ reading_unlock ();
+ debug_printf ("unknown error while trying to take ownership, %E");
+ goto errout;
+ }
+
/* Poll the connected clients for input. Make two passes. On
the first pass, just try to read from the client from which
we last read successfully. This should minimize
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 80d16f2a7..3f3f33fb5 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -867,7 +867,16 @@ peek_fifo (select_record *s, bool from_select)
}
fh->reading_lock ();
- fh->take_ownership ();
+ if (fh->take_ownership () != WAIT_OBJECT_0)
+ {
+ select_printf ("%s, unable to take ownership", fh->get_name ());
+ fh->reading_unlock ();
+ gotone += s->read_ready = true;
+ if (s->except_selected)
+ gotone += s->except_ready = true;
+ goto out;
+ }
+
fh->fifo_client_lock ();
int nconnected = 0;
for (int i = 0; i < fh->get_nhandlers (); i++)
--
2.27.0
