Re: [PATCH v5] C++ify gdb/common/environ.c

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Saturday, June 17 2017, Simon Marchi wrote:

> On 2017-06-17 00:23, Sergio Durigan Junior wrote:
>>  void
>> -set_in_environ (struct gdb_environ *e, const char *var, const char
>> *value)
>> +gdb_environ::set (const char *var, const char *value)
>>  {
>> -  int i;
>> -  int len = strlen (var);
>> -  char **vector = e->vector;
>> -  char *s;
>> -
>> -  for (i = 0; (s = vector[i]) != NULL; i++)
>> -    if (strncmp (s, var, len) == 0 && s[len] == '=')
>> -      break;
>> +  /* We have to unset the variable in the vector if it exists.  */
>> +  unset (var);
>>
>> -  if (s == 0)
>> -    {
>> -      if (i == e->allocated)
>> -	{
>> -	  e->allocated += 10;
>> -	  vector = (char **) xrealloc ((char *) vector,
>> -				       (e->allocated + 1) * sizeof (char *));
>> -	  e->vector = vector;
>> -	}
>> -      vector[i + 1] = 0;
>> -    }
>> -  else
>> -    xfree (s);
>> -
>> -  s = (char *) xmalloc (len + strlen (value) + 2);
>> -  strcpy (s, var);
>> -  strcat (s, "=");
>> -  strcat (s, value);
>> -  vector[i] = s;
>> -
>> -  /* This used to handle setting the PATH and GNUTARGET variables
>> -     specially.  The latter has been replaced by "set gnutarget"
>> -     (which has worked since GDB 4.11).  The former affects searching
>> -     the PATH to find SHELL, and searching the PATH to find the
>> -     argument of "symbol-file" or "exec-file".  Maybe we should have
>> -     some kind of "set exec-path" for that.  But in any event, having
>> -     "set env" affect anything besides the inferior is a bad idea.
>> -     What if we want to change the environment we pass to the program
>> -     without afecting GDB's behavior?  */
>> -
>> -  return;
>> +  /* Insert the element before the last one, which is always NULL.  */
>> +  m_environ_vector.insert (m_environ_vector.end () - 1,
>> +			   concat (var, "=", value, NULL));
>
> The breaks if we have just constructed an empty gdb_environ object, as
> the vector is completely empty (no terminating NULL).  So we'd need
> some kind of check before that, if the vector is empty, add a NULL
> element...
>
> I actually preferred the option of adding the NULL element to the
> vector in the gdb_environ constructor, since it allows always having
> the vector in a consistent state.  I don't think that avoiding that
> heap allocation is worth the complexity it adds to the code (unless we
> can prove otherwise by memory usage profiling).

I've had some time to think about this, and I agree.  I liked the code
better when it had the ctor doing the initialization of the vector.  I
think it also makes a lot more sense to always initialize the vector
with a NULL element, because this means we're correctly dealing with the
case where there's no environment variable to be passed to the inferior.

I'll update the code and re-submit the patch this way.

>
>> +static void
>> +run_tests ()
>> +{
>> +  if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
>> +    error ("Could not set environment variable for testing.");
>> +
>> +  gdb_environ env;
>> +
>> +  SELF_CHECK (env.envp ()[0] == NULL);
>> +
>> +  SELF_CHECK (env.get ("PWD") == NULL);
>
> If you add
>
>   env.set ("PWD", "/home");
>
> you should see a crash.

Right.  I'll make sure to add this check as well.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/