Re: [PATCH] Cygwin: select: don't report read ready on a FIFO never, opened for writing

2022-10-19 Thread Ken Brown

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


Re: [PATCH] Cygwin: select: don't report read ready on a FIFO never, opened for writing

2022-10-18 Thread Corinna Vinschen
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


[PATCH] Cygwin: select: don't report read ready on a FIFO never, opened for writing

2022-09-23 Thread Ken Brown
Patch attached.  I'm also attaching a test case, that behaves the same on Cygwin 
as on Linux.


Ken#include 
#include 
#include 
#include 
#include 
#include 
#include 

#define FIFO_PATH "/tmp/myfifo"

int
main ()
{
  int fd, tmpfd, nsel;
  fd_set readfds;
  struct timeval wait_tm = { 0l, 20l }; /* 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);
}
From f4a92734eac17dbfc7ff3541eef9611c87184ed0 Mon Sep 17 00:00:00 2001
From: Ken Brown 
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