[tarantool-patches] Re: [PATCH 2/5] Move some decode functions from alter.cc
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jul 13 19:32:24 MSK 2018
Thanks for the patch! See 7 comments below.
On 12/07/2018 14:16, imeevma at 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(®ion_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" */
>
>
More information about the Tarantool-patches
mailing list