From 2c82bef70225f1b8f4475aeaf5510079fe45f7be Mon Sep 17 00:00:00 2001 From: Xiang Xiao Date: Sat, 16 Apr 2022 19:38:17 +0800 Subject: [PATCH] sched/environ: Refine the environment variables storage layout to confirm the standard and then implement get_environ_ptr correctly Signed-off-by: Xiang Xiao --- include/nuttx/sched.h | 3 +- sched/environ/env_dup.c | 45 ++++++++++++++++------- sched/environ/env_findvar.c | 27 ++++++++------ sched/environ/env_foreach.c | 13 ++++--- sched/environ/env_getenv.c | 12 +++--- sched/environ/env_getenvironptr.c | 24 +----------- sched/environ/env_release.c | 12 ++++-- sched/environ/env_removevar.c | 61 +++++++++++++++---------------- sched/environ/env_setenv.c | 50 ++++++++++++++----------- sched/environ/env_unsetenv.c | 45 ++--------------------- sched/environ/environ.h | 10 ++--- 11 files changed, 138 insertions(+), 164 deletions(-) diff --git a/include/nuttx/sched.h b/include/nuttx/sched.h index 06235096ebc..88659844f26 100644 --- a/include/nuttx/sched.h +++ b/include/nuttx/sched.h @@ -526,8 +526,7 @@ struct task_group_s #ifndef CONFIG_DISABLE_ENVIRON /* Environment variables **************************************************/ - size_t tg_envsize; /* Size of environment string allocation */ - FAR char *tg_envp; /* Allocated environment strings */ + FAR char **tg_envp; /* Allocated environment strings */ #endif #ifndef CONFIG_DISABLE_POSIX_TIMERS diff --git a/sched/environ/env_dup.c b/sched/environ/env_dup.c index c6bddf6a73a..58f15bb9d5d 100644 --- a/sched/environ/env_dup.c +++ b/sched/environ/env_dup.c @@ -64,8 +64,8 @@ int env_dup(FAR struct task_group_s *group) { FAR struct tcb_s *ptcb = this_task(); - FAR char *envp = NULL; - size_t envlen; + FAR char **envp = NULL; + size_t envc = 0; int ret = OK; DEBUGASSERT(group != NULL && ptcb != NULL && ptcb->group != NULL); @@ -82,40 +82,59 @@ int env_dup(FAR struct task_group_s *group) { /* Yes.. The parent task has an environment allocation. */ - envlen = ptcb->group->tg_envsize; - envp = NULL; + while (ptcb->group->tg_envp[envc] != NULL) + { + envc++; + } /* A special case is that the parent has an "empty" environment * allocation, i.e., there is an allocation in place but it - * contains no variable definitions and, hence, envlen == 0. + * contains no variable definitions and, hence, envc == 0. */ - if (envlen > 0) + if (envc > 0) { /* There is an environment, duplicate it */ - envp = (FAR char *)group_malloc(group, envlen + 1); + envp = group_malloc(group, sizeof(*envp) * (envc + 1)); if (envp == NULL) { /* The parent's environment can not be inherited due to a * failure in the allocation of the child environment. */ - envlen = 0; - ret = -ENOMEM; + ret = -ENOMEM; } else { + envp[envc] = NULL; + /* Duplicate the parent environment. */ - memcpy(envp, ptcb->group->tg_envp, envlen + 1); + while (envc-- > 0) + { + envp[envc] = group_malloc(group, + strlen(ptcb->group->tg_envp[envc]) + 1); + if (envp[envc] == NULL) + { + while (envp[++envc] != NULL) + { + group_free(group, envp[envc]); + } + + group_free(group, envp); + ret = -ENOMEM; + break; + } + + strcpy(envp[envc], ptcb->group->tg_envp[envc]); + } } } - /* Save the size and child environment allocation. */ + /* Save the child environment allocation. */ - group->tg_envsize = envlen; - group->tg_envp = envp; + group->tg_envp = envp; } sched_unlock(); diff --git a/sched/environ/env_findvar.c b/sched/environ/env_findvar.c index ce7df15d342..dfeb8efcad9 100644 --- a/sched/environ/env_findvar.c +++ b/sched/environ/env_findvar.c @@ -75,7 +75,7 @@ static bool env_cmpname(const char *pszname, const char *peqname) * pname - The variable name to find * * Returned Value: - * A pointer to the name=value string in the environment + * A index to the name=value string in the environment * * Assumptions: * - Not called from an interrupt handler @@ -83,25 +83,30 @@ static bool env_cmpname(const char *pszname, const char *peqname) * ****************************************************************************/ -FAR char *env_findvar(FAR struct task_group_s *group, FAR const char *pname) +int env_findvar(FAR struct task_group_s *group, FAR const char *pname) { - FAR char *ptr; - FAR char *end; + int i; /* Verify input parameters */ DEBUGASSERT(group != NULL && pname != NULL); + if (group->tg_envp == NULL) + { + return -ENOENT; + } + /* Search for a name=value string with matching name */ - end = &group->tg_envp[group->tg_envsize]; - for (ptr = group->tg_envp; - ptr < end && !env_cmpname(pname, ptr); - ptr += (strlen(ptr) + 1)); + for (i = 0; group->tg_envp[i] != NULL; i++) + { + if (env_cmpname(pname, group->tg_envp[i])) + { + return i; + } + } - /* Check for success */ - - return (ptr < end) ? ptr : NULL; + return -ENOENT; } #endif /* CONFIG_DISABLE_ENVIRON */ diff --git a/sched/environ/env_foreach.c b/sched/environ/env_foreach.c index 05a73a2e5a2..176cb5084dc 100644 --- a/sched/environ/env_foreach.c +++ b/sched/environ/env_foreach.c @@ -65,22 +65,23 @@ int env_foreach(FAR struct task_group_s *group, env_foreach_t cb, FAR void *arg) { - FAR char *ptr; - FAR char *end; int ret = OK; + int i; /* Verify input parameters */ DEBUGASSERT(group != NULL && cb != NULL); - /* Search for a name=value string with matching name */ + if (group->tg_envp == NULL) + { + return ret; + } - end = &group->tg_envp[group->tg_envsize]; - for (ptr = group->tg_envp; ptr < end; ptr += (strlen(ptr) + 1)) + for (i = 0; group->tg_envp[i] != NULL; i++) { /* Perform the callback */ - ret = cb(arg, ptr); + ret = cb(arg, group->tg_envp[i]); /* Terminate the traversal early if the callback so requests by * returning a non-zero value. diff --git a/sched/environ/env_getenv.c b/sched/environ/env_getenv.c index c279cb5fa44..19bcca82284 100644 --- a/sched/environ/env_getenv.c +++ b/sched/environ/env_getenv.c @@ -60,7 +60,6 @@ FAR char *getenv(FAR const char *name) { FAR struct tcb_s *rtcb; FAR struct task_group_s *group; - FAR char *pvar; FAR char *pvalue = NULL; int ret = OK; @@ -68,7 +67,7 @@ FAR char *getenv(FAR const char *name) if (name == NULL) { - ret = EINVAL; + ret = -EINVAL; goto errout; } @@ -80,20 +79,19 @@ FAR char *getenv(FAR const char *name) /* Check if the variable exists */ - if (group == NULL || (pvar = env_findvar(group, name)) == NULL) + if (group == NULL || (ret = env_findvar(group, name)) < 0) { - ret = ENOENT; goto errout_with_lock; } /* It does! Get the value sub-string from the name=value string */ - pvalue = strchr(pvar, '='); + pvalue = strchr(group->tg_envp[ret], '='); if (pvalue == NULL) { /* The name=value string has no '=' This is a bug! */ - ret = EINVAL; + ret = -EINVAL; goto errout_with_lock; } @@ -106,7 +104,7 @@ FAR char *getenv(FAR const char *name) errout_with_lock: sched_unlock(); errout: - set_errno(ret); + set_errno(-ret); return NULL; } diff --git a/sched/environ/env_getenvironptr.c b/sched/environ/env_getenvironptr.c index 0ad5dc727e4..acf633140d0 100644 --- a/sched/environ/env_getenvironptr.c +++ b/sched/environ/env_getenvironptr.c @@ -54,29 +54,9 @@ FAR char **get_environ_ptr(void) { -#if 1 + FAR struct tcb_s *tcb = this_task(); - /* Type of internal representation of environment is incompatible with - * char ** return value. - */ - - return NULL; - -#else - - /* Return a reference to the thread-private environ in the TCB. */ - - FAR struct tcb_s *ptcb = this_task(); - if (ptcb->envp) - { - return &ptcb->envp->ev_env; - } - else - { - return NULL; - } - -#endif + return tcb->group->tg_envp; } #endif /* CONFIG_DISABLE_ENVIRON */ diff --git a/sched/environ/env_release.c b/sched/environ/env_release.c index 412e2661a4c..e9089a0819e 100644 --- a/sched/environ/env_release.c +++ b/sched/environ/env_release.c @@ -61,12 +61,19 @@ void env_release(FAR struct task_group_s *group) { - DEBUGASSERT(group != NULL); + int i; - /* Free any allocate environment strings */ + DEBUGASSERT(group != NULL); if (group->tg_envp) { + /* Free any allocate environment strings */ + + for (i = 0; group->tg_envp[i] != NULL; i++) + { + group_free(group, group->tg_envp[i]); + } + /* Free the environment */ group_free(group, group->tg_envp); @@ -76,7 +83,6 @@ void env_release(FAR struct task_group_s *group) * task group structure are reset to initial values. */ - group->tg_envsize = 0; group->tg_envp = NULL; } diff --git a/sched/environ/env_removevar.c b/sched/environ/env_removevar.c index 04b7c87be7b..f7afa192d1c 100644 --- a/sched/environ/env_removevar.c +++ b/sched/environ/env_removevar.c @@ -30,6 +30,8 @@ #include #include +#include + #include "environ/environ.h" /**************************************************************************** @@ -45,10 +47,10 @@ * Input Parameters: * group - The task group with the environment containing the name=value * pair - * pvar - A pointer to the name=value pair in the restroom + * index - A index to the name=value pair in the restroom * * Returned Value: - * Zero on success + * None * * Assumptions: * - Not called from an interrupt handler @@ -57,44 +59,39 @@ * ****************************************************************************/ -int env_removevar(FAR struct task_group_s *group, FAR char *pvar) +void env_removevar(FAR struct task_group_s *group, int index) { - FAR char *end; /* Pointer to the end+1 of the environment */ - int alloc; /* Size of the allocated environment */ - int ret = ERROR; + DEBUGASSERT(group != NULL && index >= 0); - DEBUGASSERT(group != NULL && pvar != NULL); + /* Free the allocate environment string */ - /* Verify that the pointer lies within the environment region */ + group_free(group, group->tg_envp[index]); - alloc = group->tg_envsize; /* Size of the allocated environment */ - end = &group->tg_envp[alloc]; /* Pointer to the end+1 of the environment */ + /* Move all of the environment strings after the removed one 'down.' + * this is inefficient, but robably not high duty. + */ - if (pvar >= group->tg_envp && pvar < end) + do { - /* Set up for the removal */ - - int len = strlen(pvar) + 1; /* Length of name=value string to remove */ - FAR char *src = &pvar[len]; /* Address of name=value string after */ - FAR char *dest = pvar; /* Location to move the next string */ - int count = end - src; /* Number of bytes to move (might be zero) */ - - /* Move all of the environment strings after the removed one 'down.' - * this is inefficient, but robably not high duty. - */ - - memmove(dest, src, count + 1); - - /* Then set to the new allocation size. The caller is expected to - * call realloc at some point but we don't do that here because the - * caller may add more stuff to the environment. - */ - - group->tg_envsize -= len; - ret = OK; + group->tg_envp[index] = group->tg_envp[index + 1]; } + while (group->tg_envp[++index] != NULL); - return ret; + /* Free the old environment (if there was one) */ + + if (index == 1) + { + group_free(group, group->tg_envp); + group->tg_envp = NULL; + } + else + { + /* Reallocate the environment to reclaim a little memory */ + + group->tg_envp = group_realloc(group, group->tg_envp, + sizeof(*group->tg_envp) * (index + 1)); + DEBUGASSERT(group->tg_envp != NULL); + } } #endif /* CONFIG_DISABLE_ENVIRON */ diff --git a/sched/environ/env_setenv.c b/sched/environ/env_setenv.c index 3dbfb80f41a..979ab500c48 100644 --- a/sched/environ/env_setenv.c +++ b/sched/environ/env_setenv.c @@ -70,8 +70,8 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite) FAR struct tcb_s *rtcb; FAR struct task_group_s *group; FAR char *pvar; - FAR char *newenvp; - int newsize; + FAR char **envp; + int envc = 0; int varlen; int ret = OK; @@ -114,7 +114,7 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite) /* Check if the variable already exists */ - if (group->tg_envp && (pvar = env_findvar(group, name)) != NULL) + if (group->tg_envp && (ret = env_findvar(group, name)) >= 0) { /* It does! Do we have permission to overwrite the existing value? */ @@ -131,7 +131,7 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite) * the environment buffer; this will happen below. */ - env_removevar(group, pvar); + env_removevar(group, ret); } /* Get the size of the new name=value string. @@ -142,38 +142,44 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite) /* Then allocate or reallocate the environment buffer */ + pvar = group_malloc(group, varlen); + if (pvar == NULL) + { + ret = ENOMEM; + goto errout_with_lock; + } + if (group->tg_envp) { - newsize = group->tg_envsize + varlen; - newenvp = (FAR char *)group_realloc(group, group->tg_envp, - newsize + 1); - if (!newenvp) + while (group->tg_envp[envc] != NULL) { - ret = ENOMEM; - goto errout_with_lock; + envc++; } - pvar = &newenvp[group->tg_envsize]; + envp = group_realloc(group, group->tg_envp, + sizeof(*envp) * (envc + 2)); + if (envp == NULL) + { + ret = ENOMEM; + goto errout_with_var; + } } else { - newsize = varlen; - newenvp = (FAR char *)group_malloc(group, varlen + 1); - if (!newenvp) + envp = group_malloc(group, sizeof(*envp) * 2); + if (envp == NULL) { ret = ENOMEM; - goto errout_with_lock; + goto errout_with_var; } - - pvar = newenvp; } - newenvp[newsize] = '\0'; + envp[envc++] = pvar; + envp[envc] = NULL; - /* Save the new buffer and size */ + /* Save the new buffer */ - group->tg_envp = newenvp; - group->tg_envsize = newsize; + group->tg_envp = envp; /* Now, put the new name=value string into the environment buffer */ @@ -181,6 +187,8 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite) sched_unlock(); return OK; +errout_with_var: + group_free(group, pvar); errout_with_lock: sched_unlock(); errout: diff --git a/sched/environ/env_unsetenv.c b/sched/environ/env_unsetenv.c index 2b6ba7e098a..6d227d0d375 100644 --- a/sched/environ/env_unsetenv.c +++ b/sched/environ/env_unsetenv.c @@ -61,9 +61,6 @@ int unsetenv(FAR const char *name) { FAR struct tcb_s *rtcb = this_task(); FAR struct task_group_s *group = rtcb->group; - FAR char *pvar; - FAR char *newenvp; - int newsize; int ret = OK; DEBUGASSERT(name && group); @@ -71,48 +68,12 @@ int unsetenv(FAR const char *name) /* Check if the variable exists */ sched_lock(); - if (group && (pvar = env_findvar(group, name)) != NULL) + if (group && (ret = env_findvar(group, name)) >= 0) { /* It does! Remove the name=value pair from the environment. */ - env_removevar(group, pvar); - - /* Reallocate the new environment buffer */ - - newsize = group->tg_envsize; - if (newsize <= 0) - { - /* Free the old environment (if there was one) */ - - if (group->tg_envp != NULL) - { - group_free(group, group->tg_envp); - group->tg_envp = NULL; - } - - group->tg_envsize = 0; - } - else - { - /* Reallocate the environment to reclaim a little memory */ - - newenvp = (FAR char *)group_realloc(group, group->tg_envp, - newsize + 1); - - if (newenvp == NULL) - { - set_errno(ENOMEM); - ret = ERROR; - } - else - { - /* Save the new environment pointer (it might have changed due - * to reallocation). - */ - - group->tg_envp = newenvp; - } - } + env_removevar(group, ret); + ret = OK; } sched_unlock(); diff --git a/sched/environ/environ.h b/sched/environ/environ.h index 3d8fdd012b1..5787801f086 100644 --- a/sched/environ/environ.h +++ b/sched/environ/environ.h @@ -111,7 +111,7 @@ void env_release(FAR struct task_group_s *group); * pname - The variable name to find * * Returned Value: - * A pointer to the name=value string in the environment + * A index to the name=value string in the environment * * Assumptions: * - Not called from an interrupt handler @@ -119,7 +119,7 @@ void env_release(FAR struct task_group_s *group); * ****************************************************************************/ -FAR char *env_findvar(FAR struct task_group_s *group, FAR const char *pname); +int env_findvar(FAR struct task_group_s *group, FAR const char *pname); /**************************************************************************** * Name: env_removevar @@ -130,10 +130,10 @@ FAR char *env_findvar(FAR struct task_group_s *group, FAR const char *pname); * Input Parameters: * group - The task group with the environment containing the name=value * pair - * pvar - A pointer to the name=value pair in the restroom + * index - A index to the name=value pair in the restroom * * Returned Value: - * Zero on success + * None * * Assumptions: * - Not called from an interrupt handler @@ -142,7 +142,7 @@ FAR char *env_findvar(FAR struct task_group_s *group, FAR const char *pname); * ****************************************************************************/ -int env_removevar(FAR struct task_group_s *group, FAR char *pvar); +void env_removevar(FAR struct task_group_s *group, int index); #undef EXTERN #ifdef __cplusplus