Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior

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

On Saturday, July 29 2017, Simon Marchi wrote:

> Hi Sergio,
>
> I took a closer look at set/unset, I have a few comments.

Thanks, Simon.

>> @@ -99,32 +107,104 @@ gdb_environ::get (const char *var) const
>>  void
>>  gdb_environ::set (const char *var, const char *value)
>>  {
>> +  char *fullvar = concat (var, "=", value, NULL);
>> +
>>    /* We have to unset the variable in the vector if it exists.  */
>> -  unset (var);
>> +  unset (var, false);
>>
>>    /* Insert the element before the last one, which is always NULL.  */
>> -  m_environ_vector.insert (m_environ_vector.end () - 1,
>> -			   concat (var, "=", value, NULL));
>> +  m_environ_vector.insert (m_environ_vector.end () - 1, fullvar);
>> +
>> +  /* Mark this environment variable as having been set by the user.
>> +     This will be useful when we deal with setting environment
>> +     variables on the remote target.  */
>> +  m_user_set_env_list.push_back (fullvar);
>> +
>> +  /* If this environment variable is marked as unset by the user, then
>> +     remove it from the list, because now the user wants to set
>> +     it.  */
>> +  for (std::vector<const char *>::iterator iter_user_unset
>> +	 = m_user_unset_env_list.begin ();
>> +       iter_user_unset != m_user_unset_env_list.end ();
>> +       ++iter_user_unset)
>> +    if (strcmp (var, *iter_user_unset) == 0)
>> +      {
>> +	void *v = (void *) *iter_user_unset;
>> +
>> +	m_user_unset_env_list.erase (iter_user_unset);
>> +	xfree (v);
>> +	break;
>> +      }
>>  }
>>
>>  /* See common/environ.h.  */
>>
>>  void
>> -gdb_environ::unset (const char *var)
>> +gdb_environ::unset (const char *var, bool update_unset_list)
>>  {
>>    size_t len = strlen (var);
>> +  std::vector<char *>::iterator it_env;
>>
>>    /* We iterate until '.end () - 1' because the last element is
>>       always NULL.  */
>> -  for (std::vector<char *>::iterator el = m_environ_vector.begin ();
>> -       el != m_environ_vector.end () - 1;
>> -       ++el)
>> -    if (match_var_in_string (*el, var, len))
>> -      {
>> -	xfree (*el);
>> -	m_environ_vector.erase (el);
>> -	break;
>> -      }
>> +  for (it_env = m_environ_vector.begin ();
>> +       it_env != m_environ_vector.end () - 1;
>> +       ++it_env)
>> +    if (match_var_in_string (*it_env, var, len))
>> +      break;
>> +
>> +  if (it_env == m_environ_vector.end () - 1)
>> +    {
>> +      /* No element has been found.  */
>> +      return;
>> +    }
>
> Do we want to support the use case to unset an environment variable
> that is defined on the remote system, but not on the local system?  If
> so, the function should probably not return immediately if the
> variable is not found in the env vector, so that the unset request
> ends up in the list of variables to unset.

I guess I hadn't thought about this use case.  It makes sense to me.  I
will make the proper modifications.

>> +
>> +  std::vector<const char *>::iterator it_user_set_env;
>> +  char *found_var = *it_env;
>> +
>> +  it_user_set_env = std::remove (m_user_set_env_list.begin (),
>> +				 m_user_set_env_list.end (),
>> +				 found_var);
>> +  if (it_user_set_env != m_user_set_env_list.end ())
>> +    {
>> +      /* We found (and removed) the element from the user_set_env
>> +	 vector.  */
>> +      m_user_set_env_list.erase (it_user_set_env,
>> m_user_set_env_list.end ());
>> +    }
>> +
>> +  if (update_unset_list)
>> +    {
>> +      bool found_in_unset = false;
>> +
>> +      for (const char *el : m_user_unset_env_list)
>> +	if (strcmp (el, var) == 0)
>> +	  {
>> +	    found_in_unset = true;
>> +	    break;
>> +	  }
>> +
>> +      if (!found_in_unset)
>> +	m_user_unset_env_list.push_back (xstrdup (var));
>> +    }
>> +
>> +  m_environ_vector.erase (it_env);
>> +  xfree (found_var);
>> +}
>
> I have the feeling that we can reduce the amount of boilerplate code
> in the set and unset methods by using std::set instead of std::vector.
> Performance-wise this may not be very good, since for any reasonable
> amount of variables, the vector would probably be more efficient.  But
> its interface makes the code clearer and lighter, in my opinion.  I
> suppose we could always make something with a set-like interface and
> behavior implemented on top of a vector.

I thought about using std::set, but given that I was recently called out
for doing "premature pessimization", I chose to stick with std::vector.
I agree that for some cases std::set would make things easier to
implement/understand.

>
>> +
>> +/* See common/environ.h.  */
>> +
>> +void
>> +gdb_environ::clear_user_set_env ()
>> +{
>> +  std::vector<const char *> copy = m_user_set_env_list;
>> +
>> +  for (const char *var : copy)
>> +    {
>> +      std::string varname (var);
>> +
>> +      varname.erase (varname.find ('='), std::string::npos);
>> +      unset (varname.c_str (), false);
>> +    }
>
> I am not sure having a method like this is correct.  It is used in
> gdbserver when we want to "reset" the environment, that is forget all
> user-made modifications, both set and unset.  To do it correctly, it
> should include restoring the variables that have been unset or
> overwritten.  But as I said in my other mail, I think the simplest
> solution will be to restore the environment from a backup copy of the
> original env.  If we do that, this method won't be needed.
>
> I have prototyped some of my suggestions to make sure they made sense.
> I thought I would push them somewhere, feel free to use whatever you
> want if that's useful to you:
>
> https://github.com/simark/binutils-gdb/commits/remote-env

Thanks, that's very useful.  I have code for most of what you requested,
but I'll use some ideas from your branch.

Cheers,

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