[tarantool-patches] Re: [tarantool-patches] Re: [PATCH v4 06/20] refactoring: remove exceptions from schema_find_grants

[Ilya Kosarev]

Hi!

Thanks for your review.
iterator_next is actually fine. It handles problems in a common way, as it is said in the comment in the header file:
/**
 * Iterate to the next tuple.
 *
 * The tuple is returned in @ret (NULL if EOF).
 * Returns 0 on success, -1 on error.
 */

It returns either what it->next(...) returns or 0 if we don't need to call it. This 0 return is not an error, although it is done under "invalidate" label.
In case it->next(...) is called, the underlying function will be working as expected. diag_set & return -1 in case of error or return 0 if everything is fine.

The same error handling is implemented in  iterator_next_xc.
Sincerely,
Ilya Kosarev


>Пятница, 18 октября 2019, 18:17 +03:00 от Sergey Ostanevich <sergos at tarantool.org>:
>
>Hi!
>
>LGTM with one nit - see below, follow up as a separate commit if needed.
>
>Sergos
>
>
>On 23 Sep 18:56, Ilya Kosarev wrote:
>> schema_find_grants is used in some triggers therefore it has to be
>> cleared from exceptions. Now it doesn't throw any more.
>> It's usages are updated.
>> 
>> Part of #4247
>> ---
>> diff --git a/src/box/schema.cc b/src/box/schema.cc
>> index 8d8aae448..9767207e0 100644
>> --- a/src/box/schema.cc
>> +++ b/src/box/schema.cc
>> @@ -599,12 +599,22 @@ func_by_name(const char *name, uint32_t name_len)
>>  	return (struct func *) mh_strnptr_node(funcs_by_name, func)->val;
>>  }
>> 
>> -bool
>> -schema_find_grants(const char *type, uint32_t id)
>> +int
>> +schema_find_grants(const char *type, uint32_t id, bool *out)
>>  {
>> -	struct space *priv = space_cache_find_xc(BOX_PRIV_ID);
>> +	struct space *priv = space_cache_find(BOX_PRIV_ID);
>> +	if (priv == NULL)
>> +		return -1;
>> +
>>  	/** "object" index */
>> -	struct index *index = index_find_system_xc(priv, 2);
>> +	if (!space_is_memtx(priv)) {
>> +		diag_set(ClientError, ER_UNSUPPORTED,
>> +			 priv->engine->name, "system data");
>> +		return -1;
>> +	}
>> +	struct index *index = index_find(priv, 2);
>> +	if (index == NULL)
>> +		return -1;
>>  	/*
>>  	 * +10 = max(mp_sizeof_uint32) +
>>  	 *       max(mp_sizeof_strl(uint32)).
>> @@ -612,9 +622,15 @@ schema_find_grants(const char *type, uint32_t id)
>>  	char key[GRANT_NAME_MAX + 10];
>>  	assert(strlen(type) <= GRANT_NAME_MAX);
>>  	mp_encode_uint(mp_encode_str(key, type, strlen(type)), id);
>> -	struct iterator *it = index_create_iterator_xc(index, ITER_EQ, key, 2);
>> +	struct iterator *it = index_create_iterator(index, ITER_EQ, key, 2);
>> +	if (it == NULL)
>> +		return -1;
>>  	IteratorGuard iter_guard(it);
>> -	return iterator_next_xc(it);
>> +	struct tuple *tuple;
>> +	if (iterator_next(it, &tuple) != 0)
>> +		return -1;
>The iterator_next() has some unclear semantics with no diag set.
>Please, reconsider the returned value and the diagnosis.
>
>> +	*out = (tuple != NULL);
>> +	return 0;
>>  }
>> 
>>  struct sequence *
>> diff --git a/src/box/schema.h b/src/box/schema.h
>> index f9d15b38d..66555ab14 100644
>> --- a/src/box/schema.h
>> +++ b/src/box/schema.h
>> @@ -185,11 +185,11 @@ func_cache_find(uint32_t fid)
>>   * Check whether or not an object has grants on it (restrict
>>   * constraint in drop object).
>>   * _priv space to look up by space id
>> - * @retval true object has grants
>> - * @retval false object has no grants
>> + * @retval (bool *out) true object has grants
>> + * @retval (bool *out) false object has no grants
>>   */
>> -bool
>> -schema_find_grants(const char *type, uint32_t id);
>> +int
>> +schema_find_grants(const char *type, uint32_t id, bool *out);
>> 
>>  /**
>>   * A wrapper around sequence_by_id() that raises an exception
>> -- 
>> 2.17.1
>> 
>> 
>


-- 
Ilya Kosarev
it->next
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20191021/2e1c94c7/attachment.html>