lib/posix: Adopt newlib-style re-entrant API for getopt family

Replace the Zephyr-specific re-entrant getopt family APIs with those from
newlib. This allows use of the newlib and picolibc native implementations
and provides a cleaner re-entrant mechanism, using a local variable to hold
the getopt state instead of having a thread-local variabled tucked away in
the shell context.

This will require changing all re-entrant users of getopt, switching from
the Zephyr-style interfaces to the newlib-style.

This also adopts the glibc/picolibc convention where setting optind = 0
causes the getopt functions to re-initialize all parsing state. This lets
us test the non-reentrant API without needing getopt_init().

Signed-off-by: Keith Packard <keithp@keithp.com>

Author: keith-packard

doc/services/shell/index.rst

@@ -690,9 +690,11 @@ in the FreeBSD project. This feature is activated by:
 :kconfig:option:`CONFIG_POSIX_C_LIB_EXT` set to ``y`` and :kconfig:option:`CONFIG_GETOPT_LONG`
 set to ``y``.
 
-This feature can be used in thread safe as well as non thread safe manner.
-The former is full compatible with regular getopt usage while the latter
-a bit differs.
+This feature can be used in thread safe as well as non thread safe
+manner.  The former is full compatible with regular getopt usage while
+the latter differs a bit. To gain access to the thread-safe APIs, add
+`#define __need_getopt_newlib` before `#include <getopt.h>` in your
+source code.
 
 An example non-thread safe usage:
 
@@ -714,20 +716,20 @@ An example thread safe usage:
 .. code-block:: c
 
   char *cvalue = NULL;
