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

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx, imeevma@xxxxxxxxxxxxx
  • Date: Fri, 13 Jul 2018 19:32:24 +0300

Thanks for the patch! See 7 comments below.

On 12/07/2018 14:16, imeevma@xxxxxxxxxxxxx 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" */


Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018