Re: [Tarantool-patches] [PATCH 2/4] raft: initialize raft completely at runtime

[Cyrill Gorcunov <gorcunov@gmail.com>]

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.