Re: [Tarantool-patches] [PATCH v2 1/2] box: refactor in_promote using a guard

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Tue, May 25, 2021 at 01:39:28PM +0300, Serge Petrenko wrote:
> ---
>  src/box/box.cc | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index c10e0d8bf..894e3d0f4 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1562,6 +1562,9 @@ box_promote(void)
>  	int rc = 0;
>  	int quorum = replication_synchro_quorum;
>  	in_promote = true;
> +	auto promote_guard = make_scoped_guard([&] {
> +		in_promote = false;
> +	});

Looks ok to me, though I must confess I always consider such
flags spread all over the code is somehow clumsy. Since this
is a common pattern in our cpp code lets keep it but still in
my humble opinion we could rather move all box_promote code
into some box_do_promote helper and we would have

int
box_promote(void)
{
	static bool in_promote = false;
	if (in_promote) {
		diag_set(ClientError, ER_UNSUPPORTED, "box.ctl.promote",
			 "simultaneous invocations");
		return -1;
	}

	in_promote = true;
	int rc = box_do_promote();
	in_promote = false;

	return rc;
}

but surely this is not a request for code refactoring, current form
is ok as well ;)

Ack.

Serge, while you're at this code anyway, could you please change

	switch (box_election_mode) {
	case ELECTION_MODE_OFF:
		try_wait = true;
		break;
	...
	default:
		panic("enexpected box_election_mode mode");
		break;
	}

instead of unreacheable() call. We should stop using unreacheable()
as much as we could.