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.

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,21 @@ 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) {
+        state = getopt_data_get();
         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,7 @@
 #include <zephyr/toolchain.h>
 
 #if defined CONFIG_SHELL_GETOPT
+#define __need_getopt_newlib
 #include <getopt.h>
 #endif
 
@@ -995,7 +996,7 @@ struct shell_ctx {
 
 #if defined CONFIG_SHELL_GETOPT
 	/*!< getopt context for a shell backend. */
-	struct getopt_state getopt;
+	struct getopt_data getopt;
 #endif
 
 	uint16_t cmd_buff_len; /*!< Command length.*/

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,27 @@ 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();
-
 	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 +73,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 +90,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 +112,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,27 @@ struct getopt_state {
 #endif
 };
 
+/*
+ * The GETOPT_DATA_INITIALIZER macro is used to initialize a statically-
+ * allocated variable of type struct getopt_data.
+ */
+
+#if CONFIG_GETOPT_LONG
+#define GETOPT_DATA_INITIALIZER	{ 1, 1, 0, 0, NULL, "", -1, -1 }
+#else
+#define GETOPT_DATA_INITIALIZER	{ 1, 1, 0, 0, NULL, "" }
+#endif
+
+#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 +69,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 +119,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,45 +3,24 @@
  *
  * 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 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 */
-
-#if CONFIG_GETOPT_LONG
-	.nonopt_start = -1, /* first non option argument (for permute) */
-	.nonopt_end = -1,   /* first option after non options (for permute) */
-#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;
@@ -50,19 +29,12 @@ void z_getopt_global_state_update(struct getopt_state *state)
 	optarg = state->optarg;
 }
 
-/* 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;
-		}
-	}
-#endif
-	/* If not a shell thread return a common pointer */
-	return &m_getopt_common_state;
+	state->opterr = opterr;
+	state->optind = optind;
+	state->optopt = optopt;
+	state->optreset = optreset;
+	state->optarg = optarg;
 }

lib/posix/c_lib_ext/getopt/getopt_common.h

@@ -7,14 +7,21 @@
 #ifndef _GETOPT_COMMON_H__
 #define _GETOPT_COMMON_H__
 
+#define __need_getopt_newlib
+#include "getopt.h"
+
 #ifdef __cplusplus
 extern "C" {
 #endif
 
-/* This function is not thread safe. All threads using getopt are calling
- * this function.
+/* These functions are not thread safe. All threads using getopt are calling
+ * them.
  */
-void z_getopt_global_state_update(struct getopt_state *state);
+/* Put state into POSIX global variables */
+void z_getopt_global_state_put(struct getopt_data *state);
+
+/* Get state from POSIX global variables */
+void z_getopt_global_state_get(struct getopt_data *state);
 
 #ifdef __cplusplus
 }