From fefe83981884642c1eb1fefde4be47a6e67ba98b Mon Sep 17 00:00:00 2001 From: patacongo Date: Wed, 2 Mar 2011 00:33:42 +0000 Subject: [PATCH] Fix pipe/fifo open logic: semaphore wait in open() must abort if a signal is received git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@3327 42af7a65-404d-4744-a932-0658087f49c3 --- ChangeLog | 5 ++ Documentation/NuttX.html | 6 +- drivers/pipes/pipe_common.c | 114 +++++++++++++++++++++--------------- examples/nsh/nsh_main.c | 2 +- fs/fs_open.c | 8 ++- 5 files changed, 85 insertions(+), 50 deletions(-) diff --git a/ChangeLog b/ChangeLog index c9be40c5d7..23520da45d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1499,3 +1499,8 @@ nsh> cat test.txt This is a test + + * drvers/pipes/pipe_common.c: Driver open method eas not returning an EINTR + error when it received a signal. Instead, it just re-started the wait. This + makes it impossible to kill a background pipe operation from NSH. + diff --git a/Documentation/NuttX.html b/Documentation/NuttX.html index 15ee60dae8..44fc71a9ea 100644 --- a/Documentation/NuttX.html +++ b/Documentation/NuttX.html @@ -8,7 +8,7 @@

NuttX RTOS

-

Last Updated: February 28, 2011

+

Last Updated: March 1, 2011

