[tarantool-patches] Re: [PATCH v2 05/11] box: port schema_find_id to C

[Vladislav Shpilevoy]

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" */
>