From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CE82F46970E for ; Mon, 10 Feb 2020 14:05:31 +0300 (MSK) Date: Mon, 10 Feb 2020 14:03:12 +0300 From: Igor Munkin Message-ID: <20200210110312.GN26983@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 1/1] app: os.setenv() affects os.environ() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Vlad, Thanks for the patch, LGTM. Minor: I see no particular reason to implement the test below in "diff" instead of TAP, but I don't insist since all other related tests are already implemented in "diff". Side note: OMG, I guess spoiling Lua standart namespaces is some kind of obsession. I can understand the reason of adding new methods in string table since it's a metatable for all string objects. However, I can't understand this activity regarding other standart namespaces starting from Mike with his table.new (but thanks for an additional 'require' required to enabled this feature) and finishing with #1718. I hope sometime we move everything to a separate namespace like the one uJIT has. The remark is not related to the patch, the changes are good, thanks again. On 06.02.20, Vladislav Shpilevoy wrote: > os.setenv() and os.environ() are Lua API for > > extern char **environ; > int setenv(); > > The Open Group standardized access points for environment > variables. But there is no a word about that environ never > changes. Programs can't relay on that. For example, addition of > a new variable may cause realloc of the whole environ array, and > therefore change of its pointer value. That was exactly the case > in os.environ() - it was using value of environ array remembered > when Tarantool started. > > And os.setenv() could realloc the array and turn the saved pointer > into garbage. > > Closes #4733 > --- > Issue: https://github.com/tarantool/tarantool/issues/4733 > Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4733-os.environ > > src/lua/env.lua | 3 +-- > test/app/env.result | 46 +++++++++++++++++++++++++++++++++++++++---- > test/app/env.test.lua | 27 +++++++++++++++++++++---- > 3 files changed, 66 insertions(+), 10 deletions(-) > > diff --git a/src/lua/env.lua b/src/lua/env.lua > index b150a48a9..dd1616a84 100644 > --- a/src/lua/env.lua > +++ b/src/lua/env.lua > @@ -9,9 +9,8 @@ ffi.cdef[[ > int unsetenv(const char *name); > ]] > > -local environ = ffi.C.environ > - > os.environ = function() > + local environ = ffi.C.environ > if not environ then > return nil > end > diff --git a/test/app/env.result b/test/app/env.result > index f725b01c0..c584947fb 100644 > --- a/test/app/env.result > +++ b/test/app/env.result > @@ -24,6 +24,9 @@ type(env_dict) > --- > - table > ... > +err = nil > +--- > +... > test_run:cmd("setopt delimiter ';'") > --- > - true > @@ -31,11 +34,46 @@ test_run:cmd("setopt delimiter ';'") > do > for k, v in pairs(env_dict) do > if type(k) ~= 'string' or type(v) ~= 'string' then > - return false > + err = {k, v} > + break > end > end > - return true > -end; > +end > +test_run:cmd("setopt delimiter ''"); > --- > -- true > +... > +err > +--- > +- null > +... > +-- > +-- gh-4733: os.setenv() should affect os.environ > +-- > +size = 0 > +--- > +... > +for _, __ in pairs(os.environ()) do size = size + 1 end > +--- > +... > +for i = 1, size do os.setenv('gh-4733-test-var'..i, tostring(i)) end > +--- > +... > +env = os.environ() > +--- > +... > +err = nil > +--- > +... > +for i = 1, size do \ > + local value = env['gh-4733-test-var'..i] \ > + if value ~= tostring(i) then \ > + err = {i, value} \ > + break \ > + end \ > +end > +--- > +... > +err > +--- > +- null > ... > diff --git a/test/app/env.test.lua b/test/app/env.test.lua > index fbc1ace86..b725f0124 100644 > --- a/test/app/env.test.lua > +++ b/test/app/env.test.lua > @@ -9,13 +9,32 @@ do os.getenv('location') end > > env_dict = os.environ() > type(env_dict) > +err = nil > test_run:cmd("setopt delimiter ';'") > do > for k, v in pairs(env_dict) do > if type(k) ~= 'string' or type(v) ~= 'string' then > - return false > + err = {k, v} > + break > end > end > - return true > -end; > -test_run:cmd("setopt delimiter ''") > +end > +test_run:cmd("setopt delimiter ''"); > +err > + > +-- > +-- gh-4733: os.setenv() should affect os.environ > +-- > +size = 0 > +for _, __ in pairs(os.environ()) do size = size + 1 end > +for i = 1, size do os.setenv('gh-4733-test-var'..i, tostring(i)) end > +env = os.environ() > +err = nil > +for i = 1, size do \ > + local value = env['gh-4733-test-var'..i] \ > + if value ~= tostring(i) then \ > + err = {i, value} \ > + break \ > + end \ > +end > +err > -- > 2.21.1 (Apple Git-122.3) > -- Best regards, IM