From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 86845 invoked by alias); 1 Aug 2017 02:43:01 -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 86799 invoked by uid 89); 1 Aug 2017 02:42:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no 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; Tue, 01 Aug 2017 02:42:53 +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 0C48315AFC7; Tue, 1 Aug 2017 02:42:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0C48315AFC7 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.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 892FF60C16; Tue, 1 Aug 2017 02:42:50 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Pedro Alves , Eli Zaretskii Subject: Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior References: <20170629194106.23070-1-sergiodj@redhat.com> <20170727033531.23066-1-sergiodj@redhat.com> Date: Tue, 01 Aug 2017 02:43:00 -0000 In-Reply-To: (Simon Marchi's message of "Sun, 30 Jul 2017 00:35:58 +0200") Message-ID: <87shhc9ffa.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/msg00001.txt.bz2 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::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::iterator it_env; >> >> /* We iterate until '.end () - 1' because the last element is >> always NULL. */ >> - for (std::vector::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::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 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/