Re: [Tarantool-patches] [PATCH] lua/key_def: fix compare_with_key() part count check

[Sergey Nikiforov <void@tarantool.org>]

Hi!

On 17.12.2020 1:34, Vladislav Shpilevoy wrote:
> Hi!
> 
> On 14.12.2020 16:35, Sergey Nikiforov wrote:
>> Added corresponding test
>>
>> Fixes: #5307
>> ---
> 
> Please, try to follow the guidelines.
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure
> 
> I don't see the issue and branch links. Also I can't find the
> branch in `git branch -a | grep 5307`. Also in the ticket's
> webpage I don't see any links to the commit, which github usually
> adds automatically. So I suspect you simply didn't push it anywhere.

Of course I did not push my branches for the first batch of patches - 
because no one said I had to. I am new to github and Tarantool workflow, 
sorry.

>> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
>> index a781aeff9..674891a85 100644
>> --- a/src/box/lua/key_def.c
>> +++ b/src/box/lua/key_def.c
>> @@ -362,6 +362,15 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>>   	size_t key_len;
>>   	const char *key_end, *key = lbox_encode_tuple_on_gc(L, 3, &key_len);
>>   	uint32_t part_count = mp_decode_array(&key);
>> +
>> +	if (part_count > key_def->part_count) {
>> +		region_truncate(region, region_svp);
>> +		tuple_unref(tuple);
>> +		diag_set(ClientError, ER_KEY_PART_COUNT,
>> +			 key_def->part_count, part_count);
>> +		return luaT_error(L);
>> +	}
> 
> Why this check and the call below can't be all simply
> replaces with box_key_def_validate_key() call?

Because we need part_count later. With box_key_def_validate_key() we 
would have to call mp_decode_array() twice or add yet another parameter 
to box_key_def_validate_key(). Is that good idea?

>> +
>>   	if (key_validate_parts(key_def, key, part_count, true,
>>   			       &key_end) != 0) {
>>   		region_truncate(region, region_svp);