Rewriting the FIFO code
Corinna Vinschen
corinna-cygwin@cygwin.com
Wed Dec 26 11:37:00 GMT 2018
On Dec 25 21:14, Ken Brown wrote:
> On 12/14/2018 8:43 AM, Ken Brown wrote:
> > I'll write
> > again when/if I've sorted it out.
>
> Hi Corinna,
>
> Here's a new start. For now, at least, I'm only trying to accommodate one
> reader and several writers. Maybe later I'll worry about more than one reader.
>
> The attached patch indicates the approach I have in mind. There's much more to
> do, and I haven't thought about the duplex case yet. And there isn't enough
> written for me to test it yet, except to make sure it compiles. But I just want
> to see if you think the approach is reasonable before I continue.
Please go ahead. I'm really excited that you're working on this. FIFOs
are badly in need of a better approch anyway(*). Turn the code upside
down, for all it's worth!
Happy Holidays,
Corinna
(*) Like so much other old code in Cygwin...
>
> Ken
> From 39b525b40af455e1ac1767359b7ff81d9f263a7c Mon Sep 17 00:00:00 2001
> From: Ken Brown <kbrown@cornell.edu>
> Date: Mon, 24 Dec 2018 10:36:57 -0500
> Subject: [PATCH] Allow several writers to open a FIFO
>
> This is work in progress.
>
> - A FIFO reader opens a Windows named pipe that allows unlimited instances.
> - Each time the FIFO is opened for writing, a new instance of the pipe
> is created for the writer to connect to.
> - This is accomplished by having the reader start a thread that
> listens for connection attempts.
> - The reader maintains a list of connected writers.
> - When the reader wants to read, it polls the writers to see if
> there's any data to be read.
> ---
> winsup/cygwin/fhandler.h | 10 ++-
> winsup/cygwin/fhandler_fifo.cc | 143 ++++++++++++++++++++-------------
> winsup/cygwin/pipe.cc | 6 +-
> 3 files changed, 99 insertions(+), 60 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> index 9e63867ab..042590883 100644
> --- a/winsup/cygwin/fhandler.h
> +++ b/winsup/cygwin/fhandler.h
> @@ -1209,7 +1209,8 @@ public:
> int init (HANDLE, DWORD, mode_t, int64_t);
> static int create (fhandler_pipe *[2], unsigned, int);
> static DWORD create (LPSECURITY_ATTRIBUTES, HANDLE *, HANDLE *, DWORD,
> - const char *, DWORD, int64_t *unique_id = NULL);
> + const char *, DWORD, int64_t *unique_id = NULL,
> + DWORD max_instances = 1);
> fhandler_pipe (void *) {}
>
> void copyto (fhandler_base *x)
> @@ -1229,12 +1230,17 @@ public:
> }
> };
>
> +#define MAX_CLIENTS PIPE_UNLIMITED_INSTANCES
> +
> class fhandler_fifo: public fhandler_base_overlapped
> {
> HANDLE read_ready;
> HANDLE write_ready;
> + fhandler_base_overlapped *client[MAX_CLIENTS];
> + int nclients;
> bool __reg2 wait (HANDLE);
> char __reg2 *fifo_name (char *, const char *);
> + bool wait_client ();
> public:
> fhandler_fifo ();
> int open (int, mode_t);
> @@ -1250,6 +1256,8 @@ public:
> select_record *select_read (select_stuff *);
> select_record *select_write (select_stuff *);
> select_record *select_except (select_stuff *);
> + int add_client (fhandler_base_overlapped *);
> + void del_client (int);
>
> fhandler_fifo (void *) {}
>
> diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
> index 5733ec778..13d0f80c3 100644
> --- a/winsup/cygwin/fhandler_fifo.cc
> +++ b/winsup/cygwin/fhandler_fifo.cc
> @@ -23,7 +23,7 @@
>
> fhandler_fifo::fhandler_fifo ():
> fhandler_base_overlapped (),
> - read_ready (NULL), write_ready (NULL)
> + read_ready (NULL), write_ready (NULL), nclients (0)
> {
> max_atomic_write = DEFAULT_PIPEBUFSIZE;
> need_fork_fixup (true);
> @@ -32,7 +32,8 @@ fhandler_fifo::fhandler_fifo ():
> #define fnevent(w) fifo_name (npbuf, w "-event")
> #define fnpipe() fifo_name (npbuf, "fifo")
> #define create_pipe(r, w) \
> - fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode)
> + fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode, NULL, \
> + PIPE_UNLIMITED_INSTANCES)
>
> char *
> fhandler_fifo::fifo_name (char *buf, const char *what)
> @@ -153,36 +154,25 @@ fhandler_fifo::open (int flags, mode_t)
> res = error_errno_set;
> goto out;
> }
> + else if (!wait_client ())
> + {
> + res = error_errno_set;
> + goto out;
> + }
>
> - /* If we're writing, it's a little tricky since it is possible that
> - we're attempting to open the other end of a pipe which is already
> - connected. In that case, we detect ERROR_PIPE_BUSY, reset the
> - read_ready event and wait for the reader to allow us to connect
> - by signalling read_ready.
> -
> - Once the pipe has been set up, we signal write_ready. */
> if (writer)
> {
> - int err;
> - while (1)
> - if (!wait (read_ready))
> - {
> - res = error_errno_set;
> - goto out;
> - }
> - else if ((err = create_pipe (NULL, &get_io_handle ())) == 0)
> - break;
> - else if (err == ERROR_PIPE_BUSY)
> - {
> - debug_only_printf ("pipe busy");
> - ResetEvent (read_ready);
> - }
> - else
> - {
> - debug_printf ("create of writer failed");
> - res = error_set_errno;
> - goto out;
> - }
> + if (!wait (read_ready))
> + {
> + res = error_errno_set;
> + goto out;
> + }
> + else if (!create_pipe (NULL, &get_io_handle ()))
> + {
> + debug_printf ("create of writer failed");
> + res = error_set_errno;
> + goto out;
> + }
> if (!arm (write_ready))
> {
> res = error_set_errno;
> @@ -221,6 +211,45 @@ out:
> return res == success;
> }
>
> +int
> +fhandler_fifo::add_client (fhandler_base_overlapped *fh)
> +{
> + if (nclients == MAX_CLIENTS)
> + {
> + set_errno (EMFILE);
> + return -1;
> + }
> +
> + client[nclients++] = fh;
> + return 0;
> +}
> +
> +void
> +fhandler_fifo::del_client (int i)
> +{
> + client[i]->close (); /* Do we need to call DisconnectNamedPipe? */
> + delete client[i]; /* Is that enough cleanup? */
> + nclients--;
> + /* Fill in the hole. */
> + if (i < nclients)
> + memmove (client + i, client + i + 1,
> + (nclients - i) * sizeof (client[i]));
> +}
> +
> +bool
> +fhandler_fifo::wait_client ()
> +{
> + /* Start a thread that loops forever and does the following:
> + - waits for a client to connect
> + - creates a new instance of the pipe
> + - sets the io_handle of the reader to the new instance
> + - creates a new fhandler_base_overlapped object whose io_handle
> + is the previous handle of the reader
> + - adds a pointer to that object to the client array.
> + - signals read_ready */
> + return true;
> +}
> +
> off_t
> fhandler_fifo::lseek (off_t offset, int whence)
> {
> @@ -282,40 +311,42 @@ fhandler_fifo::wait (HANDLE h)
> void __reg3
> fhandler_fifo::raw_read (void *in_ptr, size_t& len)
> {
> - size_t orig_len = len;
> - for (int i = 0; i < 2; i++)
> + bool keep_looping = true;
> + do
> {
> - fhandler_base_overlapped::raw_read (in_ptr, len);
> - if (len || i || WaitForSingleObject (read_ready, 0) != WAIT_OBJECT_0)
> - break;
> - /* If we got here, then fhandler_base_overlapped::raw_read returned 0,
> - indicating "EOF" and something has set read_ready to zero. That means
> - we should have a client waiting to connect.
> - FIXME: If the client CTRL-C's the open during this time then this
> - could hang indefinitely. Maybe implement a timeout? */
> - if (!DisconnectNamedPipe (get_io_handle ()))
> + if (nclients == 0) /* Return EOF. */
> {
> - debug_printf ("DisconnectNamedPipe failed, %E");
> - goto errno_out;
> + len = 0;
> + return;
> }
> - else if (!ConnectNamedPipe (get_io_handle (), get_overlapped ())
> - && GetLastError () != ERROR_IO_PENDING)
> + int i = 0;
> + while (i < nclients)
> {
> - debug_printf ("ConnectNamedPipe failed, %E");
> - goto errno_out;
> + size_t orig_len = len;
> + bool was_nonblocking = client[i]->is_nonblocking ();
> + client[i]->set_nonblocking (true);
> + client[i]->raw_read (in_ptr, len);
> + client[i]->set_nonblocking (was_nonblocking);
> + if (len > 0) /* Success. */
> + {
> + keep_looping = false;
> + break;
> + }
> + else if (len < 0 && errno != EAGAIN) /* Unexpected error. */
> + return;
> + else if (len == 0) /* Client no longer connected. */
> + del_client (i);
> + else /* No data available to read. */
> + i++;
> + len = orig_len;
> }
> - else if (!arm (read_ready))
> - goto errno_out;
> - else if (!wait (get_overlapped_buffer ()->hEvent))
> - goto errout; /* If wait() fails, errno is set so no need to set it */
> - len = orig_len; /* Reset since raw_read above set it to zero. */
> + if (is_nonblocking ())
> + keep_looping = false;
> + else
> + yield ();
> }
> + while (keep_looping);
> return;
> -
> -errno_out:
> - __seterrno ();
> -errout:
> - len = -1;
> }
>
> int __reg2
> diff --git a/winsup/cygwin/pipe.cc b/winsup/cygwin/pipe.cc
> index f1eace6a6..94de5f2d0 100644
> --- a/winsup/cygwin/pipe.cc
> +++ b/winsup/cygwin/pipe.cc
> @@ -209,7 +209,7 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
> DWORD
> fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
> DWORD psize, const char *name, DWORD open_mode,
> - int64_t *unique_id)
> + int64_t *unique_id, DWORD max_instances)
> {
> /* Default to error. */
> if (r)
> @@ -274,8 +274,8 @@ fhandler_pipe::create (LPSECURITY_ATTRIBUTES sa_ptr, PHANDLE r, PHANDLE w,
> definitely required for pty handling since fhandler_pty_master
> writes to the pipe in chunks, terminated by newline when CANON mode
> is specified. */
> - *r = CreateNamedPipe (pipename, open_mode, pipe_mode, 1, psize,
> - psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
> + *r = CreateNamedPipe (pipename, open_mode, pipe_mode, max_instances,
> + psize, psize, NMPWAIT_USE_DEFAULT_WAIT, sa_ptr);
>
> if (*r != INVALID_HANDLE_VALUE)
> {
> --
> 2.17.0
>
--
Corinna Vinschen
Cygwin Maintainer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-developers/attachments/20181226/d727f252/attachment.sig>
More information about the Cygwin-developers
mailing list