From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2D4DC469719 for ; Mon, 9 Nov 2020 17:59:37 +0300 (MSK) Received: by mail-lf1-f66.google.com with SMTP id j205so5463506lfj.6 for ; Mon, 09 Nov 2020 06:59:37 -0800 (PST) Date: Mon, 9 Nov 2020 17:59:33 +0300 From: Cyrill Gorcunov Message-ID: <20201109145933.GK2339@grain> References: <5226a32c8783879dbf1b3714730b87147767466f.1604858551.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5226a32c8783879dbf1b3714730b87147767466f.1604858551.git.v.shpilevoy@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 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 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. > > +/** > + * 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? 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.