system/popen: Avoid copying FILE

system/popen/popen.c

@@ -45,17 +45,76 @@
  * Private Types
  ****************************************************************************/
 
-/* struct popen_file_s is a cast compatible version of FILE that contains
- * the additional PID of the shell processes needed by pclose().
- */
-
 struct popen_file_s
 {
-  FILE copy;
-  FILE *original;
+  int fd;
   pid_t shell;
 };
 
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: popen_file_read
+ ****************************************************************************/
+
+static ssize_t popen_file_read(FAR void *cookie, FAR char *buf,
+                               size_t size)
+{
+  FAR struct popen_file_s *filep = (FAR struct popen_file_s *)cookie;
+
+  return read(filep->fd, buf, size);
+}
+
+/****************************************************************************
+ * Name: popen_file_write
+ ****************************************************************************/
+
+static ssize_t popen_file_write(FAR void *cookie, FAR const char *buf,
+                                size_t size)
+{
+  FAR struct popen_file_s *filep = (FAR struct popen_file_s *)cookie;
+
+  return write(filep->fd, buf, size);
+}
+
+/****************************************************************************
+ * Name: popen_file_seek
+ ****************************************************************************/
+
+static off_t popen_file_seek(FAR void *cookie, FAR off_t *offset,
+                             int whence)
+{
+  set_errno(ESPIPE);
+  return ERROR;
+}
+
+/****************************************************************************
+ * Name: popen_file_close
+ ****************************************************************************/
+
+static int popen_file_close(FAR void *cookie)
+{
+  FAR struct popen_file_s *filep = (FAR struct popen_file_s *)cookie;
+  int ret;
+#ifdef CONFIG_SCHED_WAITPID
+  int status;
+#endif
+
+  ret = close(filep->fd);
+
+#ifdef CONFIG_SCHED_WAITPID
+  if (filep->shell > 0 && waitpid(filep->shell, &status, 0) < 0)
+    {
+      ret = ERROR;
+    }
+#endif
+
+  free(filep);
+  return ret;
+}
+
 /****************************************************************************
  * Public Functions
  ****************************************************************************/
@@ -115,6 +174,15 @@ struct popen_file_s
 FILE *popen(FAR const char *command, FAR const char *mode)
 {
   FAR struct popen_file_s *container;
+  FAR FILE *stream;
+  cookie_io_functions_t popen_io =
+    {
+      .read  = popen_file_read,
+      .write = popen_file_write,
+      .seek  = popen_file_seek,
+      .close = popen_file_close
+    };
+
   struct sched_param param;
   posix_spawnattr_t attr;
   posix_spawn_file_actions_t file_actions;
@@ -136,6 +204,8 @@ FILE *popen(FAR const char *command, FAR const char *mode)
       goto errout;
     }
 
+  container->shell = ERROR;
+
   oldfd[1] = 0;
   newfd[1] = 0;
 
@@ -188,13 +258,14 @@ FILE *popen(FAR const char *command, FAR const char *mode)
   else
     {
       errcode = EINVAL;
-      goto errout_with_pipe;
+      goto errout_with_container;
     }
 
   /* Create the FILE stream return reference */
 
-  container->original = fdopen(retfd, mode);
-  if (container->original == NULL)
+  container->fd = retfd;
+  stream = fopencookie(container, mode, popen_io);
+  if (stream == NULL)
     {
       errcode = errno;
       goto errout_with_pipe;
@@ -329,10 +400,7 @@ FILE *popen(FAR const char *command, FAR const char *mode)
       ioctl(retfd, FIOCLEX, 0);
     }
 
-  /* Finale and return input input/output stream */
-
-  memcpy(&container->copy, container->original, sizeof(FILE));
-  return &container->copy;
+  return stream;
 
 errout_with_actions:
   posix_spawn_file_actions_destroy(&file_actions);
@@ -341,7 +409,17 @@ FILE *popen(FAR const char *command, FAR const char *mode)
   posix_spawnattr_destroy(&attr);
 
 errout_with_stream:
-  fclose(container->original);
+  fclose(stream);
+
+  /* fclose(stream) closes retfd.  Close the child-side descriptors too. */
+
+  close(newfd[0]);
+  if (rw && newfd[0] != newfd[1])
+    {
+      close(newfd[1]);
+    }
+
+  goto errout;
 
 errout_with_pipe:
   close(fd[0]);
@@ -399,45 +477,5 @@ FILE *popen(FAR const char *command, FAR const char *mode)
 
 int pclose(FILE *stream)
 {
-  FAR struct popen_file_s *container = (FAR struct popen_file_s *)stream;
-  FILE *original;
-  pid_t shell;
-#ifdef CONFIG_SCHED_WAITPID
-  int status;
-  int result;
-#endif
-
-  DEBUGASSERT(container != NULL && container->original != NULL);
-  original = container->original;
-
-  /* Set the state of the original file descriptor to the state of the
-   * working copy
-   */
-
-  memcpy(original, &container->copy, sizeof(FILE));
-
-  /* Then close the original and free the container (saving the PID of the
-   * shell process)
-   */
-
-  fclose(original);
-
-  shell = container->shell;
-  free(container);
-
-#ifdef CONFIG_SCHED_WAITPID
-  /* Wait for the shell to exit, retrieving the return value if available. */
-
-  result = waitpid(shell, &status, 0);
-  if (result < 0)
-    {
-      /* The errno has already been set */
-
-      return ERROR;
-    }
-
-  return status;
-#else
-  return OK;
-#endif
+  return fclose(stream);
 }

testing/libc/popen/CMakeLists.txt

@@ -0,0 +1,33 @@
+# ##############################################################################
+# apps/testing/libc/popen/CMakeLists.txt
+#
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more contributor
+# license agreements.  See the NOTICE file distributed with this work for
+# additional information regarding copyright ownership.  The ASF licenses this
+# file to you under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License.  You may obtain a copy of
+# the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations under
+# the License.
+#
+# ##############################################################################
+
+if(CONFIG_TESTING_POPEN_TEST)
+  nuttx_add_application(
+    NAME
+    popen_test
+    STACKSIZE
+    4096
+    MODULE
+    ${CONFIG_TESTING_POPEN_TEST}
+    SRCS
+    popen_test.c)
+endif()

testing/libc/popen/Kconfig

@@ -0,0 +1,12 @@
+#
+# For a description of the syntax of this configuration file,
+# see the file kconfig-language.txt in the NuttX tools repository.
+#
+
+config TESTING_POPEN_TEST
+	tristate "popen() test"
+	default n
+	depends on SYSTEM_POPEN
+	depends on !NSH_DISABLE_ECHO
+	---help---
+		Test reading from popen() streams and closing them with pclose().

testing/libc/popen/Make.defs

@@ -0,0 +1,25 @@
+############################################################################
+# apps/testing/libc/popen/Make.defs
+#
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+ifneq ($(CONFIG_TESTING_POPEN_TEST),)
+CONFIGURED_APPS += $(APPDIR)/testing/libc/popen
+endif

testing/libc/popen/Makefile

@@ -0,0 +1,34 @@
+############################################################################
+# apps/testing/libc/popen/Makefile
+#
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+############################################################################
+
+include $(APPDIR)/Make.defs
+
+# popen() test tool
+
+PROGNAME = popen_test
+PRIORITY = SCHED_PRIORITY_DEFAULT
+STACKSIZE = 4096
+MODULE = $(CONFIG_TESTING_POPEN_TEST)
+
+MAINSRC = popen_test.c
+
+include $(APPDIR)/Application.mk