From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 84524446440 for ; Tue, 10 Nov 2020 03:00:00 +0300 (MSK) References: <5226a32c8783879dbf1b3714730b87147767466f.1604858551.git.v.shpilevoy@tarantool.org> <20201109145933.GK2339@grain> From: Vladislav Shpilevoy Message-ID: <1b0cd1da-1d54-4cd6-9888-3e34e0477bfc@tarantool.org> Date: Tue, 10 Nov 2020 00:59:58 +0100 MIME-Version: 1.0 In-Reply-To: <20201109145933.GK2339@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/4] raft: initialize raft completely at runtime List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org Thanks for the review! On 09.11.2020 15:59, Cyrill Gorcunov wrote: > On Sun, Nov 08, 2020 at 07:03:55PM +0100, Vladislav Shpilevoy wrote: >> Global raft object was created mostly at compile time with >> constant values assigned to all raft fields. But it won't work >> soon because raft will be moved to a separate library, and it >> will provide methods for raft creation and destruction. >> >> This patch makes raft not rely on compile time initialization, >> and moves it entirely to raft_init(). Also there are added some >> validations via new raft_validate() function, which should ensure >> raft is not used before creation. >> >> This may be hard to support, since raft is used by recovery and >> replication, even before instance has an ID. The validation should >> help to catch bugs related to the initialization order of these >> subsystems. >> >> Part of #5303 > ... >> @@ -1079,6 +1081,11 @@ raft_schedule_broadcast(void) >> void >> raft_init(void) >> { >> + memset(&raft, 0, sizeof(raft)); >> + raft.state = RAFT_STATE_FOLLOWER; >> + raft.volatile_term = 1; >> + raft.term = 1; >> + raft.election_timeout = 5; > > Please provide a separate helper instead, just like you did > for qsync (ie limbo), say I am not sure I understand what do you mean as 'helper'. If you mean raft_create() for a non-global raft object, it is done in the next patch. Because this one is not about not accessing the global variable. It is about its initialization at runtime. > void > raft_init(struct raft *raft) > { > *raft = (struct raft) { > .state = RAFT_STATE_FOLLOWER, > .volatile_term = 1, > .term = 1, > .election_timeout = 5, > }; > } > > this way you could provide some "raft_defaults" options in future > and simply pass it via arguments if needed. Same time this eliminate > needless memset call, let the compiler do its work. If you mean this = {}, then I don't mind. Except that I won't use this extra alignment. I find it hard to follow all these spaces, when I found a needed member name in the struct, and need to find its value now. ==================== @@ -1089,11 +1089,12 @@ raft_schedule_broadcast(void) void raft_init(void) { - memset(&raft, 0, sizeof(raft)); - raft.state = RAFT_STATE_FOLLOWER; - raft.volatile_term = 1; - raft.term = 1; - raft.election_timeout = 5; + raft = (struct raft) { + .state = RAFT_STATE_FOLLOWER, + .volatile_term = 1, + .term = 1, + .election_timeout = 5, + }; ev_timer_init(&raft.timer, raft_sm_schedule_new_election_cb, 0, 0); rlist_create(&raft.on_update); } ==================== >> +/** >> + * Ensure the raft node can be used. I.e. that it is properly initialized. >> + * Entirely for debug purposes. >> + */ >> +static inline void >> +raft_validate(void) >> +{ >> + assert(raft.state != 0); >> +} >> + > > It should be rather called raft_assert(&raft) pointing that this won't has > any effect on nondebug builds. On the other hands if you plan to add more tests > here then we need to deside what to do when raft is screwed? Should we panic? It is only a debug check. It validates the most trivial cases, which for sure will explode in any test in case they are broken. > Same time calling this helper on that many operations is ugly :( In next patches > you made operations to work with box_raft instance so what we could do is instead > of passing &box_raft we provide an inline helper which would fetch the pointer > and verify its state instead. Say > > static struct raft box_raft; > > static inline struct raft * > box_raft(void) > { > raft_assert(&box_raft); > return &box_raft; > } > > > box_init() > { > ... > raft_init(&box_raft); > ... > } > > Hm? I'm not insisting though, just to share ideas. Indeed. That would look better. Raft_validate() won't be useful for non-global raft objects, because we won't be able to initialize their state as 0 at compile time. In that way we can keep the "validation" out of src/lib/raft.