From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id C8A942408E for ; Thu, 7 Feb 2019 06:31:17 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ldKAo2IIoclF for ; Thu, 7 Feb 2019 06:31:17 -0500 (EST) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1531126C61 for ; Thu, 7 Feb 2019 06:31:16 -0500 (EST) Date: Thu, 07 Feb 2019 14:31:13 +0300 From: Roman Subject: [tarantool-patches] Re: [PATCH] box: fix bug with module_reload() without box.cfg{} Message-Id: <1549539073.27209.0@smtp.mail.ru> In-Reply-To: References: <20190205001737.8475-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=-GaUoTQ3YdBsaHZOH19I0" Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy --=-GaUoTQ3YdBsaHZOH19I0 Content-Type: text/plain; charset=iso-8859-5; format=flowed Content-Transfer-Encoding: quoted-printable Hi! Thanks for review. On =B2=E2, =E4=D5=D2 5, 2019 at 6:42 PM, Vladislav Shpilevoy=20 wrote: > Hi! Thanks for the patch! See 5 comments below. > 1. Instead of duplicating commit title, it is usually > better to describe here a reason why the bug existed. Done. >=20 > 2. Please, put branch and issue links after '---'. >=20 >> --- Sorry. > 4. Instead of checking for modules !=3D NULL in each public func.h=20 > function, > it is better to move module_init() call from box_cfg_xc() to=20 > box_init(). Then > main() will create modules hash. diff --git a/src/box/box.cc b/src/box/box.cc index 8892d0f0e..6494a0f44 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -2053,6 +2053,9 @@ box_init(void) */ session_init(); + if (module_init() !=3D 0) + diag_raise(); + if (tuple_init(lua_hash) !=3D 0) diag_raise(); @@ -2080,8 +2083,6 @@ box_cfg_xc(void) gc_init(); engine_init(); - if (module_init() !=3D 0) - diag_raise(); schema_init(); replication_init(); port_init(); > 5. Not-tap tests are always run with already called box.cfg(), > so these lines here will work regardless of your patch. Please, > move it to a tap test, in app-tap directory. Now I do not see > there any existing and suitable file for this test, so you > should create a new one. For example, app-tap/func.test.lua. diff --git a/test/app-tap/func.test.lua b/test/app-tap/func.test.lua new file mode 100755 index 000000000..11bc01f1d --- /dev/null +++ b/test/app-tap/func.test.lua @@ -0,0 +1,14 @@ +#!/usr/bin/env tarantool + +local tap =3D require('tap') + +local test =3D tap.test("func") + +test:plan(2) + +-- gh-3770 Check no segfault with module_reload() without box.cfg{}. + +test:ok(not pcall(box.internal.module_reload, ''), + 'expected error: no module') +test:ok(not pcall(box.internal.module_reload, 'xxx'), + 'expected error: no module') commit afd765d8bc86188c33cba3f06478853ec76bcaea Author: Roman Khabibov Date: Tue Feb 5 03:04:33 2019 +0300 box: fix bug with module_reload() without box.cfg{} A bug existed because module_init was called during a call to=20 box_cfg{}. Modules were not initialized before calling box.cfg{}. Closes #3770 diff --git a/src/box/box.cc b/src/box/box.cc index 8892d0f0e..6494a0f44 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -2053,6 +2053,9 @@ box_init(void) */ session_init(); + if (module_init() !=3D 0) + diag_raise(); + if (tuple_init(lua_hash) !=3D 0) diag_raise(); @@ -2080,8 +2083,6 @@ box_cfg_xc(void) gc_init(); engine_init(); - if (module_init() !=3D 0) - diag_raise(); schema_init(); replication_init(); port_init(); diff --git a/test/app-tap/func.test.lua b/test/app-tap/func.test.lua new file mode 100755 index 000000000..11bc01f1d --- /dev/null +++ b/test/app-tap/func.test.lua @@ -0,0 +1,14 @@ +#!/usr/bin/env tarantool + +local tap =3D require('tap') + +local test =3D tap.test("func") + +test:plan(2) + +-- gh-3770 Check no segfault with module_reload() without box.cfg{}. + +test:ok(not pcall(box.internal.module_reload, ''), + 'expected error: no module') +test:ok(not pcall(box.internal.module_reload, 'xxx'), + 'expected error: no module') = --=-GaUoTQ3YdBsaHZOH19I0 Content-Type: text/html; charset=iso-8859-5 Content-Transfer-Encoding: quoted-printable
Hi! Thanks for review.