-  struct getopt_state *state;
-  while ((char c = getopt(argc, argv, "abhc:")) != -1) {
-        state = getopt_state_get();
+  struct getopt_data state = GETOPT_DATA_INITIALIZER;
+  while ((char c = getopt_r(argc, argv, "abhc:", &state)) != -1) {
         switch (c) {
         case 'c':
-                cvalue = state->optarg;
+                cvalue = state.optarg;
                 break;
         default:
                 break;
         }
   }
 
-Thread safe getopt functionality is activated by
-:kconfig:option:`CONFIG_SHELL_GETOPT` set to ``y``.
+Thread safe getopt functionality is enabled by defining `__need_getopt_newlib`
+before including getopt.h
+
 
 Obscured Input Feature
 **********************

include/zephyr/shell/shell.h

@@ -20,6 +20,12 @@
 #include <zephyr/toolchain.h>
 
 #if defined CONFIG_SHELL_GETOPT
+/*
+ * All shell commands should use the re-entrant APIs provided by the
+ * newlib-compatible getopt APIs. This exposes those and hides the
+ * globals used by the non-reentrant APIs.
+ */
+#define __need_getopt_newlib
 #include <getopt.h>
 #endif
 
@@ -993,11 +999,6 @@ struct shell_ctx {
 	/*!< Logging level for a backend. */
 	uint32_t log_level;
 
-#if defined CONFIG_SHELL_GETOPT
-	/*!< getopt context for a shell backend. */
-	struct getopt_state getopt;
-#endif
-
 	uint16_t cmd_buff_len; /*!< Command length.*/
 	uint16_t cmd_buff_pos; /*!< Command buffer cursor position.*/
 

lib/posix/c_lib_ext/Kconfig

@@ -23,7 +23,7 @@ config GETOPT_LONG
 	  not interfere with each other.
 	  All not shell threads share one global instance of getopt state, hence
 	  apart from shell this library is not thread safe. User can add support
-	  for other threads by extending function getopt_state_get in
+	  for other threads by extending function getopt_data_get in
 	  getopt_common.c file.
 
 endif # POSIX_C_LIB_EXT

lib/posix/c_lib_ext/getopt/getopt.c

@@ -35,7 +35,6 @@
 #else
 #include <zephyr/posix/unistd.h>
 #endif
-#include "getopt.h"
 #include "getopt_common.h"
 
 #include <zephyr/logging/log.h>
@@ -45,59 +44,41 @@ LOG_MODULE_REGISTER(getopt);
 #define BADARG ((int)':')
 #define EMSG   ""
 
-void getopt_init(void)
-{
-	struct getopt_state *state;
-
-	state = getopt_state_get();
-
-	state->opterr = 1;
-	state->optind = 1;
-	state->optopt = 0;
-	state->optreset = 0;
-	state->optarg = NULL;
-
-	state->place = ""; /* EMSG */
-
-#if CONFIG_GETOPT_LONG
-	state->nonopt_start = -1; /* first non option argument (for permute) */
-	state->nonopt_end = -1;   /* first option after non options (for permute) */
-#endif
-
-	opterr = 1;
-	optind = 1;
-	optopt = 0;
-	optreset = 0;
-	optarg = NULL;
-}
-
 /*
  * getopt --
  *	Parse argc/argv argument vector.
  */
-int getopt(int nargc, char *const nargv[], const char *ostr)
+int __getopt_r(int nargc, char *const nargv[], const char *ostr, struct getopt_data *state)
 {
-	struct getopt_state *state;
 	char *oli; /* option letter list index */
 
-	/* get getopt state of the current thread */
-	state = getopt_state_get();
+	/* glibc/picolibc compatibility -- reset if optind is 0 */
+	if (state->optind == 0) {
+		state->opterr = 1;
+		state->optind = 1;
+		state->optopt = 0;
+		state->optreset = 0;
+		state->optarg = NULL;
+		state->place = "";
+#if CONFIG_GETOPT_LONG
+		state->nonopt_start = -1;
+		state->nonopt_end = -1;
+#endif
+	}
 
 	if (state->optreset || *state->place == 0) { /* update scanning pointer */
 		state->optreset = 0;
 		state->place = nargv[state->optind];
 		if (state->optind >= nargc || *state->place++ != '-') {
 			/* Argument is absent or is not an option */
 			state->place = EMSG;
-			z_getopt_global_state_update(state);
 			return -1;
 		}
 		state->optopt = *state->place++;
 		if (state->optopt == '-' && *state->place == 0) {
 			/* "--" => end of options */
 			++state->optind;
 			state->place = EMSG;
-			z_getopt_global_state_update(state);
 			return -1;
 		}
 		if (state->optopt == 0) {
@@ -106,7 +87,6 @@ int getopt(int nargc, char *const nargv[], const char *ostr)
 			 */
 			state->place = EMSG;
 			if (strchr(ostr, '-') == NULL) {
-				z_getopt_global_state_update(state);
 				return -1;
 			}
 			state->optopt = '-';
@@ -124,7 +104,6 @@ int getopt(int nargc, char *const nargv[], const char *ostr)
 		if (state->opterr && *ostr != ':') {
 			LOG_DBG("illegal option -- %c", state->optopt);
 		}
-		z_getopt_global_state_update(state);
 		return BADCH;
 	}
 
@@ -147,18 +126,30 @@ int getopt(int nargc, char *const nargv[], const char *ostr)
 			/* option-argument absent */
 			state->place = EMSG;
 			if (*ostr == ':') {
-				z_getopt_global_state_update(state);
 				return BADARG;
 			}
 			if (state->opterr) {
 				LOG_DBG("option requires an argument -- %c", state->optopt);
 			}
-			z_getopt_global_state_update(state);
 			return BADCH;
 		}
 		state->place = EMSG;
 		++state->optind;
 	}
-	z_getopt_global_state_update(state);
 	return state->optopt; /* return option letter */
 }
+
+/*
+ * getopt --
+ *	Parse argc/argv argument vector.
+ */
+int getopt(int nargc, char *const nargv[], const char *ostr)
+{
+	struct getopt_data state;
+	int ret;
+
+	z_getopt_global_state_get(&state);
+	ret = __getopt_r(nargc, nargv, ostr, &state);
+	z_getopt_global_state_put(&state);
+	return ret;
+}

lib/posix/c_lib_ext/getopt/getopt.h

@@ -13,7 +13,8 @@ extern "C" {
 
 #include <zephyr/kernel.h>
 
-struct getopt_state {
+#ifdef __need_getopt_newlib
+struct getopt_data {
 	int opterr;   /* if error message should be printed */
 	int optind;   /* index into parent argv vector */
 	int optopt;   /* character checked for validity */
@@ -28,11 +29,23 @@ struct getopt_state {
 #endif
 };
 
+/*
+ * The GETOPT_DATA_INITIALIZER macro is used to initialize a statically-
+ * allocated variable of type struct getopt_data.
+ */
+
+#define GETOPT_DATA_INITIALIZER	{ 0 }
+
+#endif
+
+#if defined(__need_getopt_legacy) || !defined(__need_getopt_newlib)
+/* Applications using the newlib-style APIs "shouldn't" be using these variables */
 extern int optreset; /* reset getopt */
 extern char *optarg;
 extern int opterr;
 extern int optind;
 extern int optopt;
+#endif
 
 #define no_argument       0
 #define required_argument 1
@@ -52,12 +65,6 @@ struct option {
 	int val;
 };
 
-/* Function initializes getopt_state structure for current thread */
-void getopt_init(void);
-
-/* Function returns getopt_state structure for the current thread. */
-struct getopt_state *getopt_state_get(void);
-
 /**
  * @brief Parses the command-line arguments.
  *
@@ -108,6 +115,28 @@ int getopt_long(int nargc, char *const *nargv, const char *options,
 int getopt_long_only(int nargc, char *const *nargv, const char *options,
 		     const struct option *long_options, int *idx);
 
+/* Re-entrant forms of the above functions. These follow the newlib/picolibc API */
+
+#ifdef __need_getopt_newlib
+
+/* These #defines are to make accessing the reentrant functions easier.  */
+#define getopt_r		__getopt_r
+#define getopt_long_r		__getopt_long_r
+#define getopt_long_only_r	__getopt_long_only_r
+
+int __getopt_r(int __argc, char *const __argv[], const char *__optstring,
+	       struct getopt_data *__data);
+
+int __getopt_long_r(int __argc, char *const __argv[], const char *__shortopts,
+		    const struct option *__longopts, int *__longind,
+		    struct getopt_data *__data);
+
+int __getopt_long_only_r(int __argc, char *const __argv[], const char *__shortopts,
+			 const struct option *__longopts, int *__longind,
+			 struct getopt_data *__data);
+
+#endif /* __need_getopt_newlib */
+
 #ifdef __cplusplus
 }
 #endif

lib/posix/c_lib_ext/getopt/getopt_common.c

@@ -3,66 +3,54 @@
  *
  * SPDX-License-Identifier: Apache-2.0
  */
-#include <zephyr/kernel.h>
-#include <zephyr/shell/shell.h>
-#if CONFIG_SHELL_GETOPT
-#include <zephyr/sys/iterable_sections.h>
-#endif
-
-#include "getopt.h"
+#define __need_getopt_legacy
+#include "getopt_common.h"
 
 /* Referring  below variables is not thread safe. They reflects getopt state
  * only when 1 thread is using getopt.
- * When more threads are using getopt please call getopt_state_get to know
- * getopt state for the current thread.
+ * When more threads are using getopt please use getopt_r and friends
+ * to hold the getopt state in local variables
  */
-int opterr = 1; /* if error message should be printed */
-int optind = 1; /* index into parent argv vector */
+int opterr;	/* if error message should be printed */
+int optind;	/* index into parent argv vector */
 int optopt;     /* character checked for validity */
 int optreset;   /* reset getopt */
 char *optarg;   /* argument associated with option */
 
-/* Common state for all threads that did not have own getopt state. */
-static struct getopt_state m_getopt_common_state = {
-	.opterr = 1,
-	.optind = 1,
-	.optopt = 0,
-	.optreset = 0,
-	.optarg = NULL,
-
-	.place = "", /* EMSG */
-
+static char *place;
 #if CONFIG_GETOPT_LONG
-	.nonopt_start = -1, /* first non option argument (for permute) */
-	.nonopt_end = -1,   /* first option after non options (for permute) */
+static int nonopt_start;
+static int nonopt_end;
 #endif
-};
 
 /* This function is not thread safe. All threads using getopt are calling
  * this function.
  */
-void z_getopt_global_state_update(struct getopt_state *state)
+void z_getopt_global_state_put(struct getopt_data *state)
 {
 	opterr = state->opterr;
 	optind = state->optind;
 	optopt = state->optopt;
 	optreset = state->optreset;
 	optarg = state->optarg;
+	place = state->place;
+#if CONFIG_GETOPT_LONG
+	nonopt_start = state->nonopt_start;
+	nonopt_end = state->nonopt_end;
+#endif
 }
 
-/* It is internal getopt API function, it shall not be called by the user. */
-struct getopt_state *getopt_state_get(void)
+/* Place the global state into a struct */
+void z_getopt_global_state_get(struct getopt_data *state)
 {
-#if CONFIG_SHELL_GETOPT
-	k_tid_t tid;
-
-	tid = k_current_get();
-	STRUCT_SECTION_FOREACH(shell, sh) {
-		if (tid == sh->ctx->tid) {
-			return &sh->ctx->getopt;
-		}
-	}
+	state->opterr = opterr;
+	state->optind = optind;
+	state->optopt = optopt;
+	state->optreset = optreset;
+	state->optarg = optarg;
+	state->place = place;
+#if CONFIG_GETOPT_LONG
+	state->nonopt_start = nonopt_start;
+	state->nonopt_end = nonopt_end;
 #endif
-	/* If not a shell thread return a common pointer */
-	return &m_getopt_common_state;
 }