* [PATCH] Cygwin: select: don't report read ready on a FIFO never, opened for writing
@ 2022-09-23 15:31 Ken Brown
2022-10-18 15:45 ` Corinna Vinschen
0 siblings, 1 reply; 3+ messages in thread
From: Ken Brown @ 2022-09-23 15:31 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 99 bytes --]
Patch attached. I'm also attaching a test case, that behaves the same on Cygwin
as on Linux.
Ken
[-- Attachment #2: select_eof_fifo.c --]
[-- Type: text/plain, Size: 1298 bytes --]
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/select.h>
#include <errno.h>
#include <sys/stat.h>
#define FIFO_PATH "/tmp/myfifo"
int
main ()
{
int fd, tmpfd, nsel;
fd_set readfds;
struct timeval wait_tm = { 0l, 200000l }; /* 200 millisecs */
if (unlink (FIFO_PATH) < 0 && errno != ENOENT)
{
perror ("unlink");
exit (1);
}
if (mkfifo (FIFO_PATH, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH
| S_IWOTH) < 0)
{
perror ("mkfifo");
exit (1);
}
printf ("Opening a FIFO for reading with O_NONBLOCK\n");
if ((fd = open (FIFO_PATH, O_RDONLY | O_NONBLOCK)) < 0)
{
perror ("open");
exit (1);
}
printf ("Testing for read ready...\n");
FD_ZERO (&readfds);
FD_SET (fd, &readfds);
nsel = select (fd + 1, &readfds, NULL, NULL, &wait_tm);
printf (" select returned %d\n", nsel);
printf ("Opening and closing FIFO for writing...\n");
if ((tmpfd = open (FIFO_PATH, O_WRONLY)) < 0)
{
perror ("open");
exit (1);
}
if (close (tmpfd) < 0)
{
perror ("close");
exit (1);
}
FD_ZERO (&readfds);
FD_SET (fd, &readfds);
nsel = select (fd + 1, &readfds, NULL, NULL, &wait_tm);
printf (" now select returned %d\n", nsel);
}
[-- Attachment #3: 0001-Cygwin-select-don-t-report-read-ready-on-a-FIFO-neve.patch --]
[-- Type: text/plain, Size: 5619 bytes --]
From f4a92734eac17dbfc7ff3541eef9611c87184ed0 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Fri, 23 Sep 2022 10:24:04 -0400
Subject: [PATCH] Cygwin: select: don't report read ready on a FIFO never
opened for writing
According to POSIX and the Linux man page, select(2) is supposed to
report read ready if a file is at EOF. In the case of a FIFO, this
means that the pipe is empty and there are no writers. But there
seems to be an undocumented exception, observed on Linux and other
platforms: If no writer has ever been opened, then select(2) does not
report read ready. This can happen if a reader is opened with
O_NONBLOCK before any writers have opened.
This commit makes Cygwin consistent with those other platforms by
introducing a special EOF test, fhandler_fifo::select_hit_eof, which
returns false if there's never been a writer opened.
To implement this we use a new variable '_writer_opened' in the FIFO's
shared memory, which is set to 1 the first time a writer opens. New
methods writer_opened() and set_writer_opened() are used to test and
set this variable.
Addresses: https://cygwin.com/pipermail/cygwin/2022-September/252223.html
---
winsup/cygwin/fhandler/fifo.cc | 2 ++
winsup/cygwin/local_includes/fhandler.h | 9 +++++++++
winsup/cygwin/release/3.3.4 | 3 +++
winsup/cygwin/select.cc | 12 +++++++++++-
4 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/fhandler/fifo.cc b/winsup/cygwin/fhandler/fifo.cc
index 1d3e42908..ecfb33bff 100644
--- a/winsup/cygwin/fhandler/fifo.cc
+++ b/winsup/cygwin/fhandler/fifo.cc
@@ -1043,6 +1043,8 @@ writer_shmem:
set_pipe_non_blocking (get_handle (), flags & O_NONBLOCK);
nwriters_lock ();
inc_nwriters ();
+ if (!writer_opened () )
+ set_writer_opened ();
SetEvent (write_ready);
ResetEvent (writer_opening);
nwriters_unlock ();
diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
index aad7f4c37..b012c6e8f 100644
--- a/winsup/cygwin/local_includes/fhandler.h
+++ b/winsup/cygwin/local_includes/fhandler.h
@@ -1327,6 +1327,8 @@ struct fifo_reader_id_t
class fifo_shmem_t
{
LONG _nreaders, _nwriters;
+ /* Set to 1 the first time a writer opens. */
+ LONG _writer_opened;
fifo_reader_id_t _owner, _prev_owner, _pending_owner;
af_unix_spinlock_t _owner_lock, _reading_lock, _nreaders_lock, _nwriters_lock;
@@ -1343,6 +1345,9 @@ public:
int inc_nwriters () { return (int) InterlockedIncrement (&_nwriters); }
int dec_nwriters () { return (int) InterlockedDecrement (&_nwriters); }
+ bool writer_opened () const { return (bool) _writer_opened; }
+ void set_writer_opened () { InterlockedExchange (&_writer_opened, 1); }
+
fifo_reader_id_t get_owner () const { return _owner; }
void set_owner (fifo_reader_id_t fr_id) { _owner = fr_id; }
fifo_reader_id_t get_prev_owner () const { return _prev_owner; }
@@ -1425,6 +1430,8 @@ class fhandler_fifo: public fhandler_pipe_fifo
int nwriters () const { return shmem->nwriters (); }
int inc_nwriters () { return shmem->inc_nwriters (); }
int dec_nwriters () { return shmem->dec_nwriters (); }
+ bool writer_opened () const { return shmem->writer_opened (); }
+ void set_writer_opened () { shmem->set_writer_opened (); }
void nreaders_lock () { shmem->nreaders_lock (); }
void nreaders_unlock () { shmem->nreaders_unlock (); }
void nwriters_lock () { shmem->nwriters_lock (); }
@@ -1480,6 +1487,8 @@ public:
/* Called if we appear to be at EOF after polling fc_handlers. */
bool hit_eof () const
{ return !nwriters () && !IsEventSignalled (writer_opening); }
+ /* Special EOF test needed by select.cc:peek_fifo(). */
+ bool select_hit_eof () const { return hit_eof () && writer_opened (); }
int get_nhandlers () const { return nhandlers; }
fifo_client_handler &get_fc_handler (int i) { return fc_handler[i]; }
PUNICODE_STRING get_pipe_name ();
diff --git a/winsup/cygwin/release/3.3.4 b/winsup/cygwin/release/3.3.4
index bc74e6914..616f73ff0 100644
--- a/winsup/cygwin/release/3.3.4
+++ b/winsup/cygwin/release/3.3.4
@@ -37,3 +37,6 @@ Bug Fixes
- Fix a permission problem when writing DOS attributes on Samba.
Addresses: https://cygwin.com/pipermail/cygwin/2022-January/250629.html
+- For Linux compatibility, select(2) no longer reports read ready on a
+ FIFO that has never been opened for writing.
+ Addresses: https://cygwin.com/pipermail/cygwin/2022-September/252223.html
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 76ab91bbb..2fd7b72b6 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -950,7 +950,17 @@ peek_fifo (select_record *s, bool from_select)
}
}
fh->fifo_client_unlock ();
- if (!nconnected && fh->hit_eof ())
+ /* According to POSIX and the Linux man page, we're supposed to
+ report read ready if the FIFO is at EOF, i.e., if the pipe is
+ empty and there are no writers. But there seems to be an
+ undocumented exception, observed on Linux and other platforms
+ (https://cygwin.com/pipermail/cygwin/2022-September/252223.html):
+ If no writer has ever been opened, then we do not report read
+ ready. This can happen if a reader is opened with O_NONBLOCK
+ before any writers have opened. To be consistent with other
+ platforms, we use a special EOF test that returns false if
+ there's never been a writer opened. */
+ if (!nconnected && fh->select_hit_eof ())
{
select_printf ("read: %s, saw EOF", fh->get_name ());
gotone += s->read_ready = true;
--
2.37.3
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Cygwin: select: don't report read ready on a FIFO never, opened for writing
2022-09-23 15:31 [PATCH] Cygwin: select: don't report read ready on a FIFO never, opened for writing Ken Brown
@ 2022-10-18 15:45 ` Corinna Vinschen
2022-10-19 12:31 ` Ken Brown
0 siblings, 1 reply; 3+ messages in thread
From: Corinna Vinschen @ 2022-10-18 15:45 UTC (permalink / raw)
To: cygwin-patches
On Sep 23 11:31, Ken Brown wrote:
> Patch attached. I'm also attaching a test case, that behaves the same on
> Cygwin as on Linux.
Interesting case, but thinking about the scenario, it seems logical to
do it this way.
> @@ -1043,6 +1043,8 @@ writer_shmem:
> set_pipe_non_blocking (get_handle (), flags & O_NONBLOCK);
> nwriters_lock ();
> inc_nwriters ();
> + if (!writer_opened () )
> + set_writer_opened ();
My personal preference would be to skip the writer_opened() check
and just call set_writer_opened(), but that's just me. If you like
it better that way, just push.
Thanks,
Corinna
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Cygwin: select: don't report read ready on a FIFO never, opened for writing
2022-10-18 15:45 ` Corinna Vinschen
@ 2022-10-19 12:31 ` Ken Brown
0 siblings, 0 replies; 3+ messages in thread
From: Ken Brown @ 2022-10-19 12:31 UTC (permalink / raw)
To: cygwin-patches
On 10/18/2022 11:45 AM, Corinna Vinschen wrote:
> On Sep 23 11:31, Ken Brown wrote:
>> @@ -1043,6 +1043,8 @@ writer_shmem:
>> set_pipe_non_blocking (get_handle (), flags & O_NONBLOCK);
>> nwriters_lock ();
>> inc_nwriters ();
>> + if (!writer_opened () )
>> + set_writer_opened ();
>
> My personal preference would be to skip the writer_opened() check
> and just call set_writer_opened(), but that's just me.
I agree. I've pushed it with that change.
Ken
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-19 12:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-23 15:31 [PATCH] Cygwin: select: don't report read ready on a FIFO never, opened for writing Ken Brown
2022-10-18 15:45 ` Corinna Vinschen
2022-10-19 12:31 ` Ken Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).