From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61658 invoked by alias); 31 Aug 2017 20:34:50 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 61640 invoked by uid 89); 31 Aug 2017 20:34:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 31 Aug 2017 20:34:47 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 16D9B85550; Thu, 31 Aug 2017 20:34:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 16D9B85550 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D2DEFBF640; Thu, 31 Aug 2017 20:34:45 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Pedro Alves , Simon Marchi Subject: Re: [PATCH v4] Implement the ability to set/unset environment variables to GDBserver when starting the inferior References: <20170629194106.23070-1-sergiodj@redhat.com> <20170828231315.14952-1-sergiodj@redhat.com> <28c9905c-b845-a1e7-368e-fc4631e59218@ericsson.com> Date: Thu, 31 Aug 2017 20:34:00 -0000 In-Reply-To: <28c9905c-b845-a1e7-368e-fc4631e59218@ericsson.com> (Simon Marchi's message of "Thu, 31 Aug 2017 21:37:44 +0200") Message-ID: <87wp5j5vdm.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00544.txt.bz2 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 &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/