From 4d6a8663fa0932a8c1b40639464f68af670f0808 Mon Sep 17 00:00:00 2001 From: Tiago Medicci Serrano Date: Sun, 5 Mar 2023 16:45:51 -0300 Subject: [PATCH] drivers/pipe: make pipe and named pipe (mkfifo) POSIX-compliant According to https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html " When opening a FIFO with O_RDONLY or O_WRONLY set: * If O_NONBLOCK is set, an open() for reading-only shall return without delay. An open() for writing-only shall return an error if no process currently has the file open for reading. * If O_NONBLOCK is clear, an open() for reading-only shall block the calling thread until a thread opens the file for writing. An open() for writing-only shall block the calling thread until a thread opens the file for reading. " This commit has an equivalent on nuttx-apps: EXAMPLES_PIPE app was updated to be able to check pipes and named pipes behavior. --- drivers/pipes/pipe.c | 54 ++++++++++++++++---- drivers/pipes/pipe_common.c | 99 +++++++++++++++++++++++++++++++------ drivers/pipes/pipe_common.h | 6 ++- 3 files changed, 133 insertions(+), 26 deletions(-) diff --git a/drivers/pipes/pipe.c b/drivers/pipes/pipe.c index 1993597347..5f62473bca 100644 --- a/drivers/pipes/pipe.c +++ b/drivers/pipes/pipe.c @@ -169,21 +169,22 @@ static int pipe_register(size_t bufsize, int flags, ****************************************************************************/ /**************************************************************************** - * Name: pipe2 + * Name: file_pipe * * Description: - * pipe2() creates a pair of file descriptors, pointing to a pipe inode, - * and places them in the array pointed to by 'fd'. fd[0] is for reading, - * fd[1] is for writing. + * file_pipe() creates a pair of file descriptors, pointing to a pipe + * inode, and places them in the array pointed to by 'filep'. filep[0] + * is for reading, filep[1] is for writing. * * Input Parameters: - * fd[2] - The user provided array in which to catch the pipe file + * filep[2] - The user provided array in which to catch the pipe file * descriptors + * bufsize - The size of the in-memory, circular buffer in bytes. * flags - The file status flags. * * Returned Value: - * 0 is returned on success; -1 (ERROR) is returned on a failure - * with the errno value set appropriately. + * 0 is returned on success; a negated errno value is returned on a + * failure. * ****************************************************************************/ @@ -229,10 +230,30 @@ errout_with_driver: return ret; } +/**************************************************************************** + * Name: pipe2 + * + * Description: + * pipe2() creates a pair of file descriptors, pointing to a pipe inode, + * and places them in the array pointed to by 'fd'. fd[0] is for reading, + * fd[1] is for writing. + * + * Input Parameters: + * fd[2] - The user provided array in which to catch the pipe file + * descriptors + * flags - The file status flags. + * + * Returned Value: + * 0 is returned on success; -1 (ERROR) is returned on a failure + * with the errno value set appropriately. + * + ****************************************************************************/ + int pipe2(int fd[2], int flags) { char devname[32]; int ret; + bool blocking; /* Register a new pipe device */ @@ -243,14 +264,29 @@ int pipe2(int fd[2], int flags) return ERROR; } - /* Get a write file descriptor */ + /* Check for the O_NONBLOCK bit on flags */ - fd[1] = open(devname, O_WRONLY | flags); + blocking = (flags & O_NONBLOCK) == 0; + + /* Get a write file descriptor setting O_NONBLOCK temporarily */ + + fd[1] = open(devname, O_WRONLY | O_NONBLOCK | flags); if (fd[1] < 0) { goto errout_with_driver; } + /* Clear O_NONBLOCK if it was set previously */ + + if (blocking) + { + ret = fcntl(fd[1], F_SETFL, flags & (~O_NONBLOCK)); + if (ret < 0) + { + goto errout_with_driver; + } + } + /* Get a read file descriptor */ fd[0] = open(devname, O_RDONLY | flags); diff --git a/drivers/pipes/pipe_common.c b/drivers/pipes/pipe_common.c index a15504dd8a..f5df000723 100644 --- a/drivers/pipes/pipe_common.c +++ b/drivers/pipes/pipe_common.c @@ -171,8 +171,9 @@ int pipecommon_open(FAR struct file *filep) { dev->d_nwriters++; - /* If this is the first writer, then the read semaphore indicates the - * number of readers waiting for the first writer. Wake them all up. + /* If this is the first writer, then the n-readers semaphore + * indicates the number of readers waiting for the first writer. + * Wake them all up! */ if (dev->d_nwriters == 1) @@ -184,6 +185,51 @@ int pipecommon_open(FAR struct file *filep) } } + while ((filep->f_oflags & O_NONBLOCK) == 0 && /* Blocking */ + (filep->f_oflags & O_RDWR) == O_WRONLY && /* Write-only */ + dev->d_nreaders < 1 && /* No readers on the pipe */ + dev->d_wrndx == dev->d_rdndx) /* Buffer is empty */ + { + /* If opened for write-only, then wait for at least one reader + * on the pipe. + */ + + nxmutex_unlock(&dev->d_bflock); + + /* NOTE: d_wrsem is normally used to check if the write buffer is full + * and wait for it being read and being able to receive more data. But, + * until the first reader has opened the pipe, the meaning is different + * and it is used prevent O_WRONLY open calls from returning until + * there is at least one reader on the pipe. + */ + + ret = nxsem_wait(&dev->d_wrsem); + if (ret < 0) + { + ferr("ERROR: nxsem_wait failed: %d\n", ret); + + /* Immediately close the pipe that we just opened */ + + pipecommon_close(filep); + return ret; + } + + /* The nxmutex_lock() call should fail if we are awakened by a + * signal or if the task is canceled. + */ + + ret = nxmutex_lock(&dev->d_bflock); + if (ret < 0) + { + ferr("ERROR: nxmutex_lock failed: %d\n", ret); + + /* Immediately close the pipe that we just opened */ + + pipecommon_close(filep); + return ret; + } + } + /* If opened for reading, increment the count of reader on on the pipe * instance. */ @@ -191,16 +237,28 @@ int pipecommon_open(FAR struct file *filep) if ((filep->f_oflags & O_RDOK) != 0) { dev->d_nreaders++; + + /* If this is the first reader, then the n-writers semaphore + * indicates the number of writers waiting for the first reader. + * Wake them all up. + */ + + if (dev->d_nreaders == 1) + { + while (nxsem_get_value(&dev->d_wrsem, &sval) == 0 && sval <= 0) + { + nxsem_post(&dev->d_wrsem); + } + } } - while ((filep->f_oflags & O_NONBLOCK) == 0 && /* Non-blocking */ - (filep->f_oflags & O_RDWR) == O_RDONLY && /* Read-only */ - dev->d_nwriters < 1 && /* No writers on the pipe */ - dev->d_wrndx == dev->d_rdndx) /* Buffer is empty */ + while ((filep->f_oflags & O_NONBLOCK) == 0 && /* Blocking */ + (filep->f_oflags & O_RDWR) == O_RDONLY && /* Read-only */ + dev->d_nwriters < 1 && /* No writers on the pipe */ + dev->d_wrndx == dev->d_rdndx) /* Buffer is empty */ { - /* If opened for read-only, then wait for either (1) at least one - * writer on the pipe (policy == 0), or (2) until there is buffered - * data to be read (policy == 1). + /* If opened for read-only, then wait for either at least one writer + * on the pipe. */ nxmutex_unlock(&dev->d_bflock); @@ -215,12 +273,23 @@ int pipecommon_open(FAR struct file *filep) */ ret = nxsem_wait(&dev->d_rdsem); - if (ret < 0 || (ret = nxmutex_lock(&dev->d_bflock)) < 0) + if (ret < 0) { - /* The nxmutex_lock() call should fail if we are awakened by a - * signal or if the task is canceled. - */ + ferr("ERROR: nxsem_wait failed: %d\n", ret); + /* Immediately close the pipe that we just opened */ + + pipecommon_close(filep); + return ret; + } + + /* The nxmutex_lock() call should fail if we are awakened by a + * signal or if the task is canceled. + */ + + ret = nxmutex_lock(&dev->d_bflock); + if (ret < 0) + { ferr("ERROR: nxmutex_lock failed: %d\n", ret); /* Immediately close the pipe that we just opened */ @@ -305,8 +374,8 @@ int pipecommon_close(FAR struct file *filep) poll_notify(dev->d_fds, CONFIG_DEV_PIPE_NPOLLWAITERS, POLLERR); - while (nxsem_get_value(&dev->d_wrsem, &sval) == 0 - && sval <= 0) + while (nxsem_get_value(&dev->d_wrsem, &sval) == 0 && + sval <= 0) { nxsem_post(&dev->d_wrsem); } diff --git a/drivers/pipes/pipe_common.h b/drivers/pipes/pipe_common.h index eec1bac119..c94193e6e4 100644 --- a/drivers/pipes/pipe_common.h +++ b/drivers/pipes/pipe_common.h @@ -115,8 +115,10 @@ typedef uint8_t pipe_ndx_t; /* 8-bit index */ struct pipe_dev_s { mutex_t d_bflock; /* Used to serialize access to d_buffer and indices */ - sem_t d_rdsem; /* Empty buffer - Reader waits for data write */ - sem_t d_wrsem; /* Full buffer - Writer waits for data read */ + sem_t d_rdsem; /* Empty buffer - Reader waits for data write AND + * block O_RDONLY open until there is at least one writer */ + sem_t d_wrsem; /* Full buffer - Writer waits for data read AND + * block O_WRONLY open until there is at least one reader */ pipe_ndx_t d_wrndx; /* Index in d_buffer to save next byte written */ pipe_ndx_t d_rdndx; /* Index in d_buffer to return the next byte read */ pipe_ndx_t d_bufsize; /* allocated size of d_buffer in bytes */