On =B2=E2, =E4=D5=D2 5, 2019 at 6:42 PM, Vladislav Shpilevoy= <v.shpilevoy@tarantool.org> wrote:
Hi! Thanks for the pa= tch! See 5 comments below.

1. Instead of duplicating commit title, it is usually better to describe here a reason why the bug existed.
Done.

2. Please, put branch and issue links after '---'.
---
Sorry.

4. Instead of checking for modules !=3D NULL in each public fu= nc.h function, it is better to move module_init() call from box_cfg_xc() to box_init(). Th= en main() will create modules hash.
diff --git a/src/box/box.cc b/src/box/box.cc index 8892d0f0e..6494a0f44 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -2053,6 +2053,9 @@ box_init(void) */ session_init(); =20 + if (module_init() !=3D 0) + diag_raise(); + if (tuple_init(lua_hash) !=3D 0) diag_raise(); =20 @@ -2080,8 +2083,6 @@ box_cfg_xc(void) =20 gc_init(); engine_init(); - if (module_init() !=3D 0) - diag_raise(); schema_init(); replication_init(); port_init();

5. Not-tap tests are always run with already called box.cfg()= , so these lines here will work regardless of your patch. Please, move it to a tap test, in app-tap directory. Now I do not see there any existing and suitable file for this test, so you should create a new one. For example, app-tap/func.test.lua.
diff --git a= /test/app-tap/func.test.lua b/test/app-tap/func.test.lua new file mode 100755 index 000000000..11bc01f1d --- /dev/null +++ b/test/app-tap/func.test.lua @@ -0,0 +1,14 @@ +#!/usr/bin/env tarantool + +local tap =3D require('tap') + +local test =3D tap.test("func") + +test:plan(2) + +-- gh-3770 Check no segfault with module_reload() without box.cfg{}. + +test:ok(not pcall(box.internal.module_reload, ''), + 'expected error: no module') +test:ok(not pcall(box.internal.module_reload, 'xxx'), + 'expected error: no module') 


= commit afd765d8bc86188c33cba3f064788= 53ec76bcaea Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Tue Feb 5 03:04:33 2019 +0300 box: fix bug with module_reload() without box.cfg{} =20 A bug existed because module_init was called during a call to box_cfg{}= . Modules were not initialized before calling box.cfg{}. =20 Closes #3770 diff --git a/src/box/box.cc b/src/box/box.cc index 8892d0f0e..6494a0f44 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -2053,6 +2053,9 @@ box_init(void) */ session_init(); =20 + if (module_init() !=3D 0) + diag_raise(); + if (tuple_init(lua_hash) !=3D 0) diag_raise(); =20 @@ -2080,8 +2083,6 @@ box_cfg_xc(void) =20 gc_init(); engine_init(); - if (module_init() !=3D 0) - diag_raise(); schema_init(); replication_init(); port_init(); diff --git a/test/app-tap/func.test.lua b/test/app-tap/func.test.lua new file mode 100755 index 000000000..11bc01f1d --- /dev/null +++ b/test/app-tap/func.test.lua @@ -0,0 +1,14 @@ +#!/usr/bin/env tarantool + +local tap =3D require('tap') + +local test =3D tap.test("func") + +test:plan(2) + +-- gh-3770 Check no segfault with module_reload() without box.cfg{}. + +test:ok(not pcall(box.internal.module_reload, ''), + 'expected error: no module') +test:ok(not pcall(box.internal.module_reload, 'xxx'), + 'expected error: no module') 



= --=-GaUoTQ3YdBsaHZOH19I0--