This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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<const char *>::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<char *>::iterator it_env;

   /* We iterate until '.end () - 1' because the last element is
      always NULL.  */
-  for (std::vector<char *>::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<const char *>::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<const char *> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]