From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 29 Aug 2018 12:03:25 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH] box: fix assertion with duplication in repl. source Message-ID: <20180829090325.6drp4hqojae67prf@esperanza> References: <20180828162226.53550-1-krishtal.olja@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180828162226.53550-1-krishtal.olja@gmail.com> To: Olga Arkhangelskaia Cc: tarantool-patches@freelists.org List-ID: 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_connect_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 = cfg_getarr_size("replication"); > + char *repl[count-1]; > for (int i = 0; i < count; i++) { > const char *source = cfg_getarr_elem("replication", i); > box_check_uri(source, "replication"); > + repl[i] = strdup(source); > + if (repl[i] == NULL) { > + tnt_raise(OutOfMemory, sizeof(*source), "source", "malloc"); > + } > + for (int j = i; j >= 1; j--) { > + if (strcmp(repl[i], repl[j-1]) == 0) { > + tnt_raise(ClientError, ER_CFG, "replication", > + "duplication of replication source"); > + } > + } > + } > + for (int i = 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 = 12345} Instance 2: box.cfg{replication = {12345, 'localhost:12345'}} Crash. All you're supposed to do is fix the checks in replication.cc > } > } > > 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 = require('socket') > local fio = require('fio') > local uuid = require('uuid') > local msgpack = require('msgpack') > -test:plan(90) > +test:plan(91) > > -------------------------------------------------------------------------------- > -- Invalid values > @@ -446,5 +446,11 @@ code = string.format(code_fmt, dir, instance_uuid, 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 = pcall(box.cfg, {listen = 3303, replication={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.