@@ -2102,6 +2102,10 @@ nuttx-5.19 2011-xx-xx Gregory Nutt <spudmonkey@racsa.co.cr> nsh> cat test.txt This is a test + * drvers/pipes/pipe_common.c: Driver open method eas not returning an EINTR + error when it received a signal. Instead, it just re-started the wait. This + makes it impossible to kill a background pipe operation from NSH. + pascal-2.1 2011-xx-xx Gregory Nutt <spudmonkey@racsa.co.cr> buildroot-1.10 2011-xx-xx diff --git a/drivers/pipes/pipe_common.c b/drivers/pipes/pipe_common.c index e22678688b..a6af41fe19 100644 --- a/drivers/pipes/pipe_common.c +++ b/drivers/pipes/pipe_common.c @@ -1,7 +1,7 @@ /**************************************************************************** * drivers/pipes/pipe_common.c * - * Copyright (C) 2008-2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2008-2009, 2011 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -184,6 +184,7 @@ int pipecommon_open(FAR struct file *filep) struct inode *inode = filep->f_inode; struct pipe_dev_s *dev = inode->i_private; int sval; + int ret; /* Some sanity checking */ #if CONFIG_DEBUG @@ -192,66 +193,87 @@ int pipecommon_open(FAR struct file *filep) return -EBADF; } #endif - /* Make sure that we have exclusive access to the device structure */ + /* Make sure that we have exclusive access to the device structure. The + * sem_wait() call should fail only if we are awakened by a signal. + */ - if (sem_wait(&dev->d_bfsem) == 0) + ret = sem_wait(&dev->d_bfsem); + if (ret != OK) { - /* If this the first reference on the device, then allocate the buffer */ + fdbg("sem_wait failed: %d\n", errno); + DEBUGASSERT(errno > 0); + return -errno; + } - if (dev->d_refs == 0) + /* If this the first reference on the device, then allocate the buffer */ + + if (dev->d_refs == 0) + { + dev->d_buffer = (uint8_t*)malloc(CONFIG_DEV_PIPE_SIZE); + if (!dev->d_buffer) { - dev->d_buffer = (uint8_t*)malloc(CONFIG_DEV_PIPE_SIZE); - if (!dev->d_buffer) + (void)sem_post(&dev->d_bfsem); + return -ENOMEM; + } + } + + /* Increment the reference count on the pipe instance */ + + dev->d_refs++; + + /* If opened for writing, increment the count of writers on on the pipe instance */ + + if ((filep->f_oflags & O_WROK) != 0) + { + dev->d_nwriters++; + + /* If this this is the first writer, then the read semaphore indicates the + * number of readers waiting for the first writer. Wake them all up. + */ + + if (dev->d_nwriters == 1) + { + while (sem_getvalue(&dev->d_rdsem, &sval) == 0 && sval < 0) { - (void)sem_post(&dev->d_bfsem); - return -ENOMEM; + sem_post(&dev->d_rdsem); } } + } - /* Increment the reference count on the pipe instance */ + /* If opened for read-only, then wait for at least one writer on the pipe */ - dev->d_refs++; + sched_lock(); + (void)sem_post(&dev->d_bfsem); + if ((filep->f_oflags & O_RDWR) == O_RDONLY && dev->d_nwriters < 1) + { + /* NOTE: d_rdsem is normally used when the read logic waits for more + * data to be written. But until the first writer has opened the + * pipe, the meaning is different: it is used prevent O_RDONLY open + * calls from returning until there is at least one writer on the pipe. + * This is required both by spec and also because it prevents + * subsequent read() calls from returning end-of-file because there is + * no writer on the pipe. + */ - /* If opened for writing, increment the count of writers on on the pipe instance */ - - if ((filep->f_oflags & O_WROK) != 0) + ret = sem_wait(&dev->d_rdsem); + if (ret != OK) { - dev->d_nwriters++; - - /* If this this is the first writer, then the read semaphore indicates the - * number of readers waiting for the first writer. Wake them all up. + /* The sem_wait() call should fail only if we are awakened by + * a signal. */ - if (dev->d_nwriters == 1) - { - while (sem_getvalue(&dev->d_rdsem, &sval) == 0 && sval < 0) - { - sem_post(&dev->d_rdsem); - } - } + fdbg("sem_wait failed: %d\n", errno); + DEBUGASSERT(errno > 0); + ret = -errno; + + /* Immediately close the pipe that we just opened */ + + (void)pipecommon_close(filep); } + } - /* If opened for read-only, then wait for at least one writer on the pipe */ - - sched_lock(); - (void)sem_post(&dev->d_bfsem); - if ((filep->f_oflags & O_RDWR) == O_RDONLY && dev->d_nwriters < 1) - { - /* NOTE: d_rdsem is normally used when the read logic waits for more - * data to be written. But until the first writer has opened the - * pipe, the meaning is different: it is used prevent O_RDONLY open - * calls from returning until there is at least one writer on the pipe. - * This is required both by spec and also because it prevents - * subsequent read() calls from returning end-of-file because there is - * no writer on the pipe. - */ - - pipecommon_semtake(&dev->d_rdsem); - } - sched_unlock(); - return OK; - } - return ERROR; + sched_unlock(); + return ret; } /**************************************************************************** diff --git a/examples/nsh/nsh_main.c b/examples/nsh/nsh_main.c index aea823be86..9981ad0733 100644 --- a/examples/nsh/nsh_main.c +++ b/examples/nsh/nsh_main.c @@ -360,7 +360,7 @@ const char g_fmtcmdfailed[] = "nsh: %s: %s failed: %d\n"; const char g_fmtcmdoutofmemory[] = "nsh: %s: out of memory\n"; const char g_fmtinternalerror[] = "nsh: %s: Internal error\n"; #ifndef CONFIG_DISABLE_SIGNALS -const char g_fmtsignalrecvd[] = "nsh: %s: Signal received\n"; +const char g_fmtsignalrecvd[] = "nsh: %s: Interrupted by signal\n"; #endif /**************************************************************************** diff --git a/fs/fs_open.c b/fs/fs_open.c index 76b941f959..467eef858e 100644 --- a/fs/fs_open.c +++ b/fs/fs_open.c @@ -1,7 +1,7 @@ /**************************************************************************** * fs_open.c * - * Copyright (C) 2007, 2008, 2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2011 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -69,6 +69,10 @@ int inode_checkflags(FAR struct inode *inode, int oflags) } } +/**************************************************************************** + * Name: open + ****************************************************************************/ + int open(const char *path, int oflags, ...) { struct filelist *list; @@ -181,7 +185,7 @@ int open(const char *path, int oflags, ...) errout_with_inode: inode_release(inode); errout: - *get_errno_ptr() = ret; + errno = ret; return ERROR; }