From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 References: <20180828162226.53550-1-krishtal.olja@gmail.com> <20180829090325.6drp4hqojae67prf@esperanza> In-Reply-To: <20180829090325.6drp4hqojae67prf@esperanza> From: Olga Krishtal Date: Wed, 29 Aug 2018 12:36:33 +0300 Message-ID: Subject: Re: [tarantool-patches] [PATCH] box: fix assertion with duplication in repl. source Content-Type: multipart/alternative; boundary="00000000000044fbd305748fb37e" To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: --00000000000044fbd305748fb37e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks! =D1=81=D1=80, 29 =D0=B0=D0=B2=D0=B3. 2018 =D0=B3. =D0=B2 12:03, Vladimir Da= vydov : > On Tue, Aug 28, 2018 at 07:22:26PM +0300, Olga Arkhangelskaia wrote: > > When we try to connect one master more than once we used to have > > assertion instead of error. > > > > Closes #3610 > > --- > > https://github.com/tarantool/tarantool/issues/3610 > > > https://github.com/tarantool/tarantool/tree/OKriw/assert_fail_when_connec= t_master_twice > > Broken link. > > > > > src/box/box.cc | 14 ++++++++++++++ > > test/box-tap/cfg.test.lua | 8 +++++++- > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/src/box/box.cc b/src/box/box.cc > > index 8d7454d1f..3a571ae3c 100644 > > --- a/src/box/box.cc > > +++ b/src/box/box.cc > > @@ -369,9 +369,23 @@ static void > > box_check_replication(void) > > { > > int count =3D cfg_getarr_size("replication"); > > + char *repl[count-1]; > > for (int i =3D 0; i < count; i++) { > > const char *source =3D cfg_getarr_elem("replication", i); > > box_check_uri(source, "replication"); > > + repl[i] =3D strdup(source); > > + if (repl[i] =3D=3D NULL) { > > + tnt_raise(OutOfMemory, sizeof(*source), "source", > "malloc"); > > + } > > + for (int j =3D i; j >=3D 1; j--) { > > + if (strcmp(repl[i], repl[j-1]) =3D=3D 0) { > > + tnt_raise(ClientError, ER_CFG, > "replication", > > + "duplication of replication > source"); > > + } > > + } > > + } > > + for (int i =3D 0; i < count; i++) { > > + free(repl[i]); > > This is totally wrong, because different URLs can point to the same > instance, e.g. > > Instance 1: box.cfg{listen =3D 12345} > > Instance 2: box.cfg{replication =3D {12345, 'localhost:12345'}} > > Crash. > > All you're supposed to do is fix the checks in replication.cc > I am a bit lost. We have to raise an exception when have duplication uri, or or just skip duplication? > > > } > > } > > > > diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua > > index 5e72004ca..b227c5edb 100755 > > --- a/test/box-tap/cfg.test.lua > > +++ b/test/box-tap/cfg.test.lua > > @@ -6,7 +6,7 @@ local socket =3D require('socket') > > local fio =3D require('fio') > > local uuid =3D require('uuid') > > local msgpack =3D require('msgpack') > > -test:plan(90) > > +test:plan(91) > > > > > -------------------------------------------------------------------------= ------- > > -- Invalid values > > @@ -446,5 +446,11 @@ code =3D string.format(code_fmt, dir, instance_uui= d, > uuid.new()) > > test:is(run_script(code), PANIC, "replicaset_uuid mismatch") > > fio.rmdir(dir) > > > > +-- > > +--gh-3610: assertion failure when trying to connect to the same master > more than once > > +-- > > +status, reason =3D pcall(box.cfg, {listen =3D 3303, > replication=3D{3303,3303}}) > > +test:ok(not status and reason:match("Incorrect"), "Duplication of > replication source") > > + > > The test passes with and without your patch, i.e. useless. > > Besides, it doesn't test all cases described in the issue. > will fix --00000000000044fbd305748fb37e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks!

=D1=81=D1=80, 29 =D0=B0=D0=B2=D0=B3. 2018 =D0=B3. =D0=B2 12:03, Vladimir D= avydov <vdavydov.dev@gmail.com= >:
On Tue, Aug 28, 2018 at 0= 7:22:26PM +0300, Olga Arkhangelskaia wrote:
> When we try to connect one master more than once we used to have
> assertion instead of error.
>
> Closes #3610
> ---
> https://github.com/tarantool/tarantool/issues= /3610
> https://= github.com/tarantool/tarantool/tree/OKriw/assert_fail_when_connect_master_t= wice

Broken link.

>
>=C2=A0 src/box/box.cc=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 14 +++= +++++++++++
>=C2=A0 test/box-tap/cfg.test.lua |=C2=A0 8 +++++++-
>=C2=A0 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 8d7454d1f..3a571ae3c 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -369,9 +369,23 @@ static void
>=C2=A0 box_check_replication(void)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int count =3D cfg_getarr_size("replicat= ion");
> +=C2=A0 =C2=A0 =C2=A0char *repl[count-1];
>=C2=A0 =C2=A0 =C2=A0 =C2=A0for (int i =3D 0; i < count; i++) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const char *sour= ce =3D cfg_getarr_elem("replication", i);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0box_check_uri(so= urce, "replication");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0repl[i] =3D strdup(so= urce);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (repl[i] =3D=3D NU= LL) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0tnt_raise(OutOfMemory, sizeof(*source), "source", "ma= lloc");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (int j =3D i; j &= gt;=3D 1; j--) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (strcmp(repl[i], repl[j-1]) =3D=3D 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0tnt_raise(ClientError, ER_CFG, "rep= lication",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"= ;duplication of replication source");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0for (int i =3D 0; i < count; i++) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0free(repl[i]);

This is totally wrong, because different URLs can point to the same
instance, e.g.

Instance 1: box.cfg{listen =3D 12345}

Instance 2: box.cfg{replication =3D {12345, 'localhost:12345'}}

Crash.

All you're supposed to do is fix the checks in replication.cc


I am a bit lost. We have to raise= an exception when have duplication uri, or or just skip duplication?
=

=C2=A0

>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 }
>=C2=A0
> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> index 5e72004ca..b227c5edb 100755
> --- a/test/box-tap/cfg.test.lua
> +++ b/test/box-tap/cfg.test.lua
> @@ -6,7 +6,7 @@ local socket =3D require('socket')
>=C2=A0 local fio =3D require('fio')
>=C2=A0 local uuid =3D require('uuid')
>=C2=A0 local msgpack =3D require('msgpack')
> -test:plan(90)
> +test:plan(91)
>=C2=A0
>=C2=A0 ----------------------------------------------------------------= ----------------
>=C2=A0 -- Invalid values
> @@ -446,5 +446,11 @@ code =3D string.format(code_fmt, dir, instance_uu= id, uuid.new())
>=C2=A0 test:is(run_script(code), PANIC, "replicaset_uuid mismatch&= quot;)
>=C2=A0 fio.rmdir(dir)
>=C2=A0
> +--
> +--gh-3610: assertion failure when trying to connect to the same maste= r more than once
> +--
> +status, reason =3D pcall(box.cfg, {listen =3D 3303, replication=3D{33= 03,3303}})
> +test:ok(not status and reason:match("Incorrect"), "Dup= lication of replication source")
> +

The test passes with and without your patch, i.e. useless.

Besides, it doesn't test all cases described in the issue.

will fix=C2=A0
--00000000000044fbd305748fb37e--