[tarantool-patches] Re: [PATCH 2/5] Move some decode functions from alter.cc

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

Thanks for the patch! See 7 comments below.

On 12/07/2018 14:16, imeevma@tarantool.org wrote:
> Allow to use some format and options decode functions
> outside of alter.cc.
> 
> Part of #3375.
> ---
>   src/box/alter.cc    | 203 ++--------------------------------------------------
>   src/box/index_def.c |  60 ++++++++++++++++
>   src/box/index_def.h |   8 +++
>   src/box/space_def.c | 150 ++++++++++++++++++++++++++++++++++++++
>   src/box/space_def.h |  15 ++++
>   5 files changed, 237 insertions(+), 199 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 7b6bd1a..15240c0 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -250,7 +195,8 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
>   	const char *opts_field =
>   		tuple_field_with_type_xc(tuple, BOX_INDEX_FIELD_OPTS,
>   					 MP_MAP);
> -	index_opts_decode(&opts, opts_field, &fiber()->gc);
> +	if(index_opts_decode(&opts, opts_field, &fiber()->gc) != 0)

1. Please, put white space after 'if'.

> +		diag_raise();
>   	const char *parts = tuple_field(tuple, BOX_INDEX_FIELD_PARTS);
>   	uint32_t part_count = mp_decode_array(&parts);
>   	if (name_len > BOX_NAME_MAX) {
> @@ -510,6 +313,8 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode,
>   					 MP_ARRAY);
>   	fields = space_format_decode(format, &field_count, name,
>   				     name_len, errcode, region);
> +	if (field_count != 0 && fields == NULL)
> +		diag_raise();

2. You should not check more than 1 thing to test an error. When
a function can return NULL both on success and on error, you should
move the result into out parameter and return int: 0 on success, -1
on error.

>   	auto fields_guard = make_scoped_guard([=] {
>   		space_def_destroy_fields(fields, field_count);
>   	});
> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index f5ca0b5..3b31727 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
> @@ -34,6 +34,10 @@
>   #include "error.h"
>   #include "sql.h"
>   #include "msgpuck.h"
> +#include "tuple_format.h"
> +#include "schema_def.h"
> +#include "identifier.h"
> +#include "small/region.h"

3. Can you please investigate is it possible to remove some of
these headers from alter.cc?

>   
>   /**
>    * Make checks from msgpack.
> @@ -351,3 +355,149 @@ error:
> +
> +struct field_def *
> +space_format_decode(const char *data, uint32_t *out_count,
> +		    const char *space_name, uint32_t name_len,
> +		    uint32_t errcode, struct region *region)
> +{
> +	/* Type is checked by _space format. */
> +	assert(mp_typeof(*data) == MP_ARRAY);
> +	uint32_t count = mp_decode_array(&data);
> +	*out_count = count;
> +	if (count == 0)
> +		return NULL;
> +	size_t size = count * sizeof(struct field_def);
> +	struct field_def *region_defs =
> +		(struct field_def *) region_alloc(region, size);
> +	if(region_defs == NULL) {

4. + white space after 'if'.

> +		diag_set(OutOfMemory, size, "region", "new slab");

5. Please, see what arguments OutOfMemory takes:

     size_t amount, const char *allocator,
     const char *object

You should specify here "region_alloc" instead of "region", and
"region_defs" instead of "new slab".

> +		return NULL;
> +	}
> +	/*
> +	 * Nullify to prevent a case when decoding will fail in
> +	 * the middle and space_def_destroy_fields() below will
> +	 * work with garbage pointers.
> +	 */
> +	memset(region_defs, 0, size);
> +	for (uint32_t i = 0; i < count; ++i) {
> +		if(field_def_decode(&region_defs[i], &data, space_name,
> +			name_len, errcode, i, region) != 0) {

6. Broken alignment.

> +			space_def_destroy_fields(region_defs, count);
> +			return NULL;
> +		}
> +	}
> +	return region_defs;
> +}
> diff --git a/src/box/space_def.h b/src/box/space_def.h
> index 0d1e902..16188c6 100644
> --- a/src/box/space_def.h
> +++ b/src/box/space_def.h
> @@ -182,6 +182,21 @@ space_def_sizeof(uint32_t name_len, const struct field_def *fields,
>   		 uint32_t field_count, uint32_t *names_offset,
>   		 uint32_t *fields_offset, uint32_t *def_expr_offset);
>   
> +/**
> + * Decode MessagePack array of fields.
> + * @param data MessagePack array of fields.
> + * @param[out] out_count Length of a result array.
> + * @param space_name Space name to use in error messages.

7. Missing 'name_len' parameter.

> + * @param errcode Errcode for client errors.
> + * @param region Region to allocate result array.
> + *
> + * @retval Array of fields.
> + */
> +struct field_def *
> +space_format_decode(const char *data, uint32_t *out_count,
> +		    const char *space_name, uint32_t name_len,
> +		    uint32_t errcode, struct region *region);
> +
>   #if defined(__cplusplus)
>   } /* extern "C" */
>   
>