From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43])
 (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 9457B469710
 for <tarantool-patches@dev.tarantool.org>;
 Sun,  3 May 2020 21:47:02 +0300 (MSK)
References: <20200428161137.20536-1-gorcunov@gmail.com>
 <20200428161137.20536-10-gorcunov@gmail.com>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <36cb2221-6640-1974-b2ee-bc251c345aaa@tarantool.org>
Date: Sun, 3 May 2020 20:47:00 +0200
MIME-Version: 1.0
In-Reply-To: <20200428161137.20536-10-gorcunov@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH 09/17] recovery: recovery_new --
 don't throw exception
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: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org>

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/recovery.cc b/src/box/recovery.cc
> index 24036b7c1..1c7665f87 100644
> --- a/src/box/recovery.cc
> +++ b/src/box/recovery.cc
> @@ -77,24 +77,21 @@
>  /* {{{ Initial recovery */
>  
>  /**
> - * Throws an exception in  case of error.
> + * Returns NULL in case of error.
>   */
>  struct recovery *
>  recovery_new(const char *wal_dirname, bool force_recovery,
>  	     const struct vclock *vclock)
>  {
> -	struct recovery *r = (struct recovery *)
> -			calloc(1, sizeof(*r));
> +	struct recovery *r;
>  
> +	r = (struct recovery *)calloc(1, sizeof(*r));

1. Would look better, if you assign 'r' on the same line
where you declare it. It is not really necessary to declare
all variables in the beginning and assign them separately.
Looks ugly, IMO. At least this particular place.

> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index 2e17d0476..a5bcd9580 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -363,8 +363,10 @@ relay_final_join(int fd, uint64_t sync, struct vclock *start_vclock,
>  		relay_delete(relay);
>  	});
>  
> -	relay->r = recovery_new(cfg_gets("wal_dir"), false,
> -			       start_vclock);
> +	relay->r = recovery_new(cfg_gets("wal_dir"), false, start_vclock);
> +	if (relay->r == NULL)
> +		diag_raise();
> +
>  	vclock_copy(&relay->stop_vclock, stop_vclock);
>  
>  	int rc = cord_costart(&relay->cord, "final_join",
> @@ -717,8 +719,10 @@ relay_subscribe(struct replica *replica, int fd, uint64_t sync,
>  	});
>  
>  	vclock_copy(&relay->local_vclock_at_subscribe, &replicaset.vclock);
> -	relay->r = recovery_new(cfg_gets("wal_dir"), false,
> -			        replica_clock);

2. This may look like a nit, but you could keep these 2 lines unchanged.
The same about the hunk above. My arguments are the same - patch size and
git blame purity.

> +	relay->r = recovery_new(cfg_gets("wal_dir"), false, replica_clock);
> +	if (relay->r == NULL)
> +		diag_raise();
> +
>  	vclock_copy(&relay->tx.vclock, replica_clock);
>  	relay->version_id = replica_version_id;
>  
>