This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v5] C++ify gdb/common/environ.c
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Simon Marchi <simon dot marchi at polymtl dot ca>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, Pedro Alves <palves at redhat dot com>
- Date: Mon, 19 Jun 2017 00:19:16 -0400
- Subject: Re: [PATCH v5] C++ify gdb/common/environ.c
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 36287C0587D6
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 36287C0587D6
- References: <20170413040455.23996-1-sergiodj@redhat.com> <20170616222315.12779-1-sergiodj@redhat.com> <4e43c71a2ac4aa229bb262e18dec668c@polymtl.ca>
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/