From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3E3BC25B1A for ; Wed, 13 Jun 2018 14:53:18 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xOFJdzaLgDXF for ; Wed, 13 Jun 2018 14:53:18 -0400 (EDT) Received: from smtp2.mail.ru (smtp2.mail.ru [94.100.179.91]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8ED92259A5 for ; Wed, 13 Jun 2018 14:53:17 -0400 (EDT) From: Vladislav Shpilevoy Subject: [tarantool-patches] Re: [PATCH v2 05/11] box: port schema_find_id to C References: Message-ID: Date: Wed, 13 Jun 2018 21:53:13 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Shcherbatov , tarantool-patches@freelists.org Thanks for the patch! See 3 comments below. On 09/06/2018 12:32, Kirill Shcherbatov wrote: > Part of #3273. > --- > src/box/schema.cc | 54 ++++++++++++++++++++++++++++++++++++++---------------- > src/box/schema.h | 23 ++++++++++++++++------- > src/box/user.cc | 4 +++- > 3 files changed, 57 insertions(+), 24 deletions(-) > > diff --git a/src/box/schema.cc b/src/box/schema.cc > index 2ddf920..5d32e61 100644 > --- a/src/box/schema.cc > +++ b/src/box/schema.cc > @@ -222,30 +222,52 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def, > trigger_run_xc(&on_alter_space, space); > } > > -uint32_t > +int > schema_find_id(uint32_t system_space_id, uint32_t index_id, > - const char *name, uint32_t len) > + const char *name, uint32_t len, uint32_t *object_id) > { > + if (rc == 0) { > /* id is always field #1 */ > - return tuple_field_u32_xc(tuple, 0); > + if (tuple == NULL) > + *object_id = BOX_ID_NIL; > + else if (tuple_field_u32(tuple, 0, object_id) != 0) > + return -1; 1. You should not apply my diff as is with no inspection. Usually I do draft fixes in review. Here is the bug - on return -1 the iterator and region leaks. This time apply this: else if (tuple_field_u32(tuple, 0, object_id) != 0) - return -1; + rc = -1; } > diff --git a/src/box/schema.h b/src/box/schema.h > index 1f7414f..cd9bb5b 100644 > --- a/src/box/schema.h > +++ b/src/box/schema.h > @@ -102,6 +102,22 @@ space_is_system(struct space *space); > struct sequence * > sequence_by_id(uint32_t id); > > +/** > + * Find space id by name in specified system space with index. 2. Same as on the previous review: this function searches not only space id, but a system object id. > + * > + * @param system_space_id identifier of the system space. > + * @param index_id identifier of the index to lookup. > + * @param name of object to lookup. > + * @param len length of a name. > + * @param object_id[out] object_id or BOX_ID_NIL - not found. > + * > + * @retval 0 on success. > + * @retval -1 on error. > + */ > +int > +schema_find_id(uint32_t system_space_id, uint32_t index_id, > + const char *name, uint32_t len, uint32_t *object_id); 3. Invalid alignment. > + > #if defined(__cplusplus) > } /* extern "C" */ >