Re: [PATCH] Implement the ability to transmit environment variables to GDBserver when starting the inferior

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

Thanks for the review, Simon.  Comments below.

On Thursday, July 13 2017, Simon Marchi wrote:

> On 2017-06-29 21:41, Sergio Durigan Junior wrote:
>> This is the first version of this patch, which aims to implement the
>> ability to transmit user-set environment variables to GDBserver when
>> starting the remote inferior.
>>
>> User-set environment variables are only the variables that are
>> explicitly set by the user, using the 'set environment' command.  This
>> means that variables that were already present in the environment when
>> starting GDB/GDBserver are not transmitted/considered by this feature.
>>
>> The idea behind this patch is to store user-set environment variables
>> in a separate vector, part of gdb_environ, and provide this list to
>> extended_remote_create_inferior when the preparations to start the
>> inferior are happening.  extended_remote_create_inferior, then,
>> iterates over this list and sends a new packet,
>> QEnvironmentHexEncoded, which contains the hex-encoded string that
>> represents the "VAR=VALUE" format (VALUE can be empty if the user set
>> a variable with a null value, by doing 'set environment VAR=').
>>
>> The QEnvironmentHexEncoded packet is inspired on LLDB's extensions to
>> the RSP.  Details about it can be seen here:
>>
>>
>> <https://raw.githubusercontent.com/llvm-mirror/lldb/master/docs/lldb-gdb-remote.txt>
>>
>> I decided not to implement the QEnvironment packet because it is
>> considered deprecated by LLDB.  This packet, on LLDB, serves the same
>> purpose of QEnvironmentHexEncoded, but sends the information using a
>> plain text, non-hex-encoded string.
>>
>> This patch also includes updates to the documentation, testsuite, and
>> unit tests, without introducing regressions.
>>
>> gdb/ChangeLog:
>> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	* NEWS (Changes since GDB 8.0): Add entry mentioning new support
>> 	for sending environment variables to GDBserver.
>> 	(New remote packets): Add entry of QEnvironmentHexEncoded.
>> 	* common/environ.c (gdb_environ::operator=): Extend method to
>> 	handle m_user_env_list.
>> 	(gdb_environ::clear): Likewise.
>> 	(match_var_in_string): Change type of first parameter from 'char
>> 	*' to 'const char *'.
>> 	(gdb_environ::get): New variable 'fullvar'.  Handle
>> 	m_user_env_list.
>> 	(gdb_environ::unset): Extend method to handle m_user_env_list.
>> 	(gdb_environ::user_envp): New method.
>> 	* common/environ.h (gdb_environ): Add NULL to m_user_env_list;
>> 	handle m_user_env_list on move constructor/assignment.
>> 	(user_envp): New method.
>> 	(m_user_env_list): New vector.
>> 	* remote.c: Include "environ.h". Add QEnvironmentHexEncoded.
>> 	(remote_protocol_features): Add
>> 	QEnvironmentHexEncoded packet.
>> 	(extended_remote_environment_support): New function.
>> 	(extended_remote_create_inferior): Call
>> 	extended_remote_environment_support.
>> 	(_initialize_remote): Add QEnvironmentHexEncoded packet config.
>> 	* unittests/environ-selftests.c (run_tests): Add tests for
>> 	m_user_env_list.
>>
>> gdb/gdbserver/ChangeLog:
>> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	* server.c (handle_general_set): Handle QEnvironmentHexEncoded
>> 	packet.
>> 	(handle_query): Inform remote that QEnvironmentHexEncoded is
>> 	supported.
>>
>> gdb/doc/ChangeLog:
>> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	* gdb.texinfo (set environment): Add @anchor.  Explain that
>> 	environment variables set by the user are sent to GDBserver.
>> 	(Connecting) <Remote Packet>: Add "environment-hex-encoded"
>> 	and "QEnvironmentHexEncoded" to the table.
>> 	(Remote Protocol) <QEnvironmentHexEncoded>: New item, explaining
>> 	the packet.
>>
>> gdb/testsuite/ChangeLog:
>> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	* gdb.base/share-env-with-gdbserver.c: New file.
>> 	* gdb.base/share-env-with-gdbserver.exp: Likewise.
>> ---
>>  gdb/NEWS                                           |  12 +++
>>  gdb/common/environ.c                               |  59 +++++++++---
>>  gdb/common/environ.h                               |  14 ++-
>>  gdb/doc/gdb.texinfo                                |  45 +++++++++
>>  gdb/gdbserver/server.c                             |  54 ++++++++++-
>>  gdb/remote.c                                       |  50 ++++++++++
>>  gdb/testsuite/gdb.base/share-env-with-gdbserver.c  |  40 ++++++++
>>  .../gdb.base/share-env-with-gdbserver.exp          | 105
>> +++++++++++++++++++++
>>  gdb/unittests/environ-selftests.c                  |  30 ++++++
>>  9 files changed, 395 insertions(+), 14 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.base/share-env-with-gdbserver.c
>>  create mode 100644 gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
>>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index 7c8a8f6..9026733 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,14 @@
>>
>>  *** Changes since GDB 8.0
>>
>> +* On Unix systems, GDB now supports transmitting environment variables
>> +  to GDBserver that are to be passed to the inferior when it is being
>> +  started.
>> +
>> +  To inform GDB of environment variables that are to be transmitted to
>> +  GDBserver, use the "set environment" command.  Only user set
>> +  environment variables are sent to GDBserver.
>> +
>>  * On Unix systems, GDBserver now does globbing expansion and variable
>>    substitution in inferior command line arguments.
>>
>> @@ -14,6 +22,10 @@
>>
>>  * New remote packets
>>
>> +QEnvironmentHexEncoded
>> +  Inform GDBserver of an environment variable that is to be passed to
>> +  the inferior when starting it.
>> +
>>  QStartupWithShell
>>    Indicates whether the inferior must be started with a shell or not.
>>
>> diff --git a/gdb/common/environ.c b/gdb/common/environ.c
>> index 698bda3..da0af98 100644
>> --- a/gdb/common/environ.c
>> +++ b/gdb/common/environ.c
>> @@ -30,8 +30,11 @@ gdb_environ::operator= (gdb_environ &&e)
>>      return *this;
>>
>>    m_environ_vector = std::move (e.m_environ_vector);
>> +  m_user_env_list = std::move (e.m_user_env_list);
>>    e.m_environ_vector.clear ();
>> +  e.m_user_env_list.clear ();
>>    e.m_environ_vector.push_back (NULL);
>> +  e.m_user_env_list.push_back (NULL);
>>    return *this;
>>  }
>>
>> @@ -63,8 +66,10 @@ gdb_environ::clear ()
>>    for (char *v : m_environ_vector)
>>      xfree (v);
>>    m_environ_vector.clear ();
>> +  m_user_env_list.clear ();
>>    /* Always add the NULL element.  */
>>    m_environ_vector.push_back (NULL);
>> +  m_user_env_list.push_back (NULL);
>>  }
>>
>>  /* Helper function to check if STRING contains an environment variable
>> @@ -72,7 +77,7 @@ gdb_environ::clear ()
>>     if it contains, false otherwise.  */
>>
>>  static bool
>> -match_var_in_string (char *string, const char *var, size_t var_len)
>> +match_var_in_string (const char *string, const char *var, size_t
>> var_len)
>>  {
>>    if (strncmp (string, var, var_len) == 0 && string[var_len] == '=')
>>      return true;
>> @@ -99,12 +104,14 @@ 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);
>>
>>    /* 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);
>> +  m_user_env_list.insert (m_user_env_list.end () - 1, fullvar);
>>  }
>>
>>  /* See common/environ.h.  */
>> @@ -113,18 +120,38 @@ void
>>  gdb_environ::unset (const char *var)
>>  {
>>    size_t len = strlen (var);
>> +  std::vector<char *>::iterator it_env;
>> +  std::vector<const char *>::iterator it_user_env;
>> +  char *found_var;
>>
>>    /* 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;
>> +    }
>> +
>> +  found_var = *it_env;
>> +
>> +  /* We iterate until '.end () - 1' because the last element is
>> +     always NULL.  */
>> +  for (it_user_env = m_user_env_list.begin ();
>> +       it_user_env != m_user_env_list.end () - 1;
>> +       ++it_user_env)
>> +    if (match_var_in_string (*it_user_env, var, len))
>> +      break;
>
> In this second loop, can you search by pointer value instead?  A value
> in m_user_env_list will always have a duplicate value in
> m_environ_vector, right?  If so, std::remove might be a good option.

That's true.  I'll do that, thanks for the tip.

>> +
>> +  m_environ_vector.erase (it_env);
>> +  if (it_user_env != m_user_env_list.end () - 1)
>> +    m_user_env_list.erase (it_user_env);
>> +  xfree (found_var);
>>  }
>>
>>  /* See common/environ.h.  */
>> @@ -134,3 +161,11 @@ gdb_environ::envp () const
>>  {
>>    return const_cast<char **> (&m_environ_vector[0]);
>>  }
>> +
>> +/* See common/environ.h.  */
>> +
>> +const char **
>> +gdb_environ::user_envp () const
>> +{
>> +  return const_cast<const char **> (&m_user_env_list[0]);
>> +}
>
> For m_environ_p, we have the envp method the way it is because we have
> to pass it to execve.  We don't have such limitation for
> m_user_env_list.  We don't need to add a NULL element, and user_envp
> can return a const reference to the vector instead.

