[PATCH v4] Implement the ability to set/unset environment variables to GDBserver when starting the inferior
Sergio Durigan Junior
sergiodj@redhat.com
Thu Aug 31 20:34:00 GMT 2017
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/
More information about the Gdb-patches
mailing list