From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 48820 invoked by alias); 29 Jul 2017 22:36:28 -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 48805 invoked by uid 89); 29 Jul 2017 22:36:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 29 Jul 2017 22:36:25 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v6TMaINo000927 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sat, 29 Jul 2017 18:36:23 -0400 Received: by simark.ca (Postfix, from userid 112) id E20AD1EA01; Sat, 29 Jul 2017 18:36:18 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 1A21C1E043; Sat, 29 Jul 2017 18:35:58 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sat, 29 Jul 2017 22:36:00 -0000 From: Simon Marchi To: Sergio Durigan Junior 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 In-Reply-To: <20170727033531.23066-1-sergiodj@redhat.com> References: <20170629194106.23070-1-sergiodj@redhat.com> <20170727033531.23066-1-sergiodj@redhat.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sat, 29 Jul 2017 22:36:19 +0000 X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00434.txt.bz2 Hi Sergio, I took a closer look at set/unset, I have a few comments. > @@ -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. > + > + 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. > + > +/* 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, Simon