I was thinking about that, but was unsure if returning a direct
reference to the private vector was a good idea.  One of my first drafts
even implemented it this way, but then I changed things.  But I guess
you're right; there's no harm in returning a *const* reference to the
vector.  I'll change the code accordingly.

>> diff --git a/gdb/common/environ.h b/gdb/common/environ.h
>> index 0bbb191..cce7f87 100644
>> --- a/gdb/common/environ.h
>> +++ b/gdb/common/environ.h
>> @@ -32,6 +32,7 @@ public:
>>         If/when we add more variables to it, NULL will always be the
>>         last element.  */
>>      m_environ_vector.push_back (NULL);
>> +    m_user_env_list.push_back (NULL);
>>    }
>>
>>    ~gdb_environ ()
>> @@ -41,12 +42,15 @@ public:
>>
>>    /* Move constructor.  */
>>    gdb_environ (gdb_environ &&e)
>> -    : m_environ_vector (std::move (e.m_environ_vector))
>> +    : m_environ_vector (std::move (e.m_environ_vector)),
>> +      m_user_env_list (std::move (e.m_user_env_list))
>>    {
>>      /* Make sure that the moved-from vector is left at a valid
>>         state (only one NULL element).  */
>>      e.m_environ_vector.clear ();
>> +    e.m_user_env_list.clear ();
>>      e.m_environ_vector.push_back (NULL);
>> +    e.m_user_env_list.push_back (NULL);
>>    }
>>
>>    /* Move assignment.  */
>> @@ -73,9 +77,17 @@ public:
>>    /* Return the environment vector represented as a 'char **'.  */
>>    char **envp () const;
>>
>> +  /* Return the user-set environment vector represented as a 'const
>> +     char **'.  */
>> +  const char **user_envp () const;
>> +
>>  private:
>>    /* A vector containing the environment variables.  */
>>    std::vector<char *> m_environ_vector;
>> +
>> +  /* The vector containing the environment variables set by the
>> +     user.  */
>> +  std::vector<const char *> m_user_env_list;
>>  };
>>
>>  #endif /* defined (ENVIRON_H) */
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index c167a86..d632523 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -2363,6 +2363,7 @@ print the names and values of all environment
>> variables to be given to
>>  your program.  You can abbreviate @code{environment} as @code{env}.
>>
>>  @kindex set environment
>> +@anchor{set environment}
>>  @item set environment @var{varname} @r{[}=@var{value}@r{]}
>>  Set environment variable @var{varname} to @var{value}.  The value
>>  changes for your program (and the shell @value{GDBN} uses to launch
>> @@ -2391,6 +2392,10 @@ If necessary, you can avoid that by using the
>> @samp{env} program as a
>>  wrapper instead of using @code{set environment}.  @xref{set
>>  exec-wrapper}, for an example doing just that.
>>
>> +Environment variables that are set by the user are also transmitted to
>> +@command{gdbserver} to be used when starting the remote inferior.
>> +@pxref{QEnvironmentHexEncoded}.
>> +
>>  @kindex unset environment
>>  @item unset environment @var{varname}
>>  Remove variable @var{varname} from the environment to be passed to
>> your
>> @@ -20816,6 +20821,10 @@ are:
>>  @tab @code{QStartupWithShell}
>>  @tab @code{set startup-with-shell}
>>
>> +@item @code{environment-hex-encoded}
>> +@tab @code{QEnvironmentHexEncoded}
>> +@tab @code{set environment}
>> +
>>  @item @code{conditional-breakpoints-packet}
>>  @tab @code{Z0 and Z1}
>>  @tab @code{Support for target-side breakpoint condition evaluation}
>> @@ -36480,6 +36489,42 @@ actually support starting the inferior
>> using a shell.
>>  Use of this packet is controlled by the @code{set startup-with-shell}
>>  command; @pxref{set startup-with-shell}.
>>
>> +@item QEnvironmentHexEncoded:@var{hex-value}
>> +@anchor{QEnvironmentHexEncoded}
>> +@cindex environment variable, remote request
>> +@cindex @samp{QEnvironmentHexEncoded} packet
>> +On UNIX-like targets, it is possible to set environment variables that
>> +will be passed to the inferior during the startup process.  This
>> +packet is used to inform @command{gdbserver} of an environment
>> +variable that has been defined by the user on @value{GDBN} (@pxref{set
>> +environment}).
>> +
>> +The packet is composed by @var{hex-value}, an hex encoded
>> +representation of the @var{name=value} format representing an
>> +environment variable.  The name of the environment variable is
>> +represented by @var{name}, and the value to be assigned to the
>> +environment variable is represented by @var{value}.  If the variable
>> +has no value (i.e., the value is @code{null}), then @var{value} will
>> +not be present.
>> +
>> +This packet is only available in extended mode (@pxref{extended
>> +mode}).
>> +
>> +Reply:
>> +@table @samp
>> +@item OK
>> +The request succeeded.
>> +@end table
>> +
>> +This packet is not probed by default; the remote stub must request it,
>> +by supplying an appropriate @samp{qSupported} response
>> +(@pxref{qSupported}).  This should only be done on targets that
>> +actually support passing environment variables to the starting
>> +inferior.
>> +
>> +This packet is related to the @code{set environment} command;
>> +@pxref{set environment}.
>> +
>>  @item qfThreadInfo
>>  @itemx qsThreadInfo
>>  @cindex list active threads, remote request
>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
>> index 3838351..622898d 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -631,6 +631,57 @@ handle_general_set (char *own_buf)
>>        return;
>>      }
>>
>> +  if (startswith (own_buf, "QEnvironmentHexEncoded:"))
>> +    {
>> +      const char *p = own_buf + sizeof ("QEnvironmentHexEncoded:")
>> - 
>> 1;
>> +      /* The final form of the environment variable.  FINAL_VAR will
>> +	 hold the 'VAR=VALUE' format.  */
>> +      char *final_var = (char *) xcalloc (1, strlen (p) * 2);
>> +      struct cleanup *c = make_cleanup (xfree, final_var);
>> +      /* VAR_NAME will hold the environment variable name; VAR_VALUE
>> +	 will hold its value.  These will be just pointers to
>> +	 FINAL_VAR.  */
>> +      char *var_name, *var_value;
>> +
>> +      if (remote_debug)
>> +	debug_printf (_("[QEnvironmentHexEncoded received '%s']\n"), p);
>> +
>> +      /* Un-hexify the string received from the remote, which should
>> +	 be in the form 'VAR=VALUE'.  */
>> +      hex2bin (p, (gdb_byte *) final_var, strlen (p));
>> +
>> +      if (remote_debug)
>> +	{
>> +	  debug_printf (_("[Environment variable to be set: '%s']\n"),
>> +			final_var);
>> +	  debug_flush ();
>> +	}
>> +
>> +      var_name = final_var;
>> +      var_value = strchr (final_var, '=');
>> +      /* There should always be an equal sign, even if the variable is
>> +	 empty.  */
>> +      if (var_value == NULL)
>> +	{
>> +	  warning (_("Unknown environment variable '%s' from remote side."),
>> +		   final_var);
>> +	  write_enn (own_buf);
>> +	  do_cleanups (c);
>> +	  return;
>> +	}
>> +      /* Mark the end of the variable's name.  */
>> +      *var_value = '\0';
>> +      /* And skip the '=' sign (which now is the '\0').  */
>> +      ++var_value;
>> +
>> +      /* Finally, set the variable to be passed to the inferior.  */
>> +      our_environ.set (var_name, var_value);
>> +
>> +      write_ok (own_buf);
>> +      do_cleanups (c);
>> +      return;
>> +    }
>> +
>>    if (strcmp (own_buf, "QStartNoAckMode") == 0)
>>      {
>>        if (remote_debug)
>> @@ -2228,7 +2279,8 @@ handle_query (char *own_buf, int packet_len, int
>> *new_packet_len_p)
>>  	}
>>
>>        sprintf (own_buf,
>> -
>> "PacketSize=%x;QPassSignals+;QProgramSignals+;QStartupWithShell+",
>> +	       "PacketSize=%x;QPassSignals+;QProgramSignals+;"
>> +	       "QStartupWithShell+;QEnvironmentHexEncoded+",
>>  	       PBUFSIZ - 1);
>>
>>        if (target_supports_catch_syscall ())
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 8e8ee6f..8be6fd1 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -72,6 +72,7 @@
>>  #include "btrace.h"
>>  #include "record-btrace.h"
>>  #include <algorithm>
>> +#include "environ.h"
>>
>>  /* Temp hacks for tracepoint encoding migration.  */
>>  static char *target_buf;
>> @@ -1429,6 +1430,7 @@ enum {
>>    PACKET_QCatchSyscalls,
>>    PACKET_QProgramSignals,
>>    PACKET_QStartupWithShell,
>> +  PACKET_QEnvironmentHexEncoded,
>>    PACKET_qCRC,
>>    PACKET_qSearch_memory,
>>    PACKET_vAttach,
>> @@ -4636,6 +4638,8 @@ static const struct protocol_feature
>> remote_protocol_features[] = {
>>      PACKET_QProgramSignals },
>>    { "QStartupWithShell", PACKET_DISABLE, remote_supported_packet,
>>      PACKET_QStartupWithShell },
>> +  { "QEnvironmentHexEncoded", PACKET_DISABLE, remote_supported_packet,
>> +    PACKET_QEnvironmentHexEncoded },
>>    { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
>>      PACKET_QStartNoAckMode },
>>    { "multiprocess", PACKET_DISABLE, remote_supported_packet,
>> @@ -9585,6 +9589,44 @@ extended_remote_run (const std::string &args)
>>      }
>>  }
>>
>> +/* Helper function to handle the QEnvironmentHexEncoded packet and
>> +   send the user-defined environment variables to the remote.  */
>> +
>> +static void
>> +extended_remote_environment_support (struct remote_state *rs)
>> +{
>> +  gdb_environ *e = &current_inferior ()->environment;
>> +  const char **user_envp = e->user_envp ();
>> +
>> +  for (int i = 0; user_envp[i] != NULL; ++i)
>> +    {
>> +      char *encoded_fullvar;
>> +      const char *fullvar = user_envp[i];
>> +      struct cleanup *c;
>> +
>> +      encoded_fullvar = (char *) xmalloc (get_remote_packet_size ());
>> +      c = make_cleanup (xfree, encoded_fullvar);
>> +
>> +      /* Convert the environment variable to an hex string, which
>> +	 is the best format to be transmitted over the wire.  */
>> +      bin2hex ((gdb_byte *) fullvar, encoded_fullvar, strlen
>> (fullvar));
>
> A bin2hex version that returns an std::string would be nice :)

Wouldn't it?  ;-)

I'll see what can be done.

>> +
>> +      xsnprintf (rs->buf, get_remote_packet_size (),
>> +		 "QEnvironmentHexEncoded:%s", encoded_fullvar);
>> +
>> +      if (remote_debug)
>> +	fprintf_unfiltered (gdb_stdlog, _("sending packet '%s'\n"),
>> +			    rs->buf);
>
> Is this useful?  When "set debug remote" is active, we already have
> the packets printed out, isn't it the same thing?

Maybe change the message to print the variable name and its value
instead, then?  Looking at this now, it doesn't really make sense to
print the full hexified packet.

>> +
>> +      putpkt (rs->buf);
>> +      getpkt (&rs->buf, &rs->buf_size, 0);
>> +      if (strcmp (rs->buf, "OK") != 0)
>> +	warning (_("Unable to set environment variable '%s' on remote."),
>> +		 fullvar);
>> +      do_cleanups (c);
>> +    }
>> +}
>> +
>>  /* In the extended protocol we want to be able to do things like
>>     "run" and have them basically work as expected.  So we need
>>     a special create_inferior function.  We support changing the
>> @@ -9625,6 +9667,9 @@ Remote replied unexpectedly while setting
>> startup-with-shell: %s"),
>>  	       rs->buf);
>>      }
>>
>> +  if (packet_support (PACKET_QEnvironmentHexEncoded) !=
>> PACKET_DISABLE)
>> +    extended_remote_environment_support (rs);
>
> It doesn't make a big difference, but I would prefer the check for
> packet support in extended_remove_environment_support(), closer to the
> code its related to.  If we have to re-use that function for some
> reason, the check will be already there so we can't forget it.

Sure, no problem.

> One thing that worries me about the protocol is that there doesn't
> seem to be a way to unset an environment variable (empty value is
> different than unset right?).  It doesn't seem possible to start an
> inferior with just A=1 and another with just B=2.  Once you set A=1,
> it's there for good.

Yeah, this was something in my mind since the beginning as well.  My
plan, which I should have made clearer, is to tackle the "variable
setting" first, 

> So do we need another packet for GDB to tell GDBserver "forget about
> all variables set with QEnvironmentHexEncoded previously", like
> QEnvironmentReset?  GDB would send that just before setting the
> variables again with QEnvironmentHexEncoded.

Not only that, right?  I think we could have another packet,
QEnvironmentUnset, which could also be used to unset specific variables.
For example, suppose that the user wants to unset an environment
variable that was set previously (i.e., before starting GDB).  After
issuing an "unset env VAR" inside GDB, it makes sense to replicate this
to gdbserver as well.

I'll take a look at adding these other packets to the protocol and
extending the patch.

Comments about the testcase are also welcome, when you have a chance to
look at it.

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/