From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 5C98446970F for ; Wed, 27 Nov 2019 00:01:26 +0300 (MSK) References: <76faabe3149b2671950cea5b5c9bb18df15472f5.1574630240.git.k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 26 Nov 2019 22:01:24 +0100 MIME-Version: 1.0 In-Reply-To: <76faabe3149b2671950cea5b5c9bb18df15472f5.1574630240.git.k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] box: frommap() bug fix List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.tarantool.org Thanks for the patch! See 4 comments below. On 24/11/2019 22:18, Chris Sosnin wrote: > - If an optional argument is provided for > space_object:frommap() (which is {table = true|false}), > type match for first arguments is omitted, which is > incorrect. We should return the result only after making > sure it is possible to build a tuple. 1. This is a good catch. Please, provide a test. To ensure, that it won't break again in the future. I think you fit it in an existing test file, where other frommap() cases are checked. It is not worth creating a new test file just for this tiny ticket. > > - If there is a type mismatch, however, frommap() does not > return nil, err as it is mentioned in the description, so we > change it to be this way. > > Closes: #4262 > --- 2. All the same as for #4515 email. > src/box/lua/space.cc | 11 +++++++---- > test/box/tuple.result | 3 ++- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc > index f6e96f0c0..a4ffa8240 100644 > --- a/src/box/lua/space.cc > +++ b/src/box/lua/space.cc > @@ -580,14 +580,17 @@ lbox_space_frommap(struct lua_State *L) > } > lua_rawseti(L, -3, fieldno+1); > } > - if (table) > - return 1; > > lua_replace(L, 1); > lua_settop(L, 1); > tuple = luaT_tuple_new(L, -1, space->format); > - if (tuple == NULL) > - return luaT_error(L); > + if (tuple == NULL) { > + struct error *e = diag_last_error(&fiber()->diag); > + lua_pushnil(L); > + lua_pushstring(L, e->errmsg); > + return 2; > + } else if (table) > + return 1; 3. Nit: I think you may omit 'else' keyword here. Anyway the previous condition makes a return. 4. When any if's or else's body has {}, all the others in the same chain should have it as well. So 'return 1;' should also be in {}. > luaT_pushtuple(L, tuple); > return 1; > usage_error: