From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <kostja.osipov@gmail.com>
Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com
 [209.85.167.65])
 (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 24027469719
 for <tarantool-patches@dev.tarantool.org>;
 Sat, 15 Feb 2020 20:37:43 +0300 (MSK)
Received: by mail-lf1-f65.google.com with SMTP id t23so8951465lfk.6
 for <tarantool-patches@dev.tarantool.org>;
 Sat, 15 Feb 2020 09:37:43 -0800 (PST)
Date: Sat, 15 Feb 2020 20:37:41 +0300
From: Konstantin Osipov <kostja.osipov@gmail.com>
Message-ID: <20200215173741.GC19902@atlas>
References: <20200214140339.4085-1-gorcunov@gmail.com>
 <20200214140339.4085-4-gorcunov@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200214140339.4085-4-gorcunov@gmail.com>
Subject: Re: [Tarantool-patches] [PATCH v10 3/4] box/applier: prevent nil
 dereference on applier rollback
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>
Cc: tml <tarantool-patches@dev.tarantool.org>

* Cyrill Gorcunov <gorcunov@gmail.com> [20/02/14 17:06]:
> +	/*
> +	 * FIXME: Do not clear fiber()->diag since it
> +	 * cause nil dereference
> +	 *
> +	 *   applier_subscribe
> +	 *     applier_apply_tx
> +	 *       diag_raise
> +	 *
> +	 * In turn we need to redesign this code:
> +	 *  - preserve original error or log it somewhere
> +	 *  - make the error path more clear
> +	 *
> +	 *  We must never reach this point with clean diag
> +	 *  area, if we do it means we're simply screwed
> +	 *  somewhere and there is a bug.

I think this comment is obsolete now with the fix below.

You no longer clear fiber->diag.

> +	 */
> +
> +	if (!diag_is_empty(diag_get()))
> +		diag_log();
> +	else
> +		say_warn_ratelimited("applier_txn_rollback_cb: empty diag");

You can also add assert here for debug mode. It should never
happen.

> +
>  	/* Setup shared applier diagnostic area. */
>  	diag_set(ClientError, ER_WAL_IO);
> -	diag_move(&fiber()->diag, &replicaset.applier.diag);
> +	diag_add_error(&replicaset.applier.diag,
> +		       diag_last_error(&fiber()->diag));
> +

It would be nice to explain in the comment why you want to
preserve the original error in the fiber here: because when later
this fiber is joined in (add call site here), we may want to check
its diagnostics area.

>  	/* Broadcast the rollback event across all appliers. */
>  	trigger_run(&replicaset.applier.on_rollback, event);
>  	/* Rollback applier vclock to the committed one. */

-- 
Konstantin Osipov, Moscow, Russia