Re: [PATCH v3] C++ify gdb/common/environ.c

[Pedro Alves <palves at redhat dot com>]

On 05/01/2017 03:22 AM, Sergio Durigan Junior wrote:

>>> +  this->m_environ_vector.insert (this->m_environ_vector.end () - 1,
>>> +				 xstrdup (std::string (var
>>> +						       + '='
>>> +						       + value).c_str ()));
>>
>> I don't really understand the "m_environ_vector.end () - 1" here.
> 
> This is needed because the last element of m_environ_vector needs to be
> always NULL.  Therefore, this code is basically inserting the new
> variable in the second-to-last position of the vector.  I made a comment
> on top of it to clarify this part.

Ah, OK.

> 
> Also, there are other places where I need to iterate through the
> elements of the vector, and I'm also using the "- 1" in these places.
> I'll put comments where applicable.

OK, I'll take another look when you post it.

>>> -extern struct gdb_environ *make_environ (void);
>>> +class gdb_environ
>>> +{
>>> +public:
>>> +  /* Regular constructor and destructor.  */
>>> +  gdb_environ ();
>>> +  ~gdb_environ ();
>>>  
>>> -extern void free_environ (struct gdb_environ *);
>>> +  /* Reinitialize the environment stored.  This is used when the user
>>> +     wants to delete all environment variables added by him/her.  */
>>> +  void reinit ();
>>
>> This should mention that the initial state is copied from the host's
>> environ.   I think the default ctor could use a similar comment.
>> I was going to suggest to call this clear() instead, to go
>> with the standard containers' terminology, until I realized what
>> "init" really does.  Or maybe even find a more explicit name,
>> reset_from_host_environ or some such.
> 
> Sorry, I guess I wasn't aware of the importance of specifying that the
> variables come from the host.  Somehow I thought this was implied.
> 
> I guess reset_from_host_environ is a good name for the method; my other
> option would be "reinit_using_host_environ", but that's longer.
> 
>> Perhaps an even clearer approach would be to make the default ctor
>> not do a deep copy of the host's "environ", but instead add a
>> static factor method that returned such a new gdb_environ, like:
>>
>> static gdb_environ
>> gdb_environ::from_host_environ ()
>> {
>>    // build/return a gdb_environ that wraps the host's environ global.
>> }
>>
>> Not sure.  Only experimenting would tell.
> 
> Sorry, I'm not sure I understand your suggestion.  Please correct me if
> I'm wrong.
> 
> You're basically saying that the default ctor shouldn't actually do
> anything; instead, the class should have this new from_host_environ
> method which would be the "official" ctor.  The users would then call
> from_host_environ directly when they wanted an instance of the class,
> and the method would be responsible for initializing the object.  Is
> that correct?
> 
> If yes, I think I fail to see the advantage of this method over having a
> normal ctor (aside from explicitly naming the new ctor after the fact
> that we're using the host environ to build the object).

The advantage was all in that "aside" -- to make the code
document itself.  It was totally non-obvious to me at first
that:

  gdb_environ env;

makes a copy of the host's environment.  I just assumed it created
an empty environment.  Probably because in my mind I don't think
it'll make sense for a remote process to inherit the host's
environment variables.

> 
> ... after some reading ...
> 
> Oh, I think I see what you're suggesting.  Instead of building one
> gdb_environ every time someone requests it, we build just one and pass
> it along.  OK, now it makes sense.

That wasn't actually what I was saying.  :-)

> 
>> Speaking of copying the host environ:
>>
>>>  _initialize_mi_cmd_env (void)
>>>  {
>>> -  struct gdb_environ *environment;
>>> +  gdb_environ environment;
>>>    const char *env;
>>>  
>>>    /* We want original execution path to reset to, if desired later.
>>> @@ -278,13 +278,10 @@ _initialize_mi_cmd_env (void)
>>>       current_inferior ()->environment.  Also, there's no obvious
>>>       place where this code can be moved such that it surely run
>>>       before any code possibly mangles original PATH.  */
>>> -  environment = make_environ ();
>>> -  init_environ (environment);
>>> -  env = get_in_environ (environment, path_var_name);
>>> +  env = environment.get (path_var_name);
>>>  
>>>    /* Can be null if path is not set.  */
>>>    if (!env)
>>>      env = "";
>>>    orig_path = xstrdup (env);
>>> -  free_environ (environment);
>>>  }
>>
>> This usage of gdb_environ looks like pointless
>> wrapping / dupping / freeing to me -- I don't see why we
>> need to dup the whole host environment just to get at some
>> env variable.  Using good old getenv(3) directly should do,
>> and end up trimming off a bit of work from gdb's startup.
> 
> Absolutely.  I didn't pay close attention to this bit; I was just
> mechanically converting the code.
> 
>> I could see perhaps wanting to avoid / optimize the linear
>> walks that getenv must do, if we have many getenv calls in
>> gdb, but then that suggests keeping a gdb_environ global that
>> is initialized early on and is reused.  But that would miss
>> any setenv call that changes the environment since that
>> gdb_environ is created, so I'd prefer not do that unless
>> we find a real need.
> 
> I'll make the code use getenv instead.

Thanks.  That bit could/should go in as its own
preparatory/obvious patch first, no need to carry it in
the same patch.

Thanks,
Pedro Alves