From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109])
 (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 CEE1C4765E0
 for <tarantool-patches@dev.tarantool.org>;
 Thu, 24 Dec 2020 01:49:38 +0300 (MSK)
Date: Thu, 24 Dec 2020 01:49:49 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20201223224949.hpd5qm7b4fj3d4lh@tkn_work_nb>
References: <20201222082806.779193-1-void@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20201222082806.779193-1-void@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v4] lua/key_def: fix
	compare_with_key() part count check
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: "Alexander V. Tikhonov" <avtikhon@tarantool.org>, Kirill Yukhin <kyukhin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

> Issue: https://github.com/tarantool/tarantool/issues/5307
> Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5307-fix-key_def-part-count-check-v4

LGTM.

I left several style / taste comments regarding the test. It is just for
you. I think they don't worth one more review iteration.

Alexander Ti., please, approve the push from the QA team side.

Kirill, please, push to master and cherry-pick to 2.6 and 2.5.

Changelog entry ('Lua' section):

* Fixed a segfault in `key_def.compare_with_key()` on an invalid key (gh-5307).

WBR, Alexander Turenko.

----

> diff --git a/test/box-tap/gh-5307-key_def-part-count-check.test.lua b/test/box-tap/gh-5307-key_def-part-count-check.test.lua
> new file mode 100755
> index 000000000..d9458c95a
> --- /dev/null
> +++ b/test/box-tap/gh-5307-key_def-part-count-check.test.lua
> @@ -0,0 +1,26 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local mytest = tap.test('key_def part count tests')
> +
> +mytest:plan(3)

Usually 'test'.

> +
> +local key_def = require('key_def')

Usually all requires are go first, when possible.

> +local kd = key_def.new({{fieldno = 1, type = 'unsigned'}})
> +local ok, res
> +
> +-- Should succeed

We trying to write comments in the same style: start with a capital
letter, end with a period.

> +ok, res = pcall(kd.compare_with_key, kd, {1}, {1})
> +mytest:ok(ok and res == 0, "Simple equality")

In my personal taste, it is good to use the same quotes (single or
double) when possible within at least a file. (I prefer single ones, but
without any reason, just get into the habit.)

> +
> +-- Should succeed
> +ok, res = pcall(kd.compare_with_key, kd, {1}, {2})
> +mytest:ok(ok and res < 0, "Simple inequality")
> +
> +-- Should fail
> +local exp_err = "Invalid key part count (expected [0..1], got 9)"
> +ok, res = pcall(kd.compare_with_key, kd, {1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
> +mytest:is_deeply({ok, tostring(res)}, {false, exp_err},
> +    "Invalid key part count")
> +
> +os.exit(mytest:check() and 0 or 1)
> -- 
> 2.25.1
>