[tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the patch! Please, put the whole new patch at the end
of a letter after multiple '======'. It helps in review, thanks.

See 6 comments below.

1. Lets change commit title: the patch does not introduce
box_space_id_by_name. It was before the patch.

On 01/06/2018 23:24, Kirill Shcherbatov wrote:
>> 1. The comment now is wrong - space_id_by_name will be used exactly for
>> non-public API.
>> 2. space_id_by_name must not take system_space_id as an argument. It must
>> always use BOX_SPACE_ID.
>>    * Return 0 on success, -1 on error.
>>    * @param[out] space_id BOX_ID_NIL - not found. Else space_id is
>>    *             saved.
>> 3. Not _vspace.
>> 4. space_id_by_name is not public function. You must not put it inside
>> /** \cond public */.
>> 5. Why can not you just port schema_find_id to C?
> I've ported schema_find_id to C. This required change it's signature.
> 
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 2ddf920..c5055ee 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -224,28 +224,53 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def,
>   
>   uint32_t
>   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 *space_id)

2. How can you return -1, when the return type is unsigned?

3. It is not 'space_id' speaking in general. This function is used in
user.cc also to search user id by the name. Please, find another name
for the parameter.

>   {
> -	if (len > BOX_NAME_MAX)
> -		return BOX_ID_NIL;
> -	struct space *space = space_cache_find_xc(system_space_id);
> -	struct index *index = index_find_system_xc(space, index_id);
> +	*space_id = BOX_ID_NIL;
> +	if (len > BOX_NAME_MAX) {
> +		diag_set(SystemError,
> +			 "name length %d is greater than BOX_NAME_MAX", len);
> +		return -1;
> +	}
> +	struct space *space = space_cache_find(system_space_id);
> +	if (space == NULL)
> +		return -1;
> +	if (!space_is_memtx(space)) {
> +		diag_set(ClientError, ER_UNSUPPORTED,
> +			 space->engine->name, "system data");
> +		return -1;
> +	}
> +	struct index *index = index_find(space, index_id);
> +	if (index == NULL)
> +		return -1;
>   	uint32_t size = mp_sizeof_str(len);
>   	struct region *region = &fiber()->gc;
>   	uint32_t used = region_used(region);
> -	char *key = (char *) region_alloc_xc(region, size);
> -	auto guard = make_scoped_guard([=] { region_truncate(region, used); });
> +	char *key = (char *)region_alloc(region, size);
> +	if (key == NULL) {
> +		diag_set(OutOfMemory, size, "region", "new slab");

4. "new slab" -> "key". OutOfMemory takes size, alloc function and
the result variable name.

> +		return -1;
> +	}
>   	mp_encode_str(key, name, len);
> -
> -	struct iterator *it = index_create_iterator_xc(index, ITER_EQ, key, 1);
> -	IteratorGuard iter_guard(it);
> -
> -	struct tuple *tuple = iterator_next_xc(it);
> +	struct iterator *it = index_create_iterator(index, ITER_EQ, key, 1);
> +	if (it == NULL) {
> +		region_truncate(region, used);
> +		return -1;
> +	}
> +	int rc = 0;
> +	struct tuple *tuple;
> +	if (iterator_next(it, &tuple) != 0) {
> +		rc = -1;
> +		goto cleanup;
> +	}
>   	if (tuple) {
>   		/* id is always field #1 */
> -		return tuple_field_u32_xc(tuple, 0);
> +		*space_id = tuple_field_u32_xc(tuple, 0);
>   	}
> -	return BOX_ID_NIL;
> +cleanup:
> +	iterator_delete(it);
> +	region_truncate(region, used);
> +	return rc;

5. You should avoid 'goto cleanup' here.

diff --git a/src/box/schema.cc b/src/box/schema.cc
index c5055eefe..a08588aef 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -257,17 +257,12 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id,
  		region_truncate(region, used);
  		return -1;
  	}
-	int rc = 0;
  	struct tuple *tuple;
-	if (iterator_next(it, &tuple) != 0) {
-		rc = -1;
-		goto cleanup;
-	}
-	if (tuple) {
-		/* id is always field #1 */
+	int rc = iterator_next(it, &tuple);
+	if (rc == 0 && tuple != NULL) {
+		/* ID is always field #1. */
  		*space_id = tuple_field_u32_xc(tuple, 0);
  	}
-cleanup:
  	iterator_delete(it);
  	region_truncate(region, used);
  	return rc;

>   }
> diff --git a/src/box/schema.h b/src/box/schema.h
> index 1f7414f..099f9b0 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.
> + *
> + * @param system_space_id identifier of the system space.
> + * @param index_id identifier of the index to lookup.
> + * @param name space to lookup name.
> + * @param len length of a name.
> + * @param space_id[out] space_id BOX_ID_NIL - not found.

6. Same. Do not mention space. It is a function to find ID of
any system object. Not only space. This function is applicable
for _index, _space, _user, _func.

> + *
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +uint32_t
> +schema_find_id(uint32_t system_space_id, uint32_t index_id,
> +	       const char *name, uint32_t len, uint32_t *space_id);
> +
>   #if defined(__cplusplus)
>   } /* extern "C" */