xiaoxiang781216 commented on code in PR #8985:
URL: https://github.com/apache/nuttx/pull/8985#discussion_r1163124679
##########
drivers/pipes/pipe.c:
##########
@@ -243,22 +264,48 @@ int pipe2(int fd[2], int flags)
return ERROR;
}
- /* Get a write file descriptor */
+ /* Check for the O_NONBLOCK bit on flags */
+
+ blocking = (flags & O_NONBLOCK) == 0;
+
+ /* Get a write file descriptor setting O_NONBLOCK temporarily */
- fd[1] = open(devname, O_WRONLY | flags);
+ fd[1] = open(devname, O_WRONLY | O_NONBLOCK | flags);
if (fd[1] < 0)
{
goto errout_with_driver;
}
- /* Get a read file descriptor */
+ /* 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 setting O_NONBLOCK temporarily */
- fd[0] = open(devname, O_RDONLY | flags);
+ fd[0] = open(devname, O_RDONLY | O_NONBLOCK | flags);
Review Comment:
only one end need change to nonblock temporarily?
##########
drivers/pipes/pipe_common.c:
##########
@@ -171,56 +173,109 @@ 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-writers semaphore
+ * indicates the number of readers waiting for the first writer.
+ * Wake them all up!
*/
if (dev->d_nwriters == 1)
{
- while (nxsem_get_value(&dev->d_rdsem, &sval) == 0 && sval <= 0)
+ while (nxsem_get_value(&dev->d_nwrsem, &sval) == OK && sval <= 0)
{
- nxsem_post(&dev->d_rdsem);
+ nxsem_post(&dev->d_nwrsem);
}
}
}
+ while ((filep->f_oflags & O_NONBLOCK) == 0 && /* Blocking */
+ (filep->f_oflags & O_RDWR) == O_WRONLY && /* Write-only */
Review Comment:
move to the if block at line 172 and remove O_WRONLY check
##########
drivers/pipes/pipe_common.c:
##########
@@ -171,56 +173,109 @@ 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-writers semaphore
+ * indicates the number of readers waiting for the first writer.
+ * Wake them all up!
*/
if (dev->d_nwriters == 1)
{
- while (nxsem_get_value(&dev->d_rdsem, &sval) == 0 && sval <= 0)
+ while (nxsem_get_value(&dev->d_nwrsem, &sval) == OK && sval <= 0)
{
- nxsem_post(&dev->d_rdsem);
+ nxsem_post(&dev->d_nwrsem);
}
}
}
+ 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);
+
+ ret = nxsem_wait(&dev->d_nrdsem);
+ 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.
*/
if ((filep->f_oflags & O_RDOK) != 0)
{
dev->d_nreaders++;
+
+ /* If this is the first reader, then the n-readers 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_nrdsem, &sval) == OK && sval <= 0)
+ {
+ nxsem_post(&dev->d_nrdsem);
+ }
+ }
}
- 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 */
Review Comment:
move into if block at line 232 and remove O_RDONLY check
##########
drivers/pipes/pipe_common.c:
##########
@@ -105,6 +105,8 @@ FAR struct pipe_dev_s *pipecommon_allocdev(size_t bufsize)
nxmutex_init(&dev->d_bflock);
nxsem_init(&dev->d_rdsem, 0, 0);
nxsem_init(&dev->d_wrsem, 0, 0);
+ nxsem_init(&dev->d_nrdsem, 0, 0);
+ nxsem_init(&dev->d_nwrsem, 0, 0);
Review Comment:
why not to reuse d_wrsem?
##########
drivers/pipes/pipe_common.c:
##########
@@ -284,7 +339,8 @@ int pipecommon_close(FAR struct file *filep)
poll_notify(dev->d_fds, CONFIG_DEV_PIPE_NPOLLWAITERS, POLLHUP);
- while (nxsem_get_value(&dev->d_rdsem, &sval) == 0 && sval <= 0)
+ while (nxsem_get_value(&dev->d_rdsem, &sval) == OK &&
Review Comment:
lete's revert all the unrelated style change. If you really want to fix it,
provide a new patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
