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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Thursday, August 31 2017, Simon Marchi wrote:

>> diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c
>> index 1e938e6746..e2d8c70985 100644
>> --- a/gdb/unittests/environ-selftests.c
>> +++ b/gdb/unittests/environ-selftests.c
>> @@ -22,132 +22,257 @@
>>  #include "common/environ.h"
>>  #include "common/diagnostics.h"
>>  
>> +static bool
>> +set_contains (const std::set<std::string> &set, std::string key)
>> +{
>> +  return set.find (key) != set.end ();
>> +}
>> +
>>  namespace selftests {
>>  namespace gdb_environ_tests {
>>  
>> +/* Test if the vector is initialized in a correct way.  */
>> +
>>  static void
>> -run_tests ()
>> +test_vector_initialization (gdb_environ *env)
>>  {
>> -  /* Set a test environment variable.  This will be unset at the end
>> -     of this function.  */
>> -  if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
>> -    error (_("Could not set environment variable for testing."));
>> -
>> -  gdb_environ env;
>> -
>>    /* When the vector is initialized, there should always be one NULL
>>       element in it.  */
>> -  SELF_CHECK (env.envp ()[0] == NULL);
>> +  SELF_CHECK (env->envp ()[0] == NULL);
>> +  SELF_CHECK (env->user_set_env ().size () == 0);
>> +  SELF_CHECK (env->user_unset_env ().size () == 0);
>>  
>>    /* Make sure that there is no other element.  */
>> -  SELF_CHECK (env.get ("PWD") == NULL);
>> +  SELF_CHECK (env->get ("PWD") == NULL);
>> +}
>>  
>> -  /* Check if unset followed by a set in an empty vector works.  */
>> -  env.set ("PWD", "test");
>> -  SELF_CHECK (strcmp (env.get ("PWD"), "test") == 0);
>> -  /* The second element must be NULL.  */
>> -  SELF_CHECK (env.envp ()[1] == NULL);
>> -  env.unset ("PWD");
>> -  SELF_CHECK (env.envp ()[0] == NULL);
>> +/* Test initialization using the host's environ.  */
>>  
>> +static void
>> +test_init_from_host_environ (gdb_environ *env)
>> +{
>>    /* Initialize the environment vector using the host's environ.  */
>> -  env = gdb_environ::from_host_environ ();
>> +  *env = gdb_environ::from_host_environ ();
>> +
>> +  /* The user-set and user-unset lists must be empty.  */
>> +  SELF_CHECK (env->user_set_env ().size () == 0);
>> +  SELF_CHECK (env->user_unset_env ().size () == 0);
>>  
>>    /* Our test environment variable should be present at the
>>       vector.  */
>> -  SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
>> -
>> -  /* Set our test variable to another value.  */
>> -  env.set ("GDB_SELFTEST_ENVIRON", "test");
>> -  SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "test") == 0);
>> -
>> -  /* And unset our test variable.  The variable still exists in the
>> -     host's environment, but doesn't exist in our vector.  */
>> -  env.unset ("GDB_SELFTEST_ENVIRON");
>> -  SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
>> -
>> -  /* Re-set the test variable.  */
>> -  env.set ("GDB_SELFTEST_ENVIRON", "1");
>> -  SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
>> +  SELF_CHECK (strcmp (env->get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
>> +}
>>  
>> -  /* When we clear our environ vector, there should be only one
>> -     element on it (NULL), and we shouldn't be able to get our test
>> -     variable.  */
>> -  env.clear ();
>> -  SELF_CHECK (env.envp ()[0] == NULL);
>> -  SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON") == NULL);
>> +/* Test reinitialization using the host's environ.  */
>>  
>> +static void
>> +test_reinit_from_host_environ (gdb_environ *env)
>> +{
>>    /* Reinitialize our environ vector using the host environ.  We
>>       should be able to see one (and only one) instance of the test
>>       variable.  */
>> -  env = gdb_environ::from_host_environ ();
>> -  char **envp = env.envp ();
>> +  *env = gdb_environ::from_host_environ ();
>> +  char **envp = env->envp ();
>>    int num_found = 0;
>>  
>>    for (size_t i = 0; envp[i] != NULL; ++i)
>>      if (strcmp (envp[i], "GDB_SELFTEST_ENVIRON=1") == 0)
>>        ++num_found;
>>    SELF_CHECK (num_found == 1);
>> +}
>>  
>> -  /* Get rid of our test variable.  */
>> -  unsetenv ("GDB_SELFTEST_ENVIRON");
>> +/* Test the case when we set a variable A, then set a variable B,
>> +   then unset A, and make sure that we cannot find A in the environ
>> +   vector, but can still find B.  */
>> +
>> +static void
>> +test_set_A_unset_B_unset_A_cannot_find_A_can_find_B (gdb_environ *env)
>> +{
>> +  env->set ("GDB_SELFTEST_ENVIRON_1", "aaa");
>> +  SELF_CHECK (strcmp (env->get ("GDB_SELFTEST_ENVIRON_1"), "aaa") == 0);
>> +  /* User-set environ var list must contain one element.  */
>> +  SELF_CHECK (env->user_set_env ().size () == 1);
>> +  SELF_CHECK (set_contains (env->user_set_env (),
>> +			    std::string ("GDB_SELFTEST_ENVIRON_1=aaa")));
>>  
>> -  /* Test the case when we set a variable A, then set a variable B,
>> -     then unset A, and make sure that we cannot find A in the environ
>> -     vector, but can still find B.  */
>> -  env.set ("GDB_SELFTEST_ENVIRON_1", "aaa");
>> -  SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_1"), "aaa") == 0);
>> +  env->set ("GDB_SELFTEST_ENVIRON_2", "bbb");
>> +  SELF_CHECK (strcmp (env->get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
>>  
>> -  env.set ("GDB_SELFTEST_ENVIRON_2", "bbb");
>> -  SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
>> +  env->unset ("GDB_SELFTEST_ENVIRON_1");
>> +  SELF_CHECK (env->get ("GDB_SELFTEST_ENVIRON_1") == NULL);
>> +  SELF_CHECK (strcmp (env->get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
>>  
>> -  env.unset ("GDB_SELFTEST_ENVIRON_1");
>> -  SELF_CHECK (env.get ("GDB_SELFTEST_ENVIRON_1") == NULL);
>> -  SELF_CHECK (strcmp (env.get ("GDB_SELFTEST_ENVIRON_2"), "bbb") == 0);
>> +  /* The user-set environ var list must contain only one element
>> +     now.  */
>> +  SELF_CHECK (set_contains (env->user_set_env (),
>> +			    std::string ("GDB_SELFTEST_ENVIRON_2=bbb")));
>> +  SELF_CHECK (env->user_set_env ().size () == 1);
>> +}
>>  
>> -  env.clear ();
>> +/* Check if unset followed by a set in an empty vector works.  */
>>  
>> -  /* Test that after a std::move the moved-from object is left at a
>> -     valid state (i.e., its only element is NULL).  */
>> -  env.set ("A", "1");
>> -  SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>> +static void
>> +test_unset_set_empty_vector (gdb_environ *env)
>> +{
>> +  env->set ("PWD", "test");
>> +  SELF_CHECK (strcmp (env->get ("PWD"), "test") == 0);
>> +  SELF_CHECK (set_contains (env->user_set_env (), std::string ("PWD=test")));
>> +  SELF_CHECK (env->user_unset_env ().size () == 0);
>> +  /* The second element must be NULL.  */
>> +  SELF_CHECK (env->envp ()[1] == NULL);
>> +  SELF_CHECK (env->user_set_env ().size () == 1);
>> +  env->unset ("PWD");
>> +  SELF_CHECK (env->envp ()[0] == NULL);
>> +  SELF_CHECK (env->user_set_env ().size () == 0);
>> +  SELF_CHECK (env->user_unset_env ().size () == 1);
>> +  SELF_CHECK (set_contains (env->user_unset_env (), std::string ("PWD")));
>> +}
>> +
>> +/* When we clear our environ vector, there should be only one
>> +   element on it (NULL), and we shouldn't be able to get our test
>> +   variable.  */
>> +
>> +static void
>> +test_vector_clear (gdb_environ *env)
>> +{
>> +  env->clear ();
>> +  SELF_CHECK (env->envp ()[0] == NULL);
>> +  SELF_CHECK (env->user_set_env ().size () == 0);
>> +  SELF_CHECK (env->user_unset_env ().size () == 0);
>> +  SELF_CHECK (env->get ("GDB_SELFTEST_ENVIRON") == NULL);
>> +}
>> +
>> +/* Test that after a std::move the moved-from object is left at a
>> +   valid state (i.e., its only element is NULL).  */
>> +
>> +static void
>> +test_std_move (gdb_environ *env)
>> +{
>> +  env->set ("A", "1");
>> +  SELF_CHECK (strcmp (env->get ("A"), "1") == 0);
>> +  SELF_CHECK (set_contains (env->user_set_env (), std::string ("A=1")));
>> +  SELF_CHECK (env->user_set_env ().size () == 1);
>>    gdb_environ env2;
>> -  env2 = std::move (env);
>> -  SELF_CHECK (env.envp ()[0] == NULL);
>> +  env2 = std::move (*env);
>> +  SELF_CHECK (env->envp ()[0] == NULL);
>>    SELF_CHECK (strcmp (env2.get ("A"), "1") == 0);
>>    SELF_CHECK (env2.envp ()[1] == NULL);
>> -  env.set ("B", "2");
>> -  SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
>> -  SELF_CHECK (env.envp ()[1] == NULL);
>> +  SELF_CHECK (env->user_set_env ().size () == 0);
>> +  SELF_CHECK (set_contains (env2.user_set_env (), std::string ("A=1")));
>> +  SELF_CHECK (env2.user_set_env ().size () == 1);
>> +  env->set ("B", "2");
>> +  SELF_CHECK (strcmp (env->get ("B"), "2") == 0);
>> +  SELF_CHECK (env->envp ()[1] == NULL);
>> +}
>>  
>> -  /* Test that the move constructor leaves everything at a valid
>> -     state.  */
>> -  env.clear ();
>> -  env.set ("A", "1");
>> -  SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>> -  gdb_environ env3 = std::move (env);
>> -  SELF_CHECK (env.envp ()[0] == NULL);
>> +/* Test that the move constructor leaves everything at a valid
>> +   state.  */
>> +
>> +static void
>> +test_move_constructor (gdb_environ *env)
>> +{
>> +  env->clear ();
>> +  env->set ("A", "1");
>> +  SELF_CHECK (strcmp (env->get ("A"), "1") == 0);
>> +  SELF_CHECK (set_contains (env->user_set_env (), std::string ("A=1")));
>> +  gdb_environ env3 = std::move (*env);
>> +  SELF_CHECK (env->envp ()[0] == NULL);
>> +  SELF_CHECK (env->user_set_env ().size () == 0);
>>    SELF_CHECK (strcmp (env3.get ("A"), "1") == 0);
>>    SELF_CHECK (env3.envp ()[1] == NULL);
>> -  env.set ("B", "2");
>> -  SELF_CHECK (strcmp (env.get ("B"), "2") == 0);
>> -  SELF_CHECK (env.envp ()[1] == NULL);
>> +  SELF_CHECK (set_contains (env3.user_set_env (), std::string ("A=1")));
>> +  SELF_CHECK (env3.user_set_env ().size () == 1);
>> +  env->set ("B", "2");
>> +  SELF_CHECK (strcmp (env->get ("B"), "2") == 0);
>> +  SELF_CHECK (env->envp ()[1] == NULL);
>> +  SELF_CHECK (set_contains (env->user_set_env (), std::string ("B=2")));
>> +  SELF_CHECK (env->user_set_env ().size () == 1);
>> +}
>> +
>> +/* Test that self-moving works.  */
>>  
>> +static void
>> +test_self_move (gdb_environ *env)
>> +{
>>    /* Test self-move.  */
>> -  env.clear ();
>> -  env.set ("A", "1");
>> -  SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>> +  env->clear ();
>> +  env->set ("A", "1");
>> +  SELF_CHECK (strcmp (env->get ("A"), "1") == 0);
>> +  SELF_CHECK (set_contains (env->user_set_env (), std::string ("A=1")));
>> +  SELF_CHECK (env->user_set_env ().size () == 1);
>>  
>>    /* Some compilers warn about moving to self, but that's precisely what we want
>>       to test here, so turn this warning off.  */
>>    DIAGNOSTIC_PUSH
>>    DIAGNOSTIC_IGNORE_SELF_MOVE
>> -  env = std::move (env);
>> +  *env = std::move (*env);
>>    DIAGNOSTIC_POP
>>  
>> -  SELF_CHECK (strcmp (env.get ("A"), "1") == 0);
>> -  SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0);
>> -  SELF_CHECK (env.envp ()[1] == NULL);
>> +  SELF_CHECK (strcmp (env->get ("A"), "1") == 0);
>> +  SELF_CHECK (strcmp (env->envp ()[0], "A=1") == 0);
>> +  SELF_CHECK (env->envp ()[1] == NULL);
>> +  SELF_CHECK (set_contains (env->user_set_env (), std::string ("A=1")));
>> +  SELF_CHECK (env->user_set_env ().size () == 1);
>> +}
>> +
>> +/* Test if set/unset/reset works.  */
>> +
>> +static void
>> +test_set_unset_reset (gdb_environ *env)
>> +{
>> +  /* Set our test variable to another value.  */
>> +  env->set ("GDB_SELFTEST_ENVIRON", "test");
>> +  SELF_CHECK (strcmp (env->get ("GDB_SELFTEST_ENVIRON"), "test") == 0);
>> +  SELF_CHECK (env->user_set_env ().size () == 1);
>> +  SELF_CHECK (env->user_unset_env ().size () == 0);
>> +
>> +  /* And unset our test variable.  The variable still exists in the
>> +     host's environment, but doesn't exist in our vector.  */
>> +  env->unset ("GDB_SELFTEST_ENVIRON");
>> +  SELF_CHECK (env->get ("GDB_SELFTEST_ENVIRON") == NULL);
>> +  SELF_CHECK (env->user_unset_env ().size () == 1);
>> +  SELF_CHECK (set_contains (env->user_unset_env (),
>> +			    std::string ("GDB_SELFTEST_ENVIRON")));
>> +
>> +  /* Re-set the test variable.  */
>> +  env->set ("GDB_SELFTEST_ENVIRON", "1");
>> +  SELF_CHECK (strcmp (env->get ("GDB_SELFTEST_ENVIRON"), "1") == 0);
>> +}
>> +
>> +static void
>> +run_tests ()
>> +{
>> +  /* Set a test environment variable.  This will be unset at the end
>> +     of this function.  */
>> +  if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
>> +    error (_("Could not set environment variable for testing."));
>> +
>> +  gdb_environ env;
>> +
>> +  test_vector_initialization (&env);
>> +
>> +  test_unset_set_empty_vector (&env);
>> +
>> +  test_init_from_host_environ (&env);
>> +
>> +  test_set_unset_reset (&env);
>> +
>> +  test_vector_clear (&env);
>> +
>> +  test_reinit_from_host_environ (&env);
>> +
>> +  /* Get rid of our test variable.  We won't need it anymore.  */
>> +  unsetenv ("GDB_SELFTEST_ENVIRON");
>> +
>> +  test_set_A_unset_B_unset_A_cannot_find_A_can_find_B (&env);
>> +
>> +  env.clear ();
>> +
>> +  test_std_move (&env);
>> +
>> +  test_move_constructor (&env);
>> +
>> +  test_self_move (&env);
>>  }
>>  } /* namespace gdb_environ */
>>  } /* namespace selftests */
>> 
>
> Hi Sergio,
>
> The main point of splitting that long test into separate functions is to have each
> of them independent from each other.  So they should ideally not use the same environment
> object.  I suggest doing something like this (applied on top of your patch).  I did this
> super quickly, so please check that the tests still make sense :)

Ah, OK.  I guess I didn't understand that, sorry.

Anyway, thanks for the patch.  Yes, from what I have checked it covers
the existing tests.  I'd probably even go further and do a
setenv/unsetenv inside each function that needs them, so as to really
isolate things.

Do you prefer me to apply your patch on top of mine and submit a v5?

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/