[PATCH] libmisc/shell: Fix the handling of joel scripts in telnet

chrisj at rtems.org chrisj at rtems.org
Mon Feb 17 06:05:13 UTC 2020


From: Chris Johns <chrisj at rtems.org>

- Fix the passing of std[in/out] to child threads
- Fix deleting of managed memory in the key destructor
- Only set the key in the main loop thread
- Only allocate a shell env outside of the main loop
- Work around #3970
- Fix memory leak if the task start fails
- Remove error level from shell env, it cannot be returned this way. Add
  exit_code but the API is broken so it cannot be returned.

Closes #3859
---
 cpukit/include/rtems/shell.h       |   9 +-
 cpukit/libmisc/shell/shell.c       | 212 ++++++++++++++++++++---------
 testsuites/libtests/shell01/init.c | 121 +++++++++++++++-
 3 files changed, 270 insertions(+), 72 deletions(-)

diff --git a/cpukit/include/rtems/shell.h b/cpukit/include/rtems/shell.h
index 973db16605..71fc1dd8c7 100644
--- a/cpukit/include/rtems/shell.h
+++ b/cpukit/include/rtems/shell.h
@@ -217,19 +217,24 @@ extern rtems_status_code rtems_shell_script(
 /**
  *  Private environment associated with each shell instance.
  */
-typedef struct {
+typedef struct rtems_shell_env {
   /** 'S','E','N','V': Shell Environment */
   rtems_name magic;
+  bool managed;
   const char *devname;
   const char *taskname;
   bool exit_shell; /* logout */
   bool forever; /* repeat login */
-  int errorlevel;
+  int *exit_code;
+  bool exit_on_error;
   bool echo;
   char cwd[256];
   const char *input;
   const char *output;
   bool output_append;
+  FILE *parent_stdin;
+  FILE *parent_stdout;
+  FILE *parent_stderr;
   rtems_id wake_on_end;
   rtems_shell_login_check_t login_check;
 
diff --git a/cpukit/libmisc/shell/shell.c b/cpukit/libmisc/shell/shell.c
index c2ea3c4afa..208ea7cca6 100644
--- a/cpukit/libmisc/shell/shell.c
+++ b/cpukit/libmisc/shell/shell.c
@@ -20,6 +20,7 @@
 #include <time.h>
 
 #include <rtems.h>
+#include <rtems/error.h>
 #include <rtems/libio.h>
 #include <rtems/libio_.h>
 #include <rtems/shell.h>
@@ -38,20 +39,35 @@
 #include <pthread.h>
 #include <assert.h>
 
+#define SHELL_STD_DEBUG 0
+
+#if SHELL_STD_DEBUG
+static FILE* default_stdout;
+#define shell_std_debug(...) fprintf(default_stdout, __VA_ARGS__)
+#else
+#define shell_std_debug(...)
+#endif
+
 const rtems_shell_env_t rtems_global_shell_env = {
   .magic         = rtems_build_name('S', 'E', 'N', 'V'),
+  .managed       = false,
   .devname       = CONSOLE_DEVICE_NAME,
   .taskname      = "SHGL",
   .exit_shell    = false,
   .forever       = true,
-  .errorlevel    = -1,
   .echo          = false,
   .cwd           = "/",
   .input         = NULL,
   .output        = NULL,
   .output_append = false,
+  .parent_stdin  = NULL,
+  .parent_stdout = NULL,
+  .parent_stderr = NULL,
   .wake_on_end   = RTEMS_ID_NONE,
-  .login_check   = NULL
+  .exit_code     = NULL,
+  .login_check   = NULL,
+  .uid           = 0,
+  .gid           = 0
 };
 
 static pthread_once_t rtems_shell_once = PTHREAD_ONCE_INIT;
@@ -62,7 +78,7 @@ static pthread_key_t rtems_shell_current_env_key;
  *  Initialize the shell user/process environment information
  */
 static rtems_shell_env_t *rtems_shell_init_env(
-  rtems_shell_env_t *shell_env_p
+  rtems_shell_env_t *shell_env_parent
 )
 {
   rtems_shell_env_t *shell_env;
@@ -70,12 +86,17 @@ static rtems_shell_env_t *rtems_shell_init_env(
   shell_env = malloc(sizeof(rtems_shell_env_t));
   if ( !shell_env )
     return NULL;
-  if ( !shell_env_p ) {
+
+  if ( shell_env_parent == NULL ) {
+    shell_env_parent = rtems_shell_get_current_env();
+  }
+  if ( shell_env_parent == NULL ) {
     *shell_env = rtems_global_shell_env;
-    shell_env->taskname = NULL;
   } else {
-    *shell_env = *shell_env_p;
+    *shell_env = *shell_env_parent;
   }
+  shell_env->managed = true;
+  shell_env->taskname = NULL;
 
   return shell_env;
 }
@@ -90,14 +111,16 @@ static void rtems_shell_env_free(
   rtems_shell_env_t *shell_env;
   shell_env = (rtems_shell_env_t *) ptr;
 
-  if ( !ptr )
+  if ( ptr == NULL )
     return;
 
-  if ( shell_env->input )
-    free((void *)shell_env->input);
-  if ( shell_env->output )
-    free((void *)shell_env->output);
-  free( ptr );
+  if ( shell_env->managed ) {
+    if ( shell_env->input )
+      free((void *)shell_env->input);
+    if ( shell_env->output )
+      free((void *)shell_env->output);
+    free( ptr );
+  }
 }
 
 static void rtems_shell_create_file(const char *name, const char *content)
@@ -129,6 +152,10 @@ static void rtems_shell_init_once(void)
   struct passwd pwd;
   struct passwd *pwd_res;
 
+#if SHELL_STD_DEBUG
+  default_stdout = stdout;
+#endif
+
   pthread_key_create(&rtems_shell_current_env_key, rtems_shell_env_free);
 
   /* dummy call to init /etc dir */
@@ -168,15 +195,22 @@ rtems_shell_env_t *rtems_shell_get_current_env(void)
 void rtems_shell_dup_current_env(rtems_shell_env_t *copy)
 {
   rtems_shell_env_t *env = rtems_shell_get_current_env();
-  if (env) {
+  if (env != NULL) {
     *copy = *env;
   }
   else {
-    memset(copy, 0, sizeof(rtems_shell_env_t));
-    copy->magic    = rtems_build_name('S', 'E', 'N', 'V');
-    copy->devname  = CONSOLE_DEVICE_NAME;
-    copy->taskname = "RTSH";
+    *copy = rtems_global_shell_env;
+    copy->magic         = rtems_build_name('S', 'E', 'N', 'V');
+    copy->devname       = CONSOLE_DEVICE_NAME;
+    copy->taskname      = "RTSH";
+    copy->parent_stdout = stdout;
+    copy->parent_stdin  = stdin;
+    copy->parent_stderr = stderr;
   }
+  /*
+   * Duplicated environments are not managed.
+   */
+  copy->managed = false;
 }
 
 /*
@@ -704,10 +738,9 @@ static bool rtems_shell_init_user_env(void)
 #define RTEMS_SHELL_PROMPT_SIZE       (128)
 
 bool rtems_shell_main_loop(
-  rtems_shell_env_t *shell_env_arg
+  rtems_shell_env_t *shell_env
 )
 {
-  rtems_shell_env_t *shell_env;
   int                eno;
   struct termios     term;
   struct termios     previous_term;
@@ -726,37 +759,47 @@ bool rtems_shell_main_loop(
 
   rtems_shell_init_environment();
 
-  shell_env = rtems_shell_init_env(shell_env_arg);
-  if (shell_env == NULL) {
+  if (shell_env->magic != rtems_build_name('S', 'E', 'N', 'V')) {
+    rtems_error(0, "invalid shell environment passed to the main loop)");
     return false;
   }
 
   eno = pthread_setspecific(rtems_shell_current_env_key, shell_env);
   if (eno != 0) {
+    rtems_error(0, "pthread_setspecific(shell_current_env_key)");
     return false;
   }
 
   if (!rtems_shell_init_user_env()) {
+    rtems_error(0, "rtems_shell_init_user_env");
     return false;
   }
 
-  fileno(stdout);
-
-  /* fprintf( stderr,
-     "-%s-%s-\n", shell_env->input, shell_env->output );
-  */
-
-  if (shell_env->output && strcmp(shell_env->output, "stdout") != 0) {
-    if (strcmp(shell_env->output, "stderr") == 0) {
-      stdout = stderr;
+  shell_std_debug("shell: out: %d %d %s\n",
+                  fileno(stdout), fileno(shell_env->parent_stdout),
+                  shell_env->output);
+  shell_std_debug("     :  in: %d %d %s\n",
+                  fileno(stdin), fileno(shell_env->parent_stdin),
+                  shell_env->input);
+
+  if (shell_env->output != NULL) {
+    if (strcmp(shell_env->output, "stdout") == 0) {
+      if (shell_env->parent_stdout != NULL)
+        stdout = shell_env->parent_stdout;
+    }
+    else if (strcmp(shell_env->output, "stderr") == 0) {
+      if (shell_env->parent_stderr != NULL)
+        stdout = shell_env->parent_stderr;
+      else
+        stdout = stderr;
     } else if (strcmp(shell_env->output, "/dev/null") == 0) {
       fclose (stdout);
     } else {
-      FILE *output = fopen(shell_env_arg->output,
-                           shell_env_arg->output_append ? "a" : "w");
-      if (!output) {
+      FILE *output = fopen(shell_env->output,
+                           shell_env->output_append ? "a" : "w");
+      if (output == NULL) {
         fprintf(stderr, "shell: open output %s failed: %s\n",
-                shell_env_arg->output, strerror(errno));
+                shell_env->output, strerror(errno));
         return false;
       }
       stdout = output;
@@ -764,27 +807,35 @@ bool rtems_shell_main_loop(
     }
   }
 
-  if (shell_env->input && strcmp(shell_env_arg->input, "stdin") != 0) {
-    FILE *input = fopen(shell_env_arg->input, "r");
-    if (!input) {
-      fprintf(stderr, "shell: open input %s failed: %s\n",
-              shell_env_arg->input, strerror(errno));
-      return false;
+  if (shell_env->input != NULL) {
+    if (strcmp(shell_env->input, "stdin") == 0) {
+      if (shell_env->parent_stdin != NULL)
+        stdin = shell_env->parent_stdin;
+    } else {
+      FILE *input = fopen(shell_env->input, "r");
+      if (input == NULL) {
+        fprintf(stderr, "shell: open input %s failed: %s\n",
+                shell_env->input, strerror(errno));
+        if (stdoutToClose != NULL)
+          fclose(stdoutToClose);
+        return false;
+      }
+      stdin = input;
+      stdinToClose = input;
+      shell_env->forever = false;
+      input_file = true;
     }
-    stdin = input;
-    stdinToClose = input;
-    shell_env->forever = false;
-    input_file =true;
   }
-  else {
-    /* make a raw terminal,Linux Manuals */
+
+  if (!input_file) {
+    /* Make a raw terminal, Linux Manuals */
     if (tcgetattr(fileno(stdin), &previous_term) >= 0) {
       term = previous_term;
       term.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP|INLCR|IGNCR|ICRNL|IXON);
       term.c_oflag &= ~OPOST;
       term.c_oflag |= (OPOST|ONLCR); /* But with cr+nl on output */
       term.c_lflag &= ~(ECHO|ECHONL|ICANON|ISIG|IEXTEN);
-      term.c_cflag  |= CLOCAL | CREAD;
+      term.c_cflag |= CLOCAL | CREAD;
       term.c_cc[VMIN]  = 1;
       term.c_cc[VTIME] = 0;
       if (tcsetattr (fileno(stdin), TCSADRAIN, &term) < 0) {
@@ -799,8 +850,13 @@ bool rtems_shell_main_loop(
                 "shell:cannot allocate prompt memory\n");
   }
 
-  setvbuf(stdin,NULL,_IONBF,0); /* Not buffered*/
-  setvbuf(stdout,NULL,_IONBF,0); /* Not buffered*/
+  shell_std_debug("shell: child out: %d in %d\n", fileno(stdout), fileno(stdin));
+  shell_std_debug("shell: child out: %p\n", stdout);
+
+   /* Do not buffer if interactive else leave buffered */
+  if (!input_file)
+    setvbuf(stdin, NULL, _IONBF, 0);
+  setvbuf(stdout, NULL, _IONBF, 0);
 
   /*
    * Allocate the command line buffers.
@@ -863,8 +919,6 @@ bool rtems_shell_main_loop(
         shell_env->exit_shell = false;
 
         for (;;) {
-          int cmd;
-
           /* Prompt section */
           if (prompt) {
             rtems_shell_get_prompt(shell_env, prompt,
@@ -918,7 +972,11 @@ bool rtems_shell_main_loop(
           memcpy (cmd_argv, cmds[cmd], RTEMS_SHELL_CMD_SIZE);
           if (!rtems_shell_make_args(cmd_argv, &argc, argv,
                                      RTEMS_SHELL_MAXIMUM_ARGUMENTS)) {
-            shell_env->errorlevel = rtems_shell_execute_cmd(argv[0], argc, argv);
+            int exit_code = rtems_shell_execute_cmd(argv[0], argc, argv);
+            if (shell_env->exit_code != NULL)
+              *shell_env->exit_code = exit_code;
+            if (shell_env->exit_on_error)
+              shell_env->exit_shell = true;
           }
 
           /* end exec cmd section */
@@ -968,6 +1026,7 @@ static rtems_status_code rtems_shell_run (
   const char *output,
   bool output_append,
   rtems_id wake_on_end,
+  int *exit_code,
   bool echo,
   rtems_shell_login_check_t login_check
 )
@@ -977,6 +1036,8 @@ static rtems_status_code rtems_shell_run (
   rtems_shell_env_t *shell_env;
   rtems_name         name;
 
+  rtems_shell_init_environment();
+
   if ( task_name && strlen(task_name) >= 4)
     name = rtems_build_name(
       task_name[0], task_name[1], task_name[2], task_name[3]);
@@ -992,22 +1053,31 @@ static rtems_status_code rtems_shell_run (
     &task_id
   );
   if (sc != RTEMS_SUCCESSFUL) {
+    rtems_error(sc,"creating task %s in shell_init()",task_name);
     return sc;
   }
 
   shell_env = rtems_shell_init_env( NULL );
   if ( !shell_env )  {
+   rtems_error(RTEMS_NO_MEMORY,
+               "allocating shell_env %s in shell_init()",task_name);
    return RTEMS_NO_MEMORY;
   }
+
   shell_env->devname       = devname;
   shell_env->taskname      = task_name;
+
   shell_env->exit_shell    = false;
   shell_env->forever       = forever;
   shell_env->echo          = echo;
-  shell_env->input         = strdup (input);
-  shell_env->output        = strdup (output);
+  shell_env->input         = input == NULL ? NULL : strdup (input);
+  shell_env->output        = output == NULL ? NULL : strdup (output);
   shell_env->output_append = output_append;
+  shell_env->parent_stdin  = stdin;
+  shell_env->parent_stdout = stdout;
+  shell_env->parent_stderr = stderr;
   shell_env->wake_on_end   = wake_on_end;
+  shell_env->exit_code     = exit_code;
   shell_env->login_check   = login_check;
   shell_env->uid           = getuid();
   shell_env->gid           = getgid();
@@ -1015,8 +1085,11 @@ static rtems_status_code rtems_shell_run (
   getcwd(shell_env->cwd, sizeof(shell_env->cwd));
 
   sc = rtems_task_start(task_id, rtems_shell_task,
-                          (rtems_task_argument) shell_env);
+                        (rtems_task_argument) shell_env);
   if (sc != RTEMS_SUCCESSFUL) {
+    free( (void*) shell_env->input );
+    free( (void*) shell_env->output );
+    free( shell_env );
     return sc;
   }
 
@@ -1025,7 +1098,7 @@ static rtems_status_code rtems_shell_run (
     sc = rtems_event_receive (RTEMS_EVENT_1, RTEMS_WAIT, 0, &out);
   }
 
-  return 0;
+  return sc;
 }
 
 rtems_status_code rtems_shell_init(
@@ -1039,6 +1112,7 @@ rtems_status_code rtems_shell_init(
 )
 {
   rtems_id to_wake = RTEMS_ID_NONE;
+  int exit_code = 0;
 
   if ( wait )
     to_wake = rtems_task_self();
@@ -1054,12 +1128,13 @@ rtems_status_code rtems_shell_init(
     "stdout",                /* output */
     false,                   /* output_append */
     to_wake,                 /* wake_on_end */
+    &exit_code,              /* exit code of command */
     false,                   /* echo */
     login_check              /* login check */
   );
 }
 
-rtems_status_code   rtems_shell_script (
+rtems_status_code rtems_shell_script (
   const char          *task_name,
   size_t               task_stacksize,
   rtems_task_priority  task_priority,
@@ -1070,16 +1145,14 @@ rtems_status_code   rtems_shell_script (
   bool                 echo
 )
 {
-  rtems_id          current_task = RTEMS_INVALID_ID;
+  rtems_id to_wake = RTEMS_ID_NONE;
+  int exit_code = 0;
   rtems_status_code sc;
 
-  if (wait) {
-    sc = rtems_task_ident (RTEMS_SELF, RTEMS_LOCAL, &current_task);
-    if (sc != RTEMS_SUCCESSFUL)
-      return sc;
-  }
+  if ( wait )
+    to_wake = rtems_task_self();
 
-  sc = rtems_shell_run(
+  return rtems_shell_run(
     task_name,       /* task_name */
     task_stacksize,  /* task_stacksize */
     task_priority,   /* task_priority */
@@ -1089,12 +1162,17 @@ rtems_status_code   rtems_shell_script (
     input,           /* input */
     output,          /* output */
     output_append,   /* output_append */
-    current_task,    /* wake_on_end */
+    to_wake,         /* wake_on_end */
+    &exit_code,      /* exit_code */
     echo,            /* echo */
     NULL             /* login check */
   );
-  if (sc != RTEMS_SUCCESSFUL)
-    return sc;
+
+  if (sc == RTEMS_SUCCESSFUL)
+  {
+    /* Place holder until RTEMS 5 is released then the interface for
+     * this call will change. */
+  }
 
   return sc;
 }
diff --git a/testsuites/libtests/shell01/init.c b/testsuites/libtests/shell01/init.c
index 545d695d86..f3ef993162 100644
--- a/testsuites/libtests/shell01/init.c
+++ b/testsuites/libtests/shell01/init.c
@@ -50,6 +50,120 @@ static const char etc_group[] =
   "E::7:y,z\n"
   "F::8:s,moop,t\n";
 
+static const char joel_in[] =
+  "#! joel\n"
+  "jtst hello world\n"
+  "jtst 1 2 3 4 5\n";
+
+static const char joel_out_1[] =
+  " 3 'jtst hello world'\n"
+  " 6 'jtst 1 2 3 4 5'\n";
+
+static const char joel_out_2[] =
+  "\n"
+  "RTEMS Shell on (null). Use 'help' to list commands.\n"
+  " 3 'jtst hello world'\n"
+  " 6 'jtst 1 2 3 4 5'\n";
+
+static int joel_test_command(int argc, char** argv)
+{
+  int i;
+  fprintf(stdout, "%2d '", argc);
+  for (i = 0; i < argc; ++i) {
+    fprintf(stdout, argv[i]);
+    if (i < (argc - 1))
+      fprintf(stdout, " ");
+  }
+  fprintf(stdout, "'\n");
+  return 0;
+}
+
+static void test_joel(void)
+{
+  /*
+   * Running a joel script tests the shell main loop.
+   */
+  char buf[sizeof(joel_out_2) + 1];
+  rtems_shell_cmd_t* cmd;
+  FILE *in;
+  FILE *out;
+  FILE *current_stdout = stdout;
+  FILE *current_stdin = stdin;
+  size_t len;
+  rtems_status_code sc;
+
+  /*
+   * Use a private command due to the way the testsuite maps printk onto printf.
+   */
+  cmd = rtems_shell_add_cmd("jtst", "misc", "joel test", joel_test_command);
+  rtems_test_assert(cmd != NULL);
+
+  len = strlen(joel_in);
+
+  in = fopen("/jin", "w");
+  rtems_test_assert(in != NULL);
+  rtems_test_assert(fwrite(joel_in, sizeof(char), len, in) == len);
+  rtems_test_assert(fclose(in) == 0);
+
+  /*
+   * The shell opens the input and output files.
+   */
+  sc = rtems_shell_script(
+    "JTST",
+    8 * 1024,
+    1,
+    "/jin",
+    "/jout",
+    false,
+    true,
+    false);
+  rtems_test_assert(sc == RTEMS_SUCCESSFUL);
+
+  out = fopen("/jout", "r");
+  rtems_test_assert(out != NULL);
+  rtems_test_assert(!feof(out));
+  len = fread(buf, sizeof(char), sizeof(buf), out);
+  rtems_test_assert(len > 0);
+  rtems_test_assert(strcmp(joel_out_1, buf) == 0);
+  rtems_test_assert(fclose(out) == 0);
+
+  /*
+   * The shell task inherits the parent stdin and stdout
+   */
+  in = fopen("/jin", "r");
+  rtems_test_assert(in != NULL);
+  out = fopen("/jout", "w");
+  rtems_test_assert(out != NULL);
+
+  stdin = in;
+  stdout = out;
+
+  sc = rtems_shell_script(
+    "JTST",
+    8 * 1024,
+    1,
+    "stdin",
+    "stdout",
+    false,
+    true,
+    false);
+  rtems_test_assert(sc == RTEMS_SUCCESSFUL);
+
+  stdout = current_stdout;
+  stdin = current_stdin;
+
+  rtems_test_assert(fclose(in) == 0);
+  rtems_test_assert(fclose(out) == 0);
+
+  out = fopen("/jout", "r");
+  rtems_test_assert(out != NULL);
+  rtems_test_assert(!feof(out));
+  len = fread(buf, sizeof(char), sizeof(buf), out);
+  rtems_test_assert(len > 0);
+  rtems_test_assert(strcmp(joel_out_2, buf) == 0);
+  rtems_test_assert(fclose(out) == 0);
+}
+
 static void test(void)
 {
   rtems_user_env_t *uenv;
@@ -163,6 +277,7 @@ static void Init(rtems_task_argument arg)
   TEST_BEGIN();
 
   test();
+  test_joel();
 
   TEST_END();
   rtems_test_exit(0);
@@ -171,10 +286,10 @@ static void Init(rtems_task_argument arg)
 #define CONFIGURE_APPLICATION_DOES_NOT_NEED_CLOCK_DRIVER
 #define CONFIGURE_APPLICATION_NEEDS_SIMPLE_CONSOLE_DRIVER
 
-#define CONFIGURE_MAXIMUM_FILE_DESCRIPTORS 4
+#define CONFIGURE_MAXIMUM_FILE_DESCRIPTORS 5
 
-#define CONFIGURE_MAXIMUM_TASKS 1
-#define CONFIGURE_MAXIMUM_POSIX_KEYS 1
+#define CONFIGURE_MAXIMUM_TASKS 3
+#define CONFIGURE_MAXIMUM_POSIX_KEYS 2
 #define CONFIGURE_MAXIMUM_POSIX_KEY_VALUE_PAIRS 2
 
 #define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
-- 
2.23.0



More information about the devel mailing list