On 04/10, Jonathan Nieder wrote:
> Brandon Williams wrote:
>
> > In order to avoid allocation between 'fork()' and 'exec()' prepare the
> > environment to be used in the child process prior to forking.
>
> If using something like posix_spawn(), this would be needed anyway, so
> I'll review it.
>
> [...]
> > +++ b/run-command.c
> [...]
> > +static char **prep_childenv(const char *const *deltaenv)
> > +{
> > + char **childenv;
> > + int childenv_nr = 0, childenv_alloc = 0;
> > + int i;
> > +
> > + for (i = 0; environ[i]; i++)
> > + childenv_nr++;
> > + for (i = 0; deltaenv && deltaenv[i]; i++)
> > + childenv_alloc++;
> > + /* Add one for the NULL termination */
> > + childenv_alloc += childenv_nr + 1;
> > +
> > + childenv = xcalloc(childenv_alloc, sizeof(char *));
> > + memcpy(childenv, environ, childenv_nr * sizeof(char *));
> > +
> > + /* merge in deltaenv */
> > + for (i = 0; deltaenv && deltaenv[i]; i++)
> > + childenv_nr = do_putenv(childenv, childenv_nr, deltaenv[i]);
> > +
> > + return childenv;
> > +}
>
> This potentially copies around most of 'environ' several times as it
> adjusts for each deltaenv item. Can it be simplified? E.g.
The only time it copies anything is that first memcpy which makes a
duplicate of the environ, and then when an item is being removed in
do_putenv it will move the back of the array to fill in for the entry
being removed.
>
> struct argv_array result = ARGV_ARRAY_INIT;
> struct string_list mods = STRING_LIST_INIT_DUP;
> struct strbuf key = STRBUF_INIT;
> const char **p;
>
> for (p = cmd_env; *p; p++) {
> const char *equals = strchr(*p, '=');
> if (equals) {
> strbuf_reset(&key);
> strbuf_add(&key, *p, equals - *p);
> string_list_append(&mods, key.buf)->util = *p;
> } else {
> string_list_append(&mods, *p);
> }
> }
> string_list_sort(&mods);
>
> for (p = environ; *p; p++) {
> struct string_list_item *item;
> const char *equals = strchr(*p, '=');
> if (!equals)
> continue;
> strbuf_reset(&key);
> strbuf_add(&key, *p, equals - *p);
> item = string_list_lookup(&mods, key.buf);
>
> if (!item) /* no change */
> argv_array_push(&result, *p);
> else if (!item->util) /* unsetenv */
> ; /* skip */
> else /* setenv */
> argv_array_push(&result, item->util);
> }
>
> strbuf_release(&key);
> string_list_clear(&mods);
> return argv_array_detach(&result);
This is probably still incomplete as I don't see how this accounts for
entries in 'cmd_env' which are being added to the environment and not
just replacing existing ones.
--
Brandon Williams