From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87907 invoked by alias); 19 Jun 2017 04:19:20 -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 87660 invoked by uid 89); 19 Jun 2017 04:19:14 -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,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=Hx-languages-length:3303 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; Mon, 19 Jun 2017 04:19:13 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 36287C0587D6; Mon, 19 Jun 2017 04:19:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 36287C0587D6 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 36287C0587D6 Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0B7F880713; Mon, 19 Jun 2017 04:19:16 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Pedro Alves Subject: Re: [PATCH v5] C++ify gdb/common/environ.c References: <20170413040455.23996-1-sergiodj@redhat.com> <20170616222315.12779-1-sergiodj@redhat.com> <4e43c71a2ac4aa229bb262e18dec668c@polymtl.ca> Date: Mon, 19 Jun 2017 04:19:00 -0000 In-Reply-To: <4e43c71a2ac4aa229bb262e18dec668c@polymtl.ca> (Simon Marchi's message of "Sat, 17 Jun 2017 10:54:15 +0200") Message-ID: <87tw3cy5h7.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00491.txt.bz2 On Saturday, June 17 2017, Simon Marchi wrote: > On 2017-06-17 00:23, Sergio Durigan Junior wrote: >> void >> -set_in_environ (struct gdb_environ *e, const char *var, const char >> *value) >> +gdb_environ::set (const char *var, const char *value) >> { >> - int i; >> - int len = strlen (var); >> - char **vector = e->vector; >> - char *s; >> - >> - for (i = 0; (s = vector[i]) != NULL; i++) >> - if (strncmp (s, var, len) == 0 && s[len] == '=') >> - break; >> + /* We have to unset the variable in the vector if it exists. */ >> + unset (var); >> >> - if (s == 0) >> - { >> - if (i == e->allocated) >> - { >> - e->allocated += 10; >> - vector = (char **) xrealloc ((char *) vector, >> - (e->allocated + 1) * sizeof (char *)); >> - e->vector = vector; >> - } >> - vector[i + 1] = 0; >> - } >> - else >> - xfree (s); >> - >> - s = (char *) xmalloc (len + strlen (value) + 2); >> - strcpy (s, var); >> - strcat (s, "="); >> - strcat (s, value); >> - vector[i] = s; >> - >> - /* This used to handle setting the PATH and GNUTARGET variables >> - specially. The latter has been replaced by "set gnutarget" >> - (which has worked since GDB 4.11). The former affects searching >> - the PATH to find SHELL, and searching the PATH to find the >> - argument of "symbol-file" or "exec-file". Maybe we should have >> - some kind of "set exec-path" for that. But in any event, having >> - "set env" affect anything besides the inferior is a bad idea. >> - What if we want to change the environment we pass to the program >> - without afecting GDB's behavior? */ >> - >> - return; >> + /* Insert the element before the last one, which is always NULL. */ >> + m_environ_vector.insert (m_environ_vector.end () - 1, >> + concat (var, "=", value, NULL)); > > The breaks if we have just constructed an empty gdb_environ object, as > the vector is completely empty (no terminating NULL). So we'd need > some kind of check before that, if the vector is empty, add a NULL > element... > > I actually preferred the option of adding the NULL element to the > vector in the gdb_environ constructor, since it allows always having > the vector in a consistent state. I don't think that avoiding that > heap allocation is worth the complexity it adds to the code (unless we > can prove otherwise by memory usage profiling). I've had some time to think about this, and I agree. I liked the code better when it had the ctor doing the initialization of the vector. I think it also makes a lot more sense to always initialize the vector with a NULL element, because this means we're correctly dealing with the case where there's no environment variable to be passed to the inferior. I'll update the code and re-submit the patch this way. > >> +static void >> +run_tests () >> +{ >> + if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0) >> + error ("Could not set environment variable for testing."); >> + >> + gdb_environ env; >> + >> + SELF_CHECK (env.envp ()[0] == NULL); >> + >> + SELF_CHECK (env.get ("PWD") == NULL); > > If you add > > env.set ("PWD", "/home"); > > you should see a crash. Right. I'll make sure to add this check as well. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/