From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
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 42F06469719
 for <tarantool-patches@dev.tarantool.org>;
 Tue,  8 Sep 2020 01:55:07 +0300 (MSK)
References: <cover.1599173312.git.v.shpilevoy@tarantool.org>
 <c644f23625085aa0fc82c79ba89b700bdd27cb0b.1599173312.git.v.shpilevoy@tarantool.org>
 <5811bf2d-a81e-721c-5da1-719a3dc7b5e9@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <a3219d80-e456-b950-8f11-940e357a9296@tarantool.org>
Date: Tue, 8 Sep 2020 00:55:05 +0200
MIME-Version: 1.0
In-Reply-To: <5811bf2d-a81e-721c-5da1-719a3dc7b5e9@tarantool.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Subject: Re: [Tarantool-patches] [PATCH v2 06/10] raft: introduce
	box.cfg.raft_* options
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Serge Petrenko <sergepetrenko@tarantool.org>, tarantool-patches@dev.tarantool.org, gorcunov@gmail.com

Thanks for the review!

>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 281917af2..5f04a1a78 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -472,6 +472,40 @@ box_check_uri(const char *source, const char *option_name)
>>       }
>>   }
>>   +static int
>> +box_check_raft_is_enabled(void)
>> +{
>> +    int b = cfg_getb("raft_is_enabled");
>> +    if (b < 0) {
>> +        diag_set(ClientError, ER_CFG, "raft_is_enabled",
>> +             "the value must be a boolean");
>> +    }
>> +    return b;
>> +}
>> +
>> +static int
>> +box_check_raft_is_candidate(void)
>> +{
>> +    int b = cfg_getb("raft_is_candidate");
>> +    if (b < 0) {
>> +        diag_set(ClientError, ER_CFG, "raft_is_candidate",
>> +             "the value must be a boolean");
>> +    }
>> +    return b;
>> +}
>> +
>> +static double
>> +box_check_raft_election_timeout(void)
>> +{
>> +    double d = cfg_getd("raft_election_timeout");
>> +    if (d == 0) {
> 
> Should be "d <= 0" here?
> 
> Otherwise you end up with a diag_raise without appropriate diag_set
> when raft_election_timeout is negative.

Yes, this is a typo:

====================
diff --git a/src/box/box.cc b/src/box/box.cc
index 5f04a1a78..5c87d86d7 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -498,7 +498,7 @@ static double
 box_check_raft_election_timeout(void)
 {
 	double d = cfg_getd("raft_election_timeout");
-	if (d == 0) {
+	if (d <= 0) {
 		diag_set(ClientError, ER_CFG, "raft_election_timeout",
 			 "the value must be a positive number");
 		return -1;