From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22120 invoked by alias); 4 May 2017 15:30:51 -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 22020 invoked by uid 89); 4 May 2017 15:30:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=realized, Probably, wanting, importance X-HELO: mail-wr0-f170.google.com Received: from mail-wr0-f170.google.com (HELO mail-wr0-f170.google.com) (209.85.128.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 04 May 2017 15:30:39 +0000 Received: by mail-wr0-f170.google.com with SMTP id l50so9936046wrc.3 for ; Thu, 04 May 2017 08:30:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=Ld0yFl1DZtNxNr7engqvBp8BbYOZ8eAYDzQJwPgIUIc=; b=WUvUz86/gqPuQkZgRLdLv6MfHE6+viMPQJ72+vdFJ52ZH/1rdTCLnNIFXEc3+KwobZ TeQ2nxV/JKgwZuZZlKg3avmVvu6w7WmsVgF1DW2ne/tH1Z5KuXz8AKjyLOLeaVsHKTes Y4GOlEZL9IoiiaowUaX5n1MrYvEkdFQ/dkpCqMNDYYj3SpjHaVX/lxFw07nReSGxQQZI 4gUnZf14zvc5W9VAkwBwoP3Q+B6PZMwl+6XcKzFPESaouD6TXzMtKLdNa7s8yO+kbEwL 34uuq90tySLrMW+xBj6sPAW5EpgsZ5qFQU1zcFe40lmHTAve6bQHzYdG9nE5B7Ge5JeU Xdiw== X-Gm-Message-State: AN3rC/4kE91NAFTkSeVhxyfD9wlhvb8Vg5UPvTg9H0HI5BcTysXHb103 6pQ74gRnZ2ITP2oRU5Wsvg== X-Received: by 10.223.133.216 with SMTP id 24mr33127676wru.194.1493911839470; Thu, 04 May 2017 08:30:39 -0700 (PDT) Received: from [192.168.0.102] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id t30sm1929346wrc.24.2017.05.04.08.30.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 04 May 2017 08:30:38 -0700 (PDT) Subject: Re: [PATCH v3] C++ify gdb/common/environ.c To: Sergio Durigan Junior References: <20170413040455.23996-1-sergiodj@redhat.com> <20170418030319.12637-1-sergiodj@redhat.com> <87o9vdjoz4.fsf@redhat.com> Cc: GDB Patches , Simon Marchi From: Pedro Alves Message-ID: <744e83e1-e099-3c75-38f7-53db41d578b0@redhat.com> Date: Thu, 04 May 2017 15:30:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <87o9vdjoz4.fsf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00113.txt.bz2 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