You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/04/16 16:57:55 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #6083: sched/environ: Refine the environment variables storage layout
xiaoxiang781216 opened a new pull request, #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083
## Summary
to confirm the standard and then implement get_environ_ptr correctly
## Impact
environment variable Internal implementation
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851942781
##########
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 */
Review Comment:
> It is fine for now, but we can consider an improvement of runtime speed by adding `size_t tg_envc;` and not do not perform
>
> ```
> while (ptcb->group->tg_envp[envc] != NULL)
> {
> envc++;
> }
> ```
>
> every time we need to get the number of variables.
I consider this before, but it require the caller pass envc along envp, which isn't easy with the current api, e.g.:
```
int execve(const char *pathname, char *const argv[], char *const envp[]);
int posix_spawn(pid_t *restrict pid, const char *restrict path,
const posix_spawn_file_actions_t *restrict file_actions,
const posix_spawnattr_t *restrict attrp,
char *const argv[],
char *const envp[]);
```
> Adding it will also eliminate requirement for `group->tg_envp` to be NULL-terminated.
tg_envp need be null terminated to implement get_environ_ptr(environ):
https://man7.org/linux/man-pages/man7/environ.7.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#issuecomment-1100846268
This implementation is already much cleaner than the old one, thanks for fixing it.
My only question is how the string vectors are allocated. IMO the allocation can be done exactly once, but maybe there is some other reason for allocating memory for the vector and the strings separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851926283
##########
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 */
Review Comment:
It is fine for now, but we can consider an improvement of runtime speed by adding `size_t tg_envc;` and not do not perform
```
while (ptcb->group->tg_envp[envc] != NULL)
{
envc++;
}
```
every time we need to get the number of variables. Adding it will also eliminate requirement for `group->tg_envp` to be NULL-terminated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851654615
##########
sched/environ/env_setenv.c:
##########
@@ -144,36 +144,42 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite)
if (group->tg_envp)
{
- newsize = group->tg_envsize + varlen;
- newenvp = (FAR char *)group_realloc(group, group->tg_envp,
- newsize + 1);
+ while (group->tg_envp[oldsize] != NULL)
+ {
+ oldsize++;
+ }
+
+ newenvp = group_realloc(group, group->tg_envp,
+ sizeof(*newenvp) * (oldsize + 2));
if (!newenvp)
{
ret = ENOMEM;
goto errout_with_lock;
}
- pvar = &newenvp[group->tg_envsize];
+ newenvp[oldsize + 1] = NULL;
}
else
{
- newsize = varlen;
- newenvp = (FAR char *)group_malloc(group, varlen + 1);
+ oldsize = 0;
+ newenvp = group_zalloc(group, sizeof(*newenvp) * 2);
if (!newenvp)
{
ret = ENOMEM;
goto errout_with_lock;
}
-
- pvar = newenvp;
}
- newenvp[newsize] = '\0';
+ group->tg_envp = newenvp;
- /* Save the new buffer and size */
+ pvar = group_malloc(group, varlen);
Review Comment:
Maybe we can do allocation of `pvar` before realloc/zalloc above just to have better error handling?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851846811
##########
sched/environ/env_setenv.c:
##########
@@ -142,45 +142,53 @@ 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));
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851654002
##########
sched/environ/env_getenv.c:
##########
@@ -106,7 +104,7 @@ FAR char *getenv(FAR const char *name)
errout_with_lock:
sched_unlock();
errout:
- set_errno(ret);
+ set_errno(-ret);
Review Comment:
what is the benefit of this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851847372
##########
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]; i++)
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851942781
##########
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 */
Review Comment:
> It is fine for now, but we can consider an improvement of runtime speed by adding `size_t tg_envc;` and not do not perform
>
> ```
> while (ptcb->group->tg_envp[envc] != NULL)
> {
> envc++;
> }
> ```
>
> every time we need to get the number of variables.
I consider this before, but it require the caller pass envc along envp, which isn't easy with the current api, e.g.:
```
int execve(const char *pathname, char *const argv[], char *const envp[]);
int posix_spawn(pid_t *pid, const char *path,
const posix_spawn_file_actions_t *file_actions,
const posix_spawnattr_t *attrp,
char *const argv[],
char *const envp[]);
```
You can see all api doesn't require the caller pass the length of envp, but assume terminated with NULL.
> Adding it will also eliminate requirement for `group->tg_envp` to be NULL-terminated.
tg_envp need be null terminated to implement get_environ_ptr(environ):
https://man7.org/linux/man-pages/man7/environ.7.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#issuecomment-1100874550
> This implementation is already much cleaner than the old one, thanks for fixing it.
>
> My only question is how the string vectors are allocated. IMO the allocation can be done exactly once, but maybe there is some other reason for allocating memory for the vector and the strings separately.
It's more efficient to allocate the array and string in one allocation for read only string(e.g. argv), but since environ string is modifiable:
1. One allocation make the management more hard
2. More important the follow code is broken:
```
setenv("env1", "Hello");
const char *env1 = getenv("env1");
setenv("env2", "World");
printf("%s\n", evn1);
```
The last statement may access the freed memory because the 3rd statement may reallocate the whole buffer to new location.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851770972
##########
sched/environ/env_dup.c:
##########
@@ -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)
Review Comment:
Reply at the end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#issuecomment-1100900932
@pussuw #6087 is an example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851652926
##########
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 envlen = 0;
Review Comment:
probably renaming `envlen` -> `envc` can be a good option.
##########
sched/environ/env_dup.c:
##########
@@ -94,28 +96,50 @@ int env_dup(FAR struct task_group_s *group)
{
/* There is an environment, duplicate it */
- envp = (FAR char *)group_malloc(group, envlen + 1);
+ envp = group_malloc(group, sizeof(*envp) * (envlen + 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
{
/* Duplicate the parent environment. */
- memcpy(envp, ptcb->group->tg_envp, envlen + 1);
+ do
+ {
+ if (ptcb->group->tg_envp[envlen] == NULL)
+ {
+ envp[envlen] = NULL;
+ continue;
Review Comment:
optional: below code can be moved to `else` and `continue` can be eliminated. Or even this may be moved out of `do {} while` like:
```
/* Duplicate the parent environment. */
envp[envlen--] = NULL;
for (; envlen > 0; envlen--)
{
/* perform allocation here */
}
```
##########
sched/environ/env_dup.c:
##########
@@ -94,28 +96,50 @@ int env_dup(FAR struct task_group_s *group)
{
/* There is an environment, duplicate it */
- envp = (FAR char *)group_malloc(group, envlen + 1);
+ envp = group_malloc(group, sizeof(*envp) * (envlen + 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
{
/* Duplicate the parent environment. */
- memcpy(envp, ptcb->group->tg_envp, envlen + 1);
+ do
+ {
+ if (ptcb->group->tg_envp[envlen] == NULL)
+ {
+ envp[envlen] = NULL;
+ continue;
+ }
+
+ envp[envlen] = group_malloc(group,
+ strlen(ptcb->group->tg_envp[envlen]) + 1);
+ if (envp[envlen] == NULL)
+ {
+ while (envp[++envlen] != NULL)
+ {
+ group_free(group, envp[envlen]);
+ }
+
+ group_free(group, envp);
+ ret = -ENOMEM;
+ break;
+ }
+
+ strcpy(envp[envlen], ptcb->group->tg_envp[envlen]);
+ }
+ while (envlen--);
Review Comment:
optional `> 0`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#issuecomment-1101123493
@pkarashchenko macOS CI always fail to:
" wget --quiet https://static.dev.sifive.com/dev-tools/riscv64-unknown-elf-gcc-8.3.0-2019.08.0-x86_64-apple-darwin.tar.gz"
Let's ignore this error temporarily?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851733651
##########
sched/environ/env_dup.c:
##########
@@ -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)
Review Comment:
Another option would be to allocate memory for the stings at the same time as you allocate memory for the string pointers.
https://github.com/apache/incubator-nuttx/blob/master/binfmt/binfmt_copyargv.c does exactly this. I think it might be a bit less wasteful to allocate all of the memory at once ? Less chance for fragmentation too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851655609
##########
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 envlen = 0;
Review Comment:
Done.
##########
sched/environ/env_getenv.c:
##########
@@ -106,7 +104,7 @@ FAR char *getenv(FAR const char *name)
errout_with_lock:
sched_unlock();
errout:
- set_errno(ret);
+ set_errno(-ret);
Review Comment:
avoid at line 85:
```
ret = -ret;
```
##########
sched/environ/env_setenv.c:
##########
@@ -144,36 +144,42 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite)
if (group->tg_envp)
{
- newsize = group->tg_envsize + varlen;
- newenvp = (FAR char *)group_realloc(group, group->tg_envp,
- newsize + 1);
+ while (group->tg_envp[oldsize] != NULL)
+ {
+ oldsize++;
+ }
+
+ newenvp = group_realloc(group, group->tg_envp,
+ sizeof(*newenvp) * (oldsize + 2));
if (!newenvp)
{
ret = ENOMEM;
goto errout_with_lock;
}
- pvar = &newenvp[group->tg_envsize];
+ newenvp[oldsize + 1] = NULL;
}
else
{
- newsize = varlen;
- newenvp = (FAR char *)group_malloc(group, varlen + 1);
+ oldsize = 0;
+ newenvp = group_zalloc(group, sizeof(*newenvp) * 2);
if (!newenvp)
{
ret = ENOMEM;
goto errout_with_lock;
}
-
- pvar = newenvp;
}
- newenvp[newsize] = '\0';
+ group->tg_envp = newenvp;
- /* Save the new buffer and size */
+ pvar = group_malloc(group, varlen);
Review Comment:
Actually, the initial code just like what you suggest above, but I change it to the current form, because we don't need release newenvp or pvar at all.
##########
sched/environ/env_dup.c:
##########
@@ -94,28 +96,50 @@ int env_dup(FAR struct task_group_s *group)
{
/* There is an environment, duplicate it */
- envp = (FAR char *)group_malloc(group, envlen + 1);
+ envp = group_malloc(group, sizeof(*envp) * (envlen + 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
{
/* Duplicate the parent environment. */
- memcpy(envp, ptcb->group->tg_envp, envlen + 1);
+ do
+ {
+ if (ptcb->group->tg_envp[envlen] == NULL)
+ {
+ envp[envlen] = NULL;
+ continue;
Review Comment:
Done.
##########
sched/environ/env_dup.c:
##########
@@ -94,28 +96,50 @@ int env_dup(FAR struct task_group_s *group)
{
/* There is an environment, duplicate it */
- envp = (FAR char *)group_malloc(group, envlen + 1);
+ envp = group_malloc(group, sizeof(*envp) * (envlen + 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
{
/* Duplicate the parent environment. */
- memcpy(envp, ptcb->group->tg_envp, envlen + 1);
+ do
+ {
+ if (ptcb->group->tg_envp[envlen] == NULL)
+ {
+ envp[envlen] = NULL;
+ continue;
+ }
+
+ envp[envlen] = group_malloc(group,
+ strlen(ptcb->group->tg_envp[envlen]) + 1);
+ if (envp[envlen] == NULL)
+ {
+ while (envp[++envlen] != NULL)
+ {
+ group_free(group, envp[envlen]);
+ }
+
+ group_free(group, envp);
+ ret = -ENOMEM;
+ break;
+ }
+
+ strcpy(envp[envlen], ptcb->group->tg_envp[envlen]);
+ }
+ while (envlen--);
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851658426
##########
sched/environ/env_setenv.c:
##########
@@ -144,36 +144,42 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite)
if (group->tg_envp)
{
- newsize = group->tg_envsize + varlen;
- newenvp = (FAR char *)group_realloc(group, group->tg_envp,
- newsize + 1);
+ while (group->tg_envp[oldsize] != NULL)
+ {
+ oldsize++;
+ }
+
+ newenvp = group_realloc(group, group->tg_envp,
+ sizeof(*newenvp) * (oldsize + 2));
if (!newenvp)
{
ret = ENOMEM;
goto errout_with_lock;
}
- pvar = &newenvp[group->tg_envsize];
+ newenvp[oldsize + 1] = NULL;
}
else
{
- newsize = varlen;
- newenvp = (FAR char *)group_malloc(group, varlen + 1);
+ oldsize = 0;
+ newenvp = group_zalloc(group, sizeof(*newenvp) * 2);
if (!newenvp)
{
ret = ENOMEM;
goto errout_with_lock;
}
-
- pvar = newenvp;
}
- newenvp[newsize] = '\0';
+ group->tg_envp = newenvp;
- /* Save the new buffer and size */
+ pvar = group_malloc(group, varlen);
Review Comment:
I mean that if `pvar` allocation fails the code
```
while (group->tg_envp[envc] != NULL)
{
envc++;
}
```
will con provide us a reliable information about length of memory pointed by `group->tg_envp` during the next API call. I mean that if allocation of `pvar` fails early then we can bypass all the reallocation code and return error on early stages. If that makes sense to you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851659107
##########
sched/environ/env_setenv.c:
##########
@@ -144,36 +144,42 @@ int setenv(FAR const char *name, FAR const char *value, int overwrite)
if (group->tg_envp)
{
- newsize = group->tg_envsize + varlen;
- newenvp = (FAR char *)group_realloc(group, group->tg_envp,
- newsize + 1);
+ while (group->tg_envp[oldsize] != NULL)
+ {
+ oldsize++;
+ }
+
+ newenvp = group_realloc(group, group->tg_envp,
+ sizeof(*newenvp) * (oldsize + 2));
if (!newenvp)
{
ret = ENOMEM;
goto errout_with_lock;
}
- pvar = &newenvp[group->tg_envsize];
+ newenvp[oldsize + 1] = NULL;
}
else
{
- newsize = varlen;
- newenvp = (FAR char *)group_malloc(group, varlen + 1);
+ oldsize = 0;
+ newenvp = group_zalloc(group, sizeof(*newenvp) * 2);
if (!newenvp)
{
ret = ENOMEM;
goto errout_with_lock;
}
-
- pvar = newenvp;
}
- newenvp[newsize] = '\0';
+ group->tg_envp = newenvp;
- /* Save the new buffer and size */
+ pvar = group_malloc(group, varlen);
Review Comment:
Ok, done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851770972
##########
sched/environ/env_dup.c:
##########
@@ -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)
Review Comment:
Reply at the end. But, one allocation is good for copying userspace memory to kernel space. So, you can share binfmt_copyargv with binfmt_copyenv.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851847591
##########
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++)
Review Comment:
I considered this form before, but prefer the current one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6083: sched/environ: Refine the environment variables storage layout
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6083:
URL: https://github.com/apache/incubator-nuttx/pull/6083#discussion_r851793527
##########
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]; i++)
Review Comment:
```suggestion
for (i = 0; group->tg_envp[i] != NULL; i++)
```
##########
sched/environ/env_setenv.c:
##########
@@ -142,45 +142,53 @@ 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));
Review Comment:
```suggestion
envp = group_realloc(group, group->tg_envp,
sizeof(*envp) * (envc + 2));
```
##########
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++)
Review Comment:
Minor. Not even sure if should be applied:
```
for (i = 0; group->tg_envp != NULL && group->tg_envp[i] != NULL; i++)
```
this will eliminate
```
if (group->tg_envp == NULL)
{
return ret;
}
```
but will make a long line. Not sure what is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org