* [Tarantool-patches] [PATCH 1/9] diag: return created error from diag_set()
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 2/9] error: introduce error_payload Vladislav Shpilevoy via Tarantool-patches
` (8 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
And from diag_add(). This will be helpful not to bother with
box_error_last() and diag_last_error(diag_get()) in the future
patch. It will change some attributes of a just created
ER_READONLY error to add more details.
Part of #5568
---
src/lib/core/diag.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index fa85326ad..d7d09f6fb 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -347,7 +347,7 @@ struct error *
BuildSocketError(const char *file, unsigned line, const char *socketname,
const char *format, ...);
-#define diag_set_detailed(file, line, class, ...) do { \
+#define diag_set_detailed(file, line, class, ...) ({ \
/* Preserve the original errno. */ \
int save_errno = errno; \
say_debug("%s at %s:%i", #class, file, line); \
@@ -356,19 +356,21 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
diag_set_error(diag_get(), e); \
/* Restore the errno which might have been reset. */ \
errno = save_errno; \
-} while (0)
+ e; \
+})
#define diag_set(...) \
diag_set_detailed(__FILE__, __LINE__, __VA_ARGS__)
-#define diag_add(class, ...) do { \
+#define diag_add(class, ...) ({ \
int save_errno = errno; \
say_debug("%s at %s:%i", #class, __FILE__, __LINE__); \
struct error *e; \
e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__); \
diag_add_error(diag_get(), e); \
errno = save_errno; \
-} while (0)
+ e; \
+})
#if defined(__cplusplus)
} /* extern "C" */
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* [Tarantool-patches] [PATCH 2/9] error: introduce error_payload
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 1/9] diag: return created error from diag_set() Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-08 15:14 ` Serge Petrenko via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 3/9] error: move code to struct error from ClientError Vladislav Shpilevoy via Tarantool-patches
` (7 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
It is a dictionary-like struct which stores keys with binary data
values. The values are supposed to be arbitrary MessagePack blobs:
number, string, bool, UUID, array, map, anything.
The payload is an array inside instead of a hash table because
number of keys will be <= 3 in all cases. And even when it will be
public, it is very unlikely it will be bigger.
Object of error_payload in a future patch will be stored in struct
error and will allow to extend it dynamically with more members.
This in turn is needed to extend ER_READONLY error with more
details about why it happened.
Part of #5568
---
src/lib/core/CMakeLists.txt | 1 +
src/lib/core/error_payload.c | 282 +++++++++++++++++++++
src/lib/core/error_payload.h | 166 +++++++++++++
src/lib/uuid/mp_uuid.c | 26 ++
src/lib/uuid/tt_uuid.h | 24 ++
test/unit/CMakeLists.txt | 2 +
test/unit/error.c | 461 +++++++++++++++++++++++++++++++++++
test/unit/error.result | 160 ++++++++++++
8 files changed, 1122 insertions(+)
create mode 100644 src/lib/core/error_payload.c
create mode 100644 src/lib/core/error_payload.h
create mode 100644 test/unit/error.c
create mode 100644 test/unit/error.result
diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
index 0cc742a1c..860758515 100644
--- a/src/lib/core/CMakeLists.txt
+++ b/src/lib/core/CMakeLists.txt
@@ -21,6 +21,7 @@ set(core_sources
fio.c
exception.cc
errinj.c
+ error_payload.c
reflection.c
assoc.c
util.c
diff --git a/src/lib/core/error_payload.c b/src/lib/core/error_payload.c
new file mode 100644
index 000000000..98ef08ada
--- /dev/null
+++ b/src/lib/core/error_payload.c
@@ -0,0 +1,282 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+#include "error_payload.h"
+#include "msgpuck.h"
+
+void
+error_payload_create(struct error_payload *p)
+{
+ p->count = 0;
+ p->fields = NULL;
+}
+
+void
+error_payload_destroy(struct error_payload *p)
+{
+ for (int i = 0; i < p->count; ++i) {
+ TRASH(p->fields[i]);
+ free(p->fields[i]);
+ }
+ free(p->fields);
+ TRASH(p);
+}
+
+const char *
+error_payload_get_str(const struct error_payload *e, const char *name)
+{
+ const struct error_field *f = error_payload_find(e, name);
+ if (f == NULL)
+ return NULL;
+ const char *data = f->data;
+ if (mp_typeof(*data) != MP_STR)
+ return NULL;
+ uint32_t len;
+ /*
+ * 0 is explicitly written after encoded string to be able to return
+ * them without copying.
+ */
+ data = mp_decode_str(&data, &len);
+ assert(data[len] == 0);
+ return data;
+}
+
+void
+error_payload_set_str(struct error_payload *e, const char *name,
+ const char *value)
+{
+ uint32_t len = strlen(value);
+ char *data = error_payload_prepare(
+ e, name, mp_sizeof_str(len), 1)->data;
+ /*
+ * 1 extra byte in the end is reserved to append 0 after the encoded
+ * string. To be able to return it from get() without copying.
+ */
+ *mp_encode_str(data, value, len) = 0;
+}
+
+bool
+error_payload_get_uint(const struct error_payload *e, const char *name,
+ uint64_t *value)
+{
+ const struct error_field *f = error_payload_find(e, name);
+ if (f == NULL)
+ goto not_found;
+ const char *data = f->data;
+ if (mp_typeof(*data) != MP_UINT)
+ goto not_found;
+ *value = mp_decode_uint(&data);
+ return true;
+
+not_found:
+ *value = 0;
+ return false;
+}
+
+void
+error_payload_set_uint(struct error_payload *e, const char *name,
+ uint64_t value)
+{
+ char *data = error_payload_prepare(
+ e, name, mp_sizeof_uint(value), 0)->data;
+ mp_encode_uint(data, value);
+}
+
+bool
+error_payload_get_int(const struct error_payload *e, const char *name,
+ int64_t *value)
+{
+ const struct error_field *f = error_payload_find(e, name);
+ if (f == NULL)
+ goto not_found;
+ const char *data = f->data;
+ if (mp_typeof(*data) == MP_UINT) {
+ uint64_t u = mp_decode_uint(&data);
+ if (u > INT64_MAX)
+ goto not_found;
+ *value = u;
+ return true;
+ } else if (mp_typeof(*data) == MP_INT) {
+ *value = mp_decode_int(&data);
+ return true;
+ }
+not_found:
+ *value = 0;
+ return false;
+}
+
+void
+error_payload_set_int(struct error_payload *e, const char *name, int64_t value)
+{
+ if (value >= 0)
+ return error_payload_set_uint(e, name, value);
+ char *data = error_payload_prepare(
+ e, name, mp_sizeof_int(value), 0)->data;
+ mp_encode_int(data, value);
+}
+
+bool
+error_payload_get_double(const struct error_payload *e, const char *name,
+ double *value)
+{
+ const struct error_field *f = error_payload_find(e, name);
+ if (f == NULL)
+ goto not_found;
+ const char *data = f->data;
+ if (mp_typeof(*data) == MP_DOUBLE) {
+ *value = mp_decode_double(&data);
+ return true;
+ } else if (mp_typeof(*data) == MP_FLOAT) {
+ *value = mp_decode_float(&data);
+ return true;
+ }
+not_found:
+ *value = 0;
+ return false;
+}
+
+void
+error_payload_set_double(struct error_payload *e, const char *name,
+ double value)
+{
+ char *data = error_payload_prepare(
+ e, name, mp_sizeof_double(value), 0)->data;
+ mp_encode_double(data, value);
+}
+
+bool
+error_payload_get_bool(const struct error_payload *e, const char *name,
+ bool *value)
+{
+ const struct error_field *f = error_payload_find(e, name);
+ if (f == NULL)
+ goto not_found;
+ const char *data = f->data;
+ if (mp_typeof(*data) != MP_BOOL)
+ goto not_found;
+ *value = mp_decode_bool(&data);
+ return true;
+
+not_found:
+ *value = false;
+ return false;
+}
+
+void
+error_payload_set_bool(struct error_payload *e, const char *name, bool value)
+{
+ char *data = error_payload_prepare(
+ e, name, mp_sizeof_bool(value), 0)->data;
+ mp_encode_bool(data, value);
+}
+
+const char *
+error_payload_get_mp(const struct error_payload *e, const char *name,
+ uint32_t *size)
+{
+ const struct error_field *f = error_payload_find(e, name);
+ if (f == NULL) {
+ *size = 0;
+ return NULL;
+ }
+ *size = f->size;
+ return f->data;
+}
+
+void
+error_payload_set_mp(struct error_payload *e, const char *name,
+ const char *src, uint32_t size)
+{
+ char *data;
+ if (mp_typeof(*src) == MP_STR) {
+ data = error_payload_prepare(e, name, size, 1)->data;
+ /*
+ * Add the terminating zero after the buffer so as get_str()
+ * would work without copying.
+ */
+ data[size] = 0;
+ } else {
+ data = error_payload_prepare(e, name, size, 0)->data;
+ }
+ memcpy(data, src, size);
+}
+
+void
+error_payload_unset(struct error_payload *e, const char *name)
+{
+ struct error_field **fields = e->fields;
+ for (int i = 0; i < e->count; ++i) {
+ struct error_field *f = fields[i];
+ if (strcmp(name, f->name) != 0)
+ continue;
+ TRASH(f);
+ free(f);
+ int count = --e->count;
+ if (count == 0) {
+ free(fields);
+ e->fields = NULL;
+ return;
+ }
+ /* Cyclic deletion. Order does not matter in a dictionary. */
+ fields[i] = fields[count];
+ return;
+ }
+}
+
+void
+error_payload_move(struct error_payload *dst, struct error_payload *src)
+{
+ for (int i = 0; i < dst->count; ++i) {
+ TRASH(dst->fields[i]);
+ free(dst->fields[i]);
+ }
+ free(dst->fields);
+ dst->fields = src->fields;
+ dst->count = src->count;
+ src->fields = NULL;
+ src->count = 0;
+}
+
+const struct error_field *
+error_payload_find(const struct error_payload *e, const char *name)
+{
+ for (int i = 0; i < e->count; ++i) {
+ const struct error_field *f = e->fields[i];
+ if (strcmp(name, f->name) == 0)
+ return f;
+ }
+ return NULL;
+}
+
+struct error_field *
+error_payload_prepare(struct error_payload *e, const char *name,
+ uint32_t value_size, uint32_t extra)
+{
+ struct error_field *f;
+ uint32_t name_size = strlen(name) + 1;
+ uint32_t data_offset = name_size + sizeof(*f);
+ uint32_t total = data_offset + value_size + extra;
+
+ struct error_field **fields = e->fields;
+ int count = e->count;
+ int i;
+ for (i = 0; i < count; ++i) {
+ f = fields[i];
+ if (strcmp(name, f->name) != 0)
+ continue;
+ f = xrealloc(f, total);
+ goto set;
+ }
+ e->count = ++count;
+ fields = xrealloc(fields, sizeof(fields[0]) * count);
+ e->fields = fields;
+ f = xmalloc(total);
+ memcpy(f->name, name, name_size);
+set:
+ f->data = (char *)f + data_offset;
+ f->size = value_size;
+ fields[i] = f;
+ return f;
+}
diff --git a/src/lib/core/error_payload.h b/src/lib/core/error_payload.h
new file mode 100644
index 000000000..b6bf9ceee
--- /dev/null
+++ b/src/lib/core/error_payload.h
@@ -0,0 +1,166 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+#pragma once
+
+#include "trivia/util.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+/** Single field of error payload. */
+struct error_field {
+ /** Data size. */
+ uint32_t size;
+ /** MessagePack field value. */
+ char *data;
+ /** Zero terminated field name. */
+ char name[0];
+};
+
+/** An array of key-value pairs. */
+struct error_payload {
+ /** Number of fields. */
+ int count;
+ /**
+ * Array of field pointers. Not very optimized, but the errors are
+ * supposed to be rare. Also not storing them by values simplifies
+ * addition of new fields and their removal.
+ */
+ struct error_field **fields;
+};
+
+/** Create error payload. */
+void
+error_payload_create(struct error_payload *p);
+
+/** Destroy error payload. */
+void
+error_payload_destroy(struct error_payload *p);
+
+/**
+ * Get value of a payload field as a string. If it is not string or is not
+ * found - return NULL.
+ */
+const char *
+error_payload_get_str(const struct error_payload *e, const char *name);
+
+/**
+ * Set value of a payload field to a string. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_str(struct error_payload *e, const char *name,
+ const char *value);
+
+/**
+ * Get value of a payload field as a uint. If it is not uint or is not found -
+ * return false and the out parameter is set to 0.
+ */
+bool
+error_payload_get_uint(const struct error_payload *e, const char *name,
+ uint64_t *value);
+
+/**
+ * Set value of a payload field to a uint. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_uint(struct error_payload *e, const char *name,
+ uint64_t value);
+
+/**
+ * Get value of a payload field as an int. If it is not int, or is not found, or
+ * does not fit int64_t - return false and the out parameter is set to 0.
+ */
+bool
+error_payload_get_int(const struct error_payload *e, const char *name,
+ int64_t *value);
+
+/**
+ * Set value of a payload field to an int. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_int(struct error_payload *e, const char *name, int64_t value);
+
+/**
+ * Get value of a payload field as a double. If it is not double or is not
+ * found - return false and the out parameter is set to 0.
+ */
+bool
+error_payload_get_double(const struct error_payload *e, const char *name,
+ double *value);
+
+/**
+ * Set value of a payload field to a double. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_double(struct error_payload *e, const char *name,
+ double value);
+
+/**
+ * Get value of a payload field as a bool. If it is not bool or is not found -
+ * return false and the out parameter is set to false.
+ */
+bool
+error_payload_get_bool(const struct error_payload *e, const char *name,
+ bool *value);
+
+/**
+ * Set value of a payload field to a bool. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_bool(struct error_payload *e, const char *name, bool value);
+
+/**
+ * Get MessagePack value of a payload field. If it is not found - return NULL
+ * and the out parameter is set to 0.
+ */
+const char *
+error_payload_get_mp(const struct error_payload *e, const char *name,
+ uint32_t *size);
+
+/**
+ * Set value of a payload field to a MessagePack buffer. If the field existed
+ * before, it is overwritten.
+ */
+void
+error_payload_set_mp(struct error_payload *e, const char *name,
+ const char *src, uint32_t size);
+
+/** Remove the given field from the payload. */
+void
+error_payload_unset(struct error_payload *e, const char *name);
+
+/**
+ * Move all fields of one payload into another. Old fields of the destination
+ * are all deleted. The source stays valid but empty.
+ */
+void
+error_payload_move(struct error_payload *dst, struct error_payload *src);
+
+/** Find a payload field by name. */
+const struct error_field *
+error_payload_find(const struct error_payload *e, const char *name);
+
+/**
+ * Prepare a payload field to get a new value. If the field didn't exist, it is
+ * added. If it existed then it is reallocated if was necessary and should be
+ * overwritten. Name is already filled in the field. Only need to fill the
+ * value.
+ * Extra parameter allows to allocate extra memory for arbitrary usage after the
+ * error field and its value.
+ */
+struct error_field *
+error_payload_prepare(struct error_payload *e, const char *name,
+ uint32_t value_size, uint32_t extra);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c
index b2341ae36..a60716eb5 100644
--- a/src/lib/uuid/mp_uuid.c
+++ b/src/lib/uuid/mp_uuid.c
@@ -32,6 +32,7 @@
#include "mp_uuid.h"
#include "msgpuck.h"
#include "mp_extension_types.h"
+#include "error_payload.h"
inline uint32_t
mp_sizeof_uuid(void)
@@ -113,3 +114,28 @@ mp_fprint_uuid(FILE *file, const char **data, uint32_t len)
return -1;
return fprintf(file, "%s", tt_uuid_str(&uuid));
}
+
+bool
+error_payload_get_uuid(const struct error_payload *e, const char *name,
+ struct tt_uuid *uuid)
+{
+ const struct error_field *f = error_payload_find(e, name);
+ if (f == NULL)
+ goto not_found;
+ const char *data = f->data;
+ if (mp_decode_uuid(&data, uuid) == NULL)
+ goto not_found;
+ return true;
+
+not_found:
+ *uuid = uuid_nil;
+ return false;
+}
+
+void
+error_payload_set_uuid(struct error_payload *e, const char *name,
+ const struct tt_uuid *uuid)
+{
+ char *data = error_payload_prepare(e, name, mp_sizeof_uuid(), 0)->data;
+ mp_encode_uuid(data, uuid);
+}
diff --git a/src/lib/uuid/tt_uuid.h b/src/lib/uuid/tt_uuid.h
index 70c3b98b1..866764e77 100644
--- a/src/lib/uuid/tt_uuid.h
+++ b/src/lib/uuid/tt_uuid.h
@@ -41,6 +41,8 @@
extern "C" {
#endif
+struct error_payload;
+
enum { UUID_LEN = 16, UUID_STR_LEN = 36 };
/**
@@ -182,6 +184,28 @@ tt_uuid_str(const struct tt_uuid *uu);
int
tt_uuid_from_strl(const char *in, size_t len, struct tt_uuid *uu);
+/**
+ * Error payload couldn't be implemented in libcore because requires UUID type
+ * and its methods. That would be a cyclic dep. Thus in places where need to use
+ * UUID fields in error payload also need to include libuuid.
+ */
+
+/**
+ * Get value of a payload field as a UUID. If it is not UUID or is not found -
+ * return false and the out parameter is set to UUID with zeros.
+ */
+bool
+error_payload_get_uuid(const struct error_payload *e, const char *name,
+ struct tt_uuid *uuid);
+
+/**
+ * Set value of a payload field to a UUID. If the field existed before, it is
+ * overwritten.
+ */
+void
+error_payload_set_uuid(struct error_payload *e, const char *name,
+ const struct tt_uuid *uuid);
+
#if defined(__cplusplus)
} /* extern "C" */
#endif
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index ae8b5b9ac..0bb71ccce 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -60,6 +60,8 @@ add_executable(xmalloc.test xmalloc.c core_test_utils.c)
target_link_libraries(xmalloc.test unit)
add_executable(datetime.test datetime.c)
target_link_libraries(datetime.test tzcode core cdt unit)
+add_executable(error.test error.c core_test_utils.c)
+target_link_libraries(error.test unit core uuid)
add_executable(bps_tree.test bps_tree.cc)
target_link_libraries(bps_tree.test small misc)
diff --git a/test/unit/error.c b/test/unit/error.c
new file mode 100644
index 000000000..ef17c56ec
--- /dev/null
+++ b/test/unit/error.c
@@ -0,0 +1,461 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+#include "error_payload.h"
+#include "msgpuck.h"
+#include "random.h"
+#include "unit.h"
+#include "uuid/mp_uuid.h"
+#include "uuid/tt_uuid.h"
+
+#include <float.h>
+
+static void
+test_payload_field_str(void)
+{
+ header();
+ plan(15);
+
+ struct error_payload p;
+ error_payload_create(&p);
+ is(p.count, 0, "no fields in the beginning");
+ is(error_payload_get_str(&p, "key"), NULL, "get_str() empty");
+
+ error_payload_set_str(&p, "key1", "value1");
+ is(p.count, 1, "++count");
+ is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0,
+ "get_str() finds");
+
+ error_payload_set_str(&p, "key2", "value2");
+ is(p.count, 2, "++count");
+ is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0,
+ "get_str() finds old");
+ is(strcmp(error_payload_get_str(&p, "key2"), "value2"), 0,
+ "get_str() finds new");
+ is(error_payload_find(&p, "key1")->size,
+ mp_sizeof_str(strlen("value1")),
+ "size does not include terminating zero");
+
+ error_payload_set_str(&p, "key1", "new_value1");
+ is(p.count, 2, "field count is same");
+ is(strcmp(error_payload_get_str(&p, "key1"), "new_value1"), 0,
+ "get_str() finds new value");
+ is(strcmp(error_payload_get_str(&p, "key2"), "value2"), 0,
+ "get_str() finds other key old value");
+
+ error_payload_unset(&p, "key2");
+ is(p.count, 1, "--count");
+ is(strcmp(error_payload_get_str(&p, "key1"), "new_value1"), 0,
+ "get_str() finds new value");
+ is(error_payload_get_str(&p, "key2"), NULL,
+ "get_str() can't find deleted value");
+
+ error_payload_set_uint(&p, "key2", 1);
+ is(error_payload_get_str(&p, "key2"), NULL, "wrong type");
+
+ error_payload_destroy(&p);
+
+ check_plan();
+ footer();
+}
+
+static void
+test_payload_field_uint(void)
+{
+ header();
+ plan(17);
+
+ struct error_payload p;
+ error_payload_create(&p);
+ uint64_t val = 1;
+ ok(!error_payload_get_uint(&p, "key", &val) && val == 0,
+ "get_uint() empty");
+
+ error_payload_set_uint(&p, "key1", 1);
+ is(p.count, 1, "++count");
+ ok(error_payload_get_uint(&p, "key1", &val), "get_uint() finds");
+ is(val, 1, "value match");
+
+ val = 100;
+ ok(!error_payload_get_uint(&p, "key2", &val), "get_uint() fails");
+ is(val, 0, "value is default");
+
+ is(error_payload_find(&p, "key1")->size, mp_sizeof_uint(1),
+ "small number size");
+
+ error_payload_set_uint(&p, "key2", UINT32_MAX);
+ ok(error_payload_get_uint(&p, "key2", &val), "get_uint() 32 bit");
+ is(val, UINT32_MAX, "value match");
+ is(error_payload_find(&p, "key2")->size, mp_sizeof_uint(UINT32_MAX),
+ "middle number size");
+ is(p.count, 2, "field count is same");
+
+ error_payload_set_uint(&p, "key1", UINT64_MAX);
+ ok(error_payload_get_uint(&p, "key1", &val) && val == UINT64_MAX,
+ "value 1");
+ ok(error_payload_get_uint(&p, "key2", &val) && val == UINT32_MAX,
+ "value 2");
+
+ error_payload_unset(&p, "key2");
+ is(p.count, 1, "--count");
+ ok(error_payload_get_uint(&p, "key1", &val) && val == UINT64_MAX,
+ "remained value");
+ ok(!error_payload_get_uint(&p, "key2", &val) && val == 0,
+ "deleted value");
+
+ error_payload_set_str(&p, "key2", "1");
+ ok(!error_payload_get_uint(&p, "key2", &val) && val == 0, "wrong type");
+
+ error_payload_destroy(&p);
+
+ check_plan();
+ footer();
+}
+
+static void
+test_payload_field_int(void)
+{
+ header();
+ plan(20);
+
+ struct error_payload p;
+ error_payload_create(&p);
+ int64_t val = 1;
+ ok(!error_payload_get_int(&p, "key", &val) && val == 0,
+ "get_int() empty");
+
+ error_payload_set_int(&p, "key1", 1);
+ is(p.count, 1, "++count");
+ ok(error_payload_get_int(&p, "key1", &val), "get_int() finds");
+ is(val, 1, "value match");
+
+ val = 100;
+ ok(!error_payload_get_int(&p, "key2", &val), "get_int() fails");
+ is(val, 0, "value is default");
+
+ is(error_payload_find(&p, "key1")->size, mp_sizeof_uint(1),
+ "small number size");
+
+ error_payload_set_int(&p, "key2", UINT32_MAX);
+ ok(error_payload_get_int(&p, "key2", &val), "get_int() 32 bit");
+ is(val, UINT32_MAX, "value match");
+ is(error_payload_find(&p, "key2")->size, mp_sizeof_uint(UINT32_MAX),
+ "middle number size");
+ is(p.count, 2, "field count is same");
+
+ error_payload_set_int(&p, "key1", INT64_MAX);
+ ok(error_payload_get_int(&p, "key1", &val) && val == INT64_MAX,
+ "value 1");
+ ok(error_payload_get_int(&p, "key2", &val) && val == UINT32_MAX,
+ "value 2");
+
+ error_payload_unset(&p, "key2");
+ is(p.count, 1, "--count");
+ ok(error_payload_get_int(&p, "key1", &val) && val == INT64_MAX,
+ "remained value");
+ ok(!error_payload_get_int(&p, "key2", &val) && val == 0,
+ "deleted value");
+
+ error_payload_set_str(&p, "key2", "1");
+ ok(!error_payload_get_int(&p, "key2", &val) && val == 0, "wrong type");
+
+ error_payload_set_uint(&p, "key2", (uint64_t)INT64_MAX + 1);
+ ok(!error_payload_get_int(&p, "key2", &val) && val == 0, "overflow");
+
+ error_payload_set_uint(&p, "key2", 100);
+ ok(error_payload_get_int(&p, "key2", &val) && val == 100, "conversion");
+
+ error_payload_set_int(&p, "key2", INT64_MIN);
+ ok(error_payload_get_int(&p, "key2", &val) && val == INT64_MIN,
+ "min value");
+
+ error_payload_destroy(&p);
+
+ check_plan();
+ footer();
+}
+
+static void
+test_payload_field_double(void)
+{
+ header();
+ plan(13);
+
+ struct error_payload p;
+ error_payload_create(&p);
+ double val = 1;
+ ok(!error_payload_get_double(&p, "key", &val) && val == 0,
+ "get_double() empty");
+
+ error_payload_set_double(&p, "key1", 1.5);
+ is(p.count, 1, "++count");
+ ok(error_payload_get_double(&p, "key1", &val), "get_double() finds");
+ is(val, 1.5, "value match");
+
+ val = 1;
+ ok(!error_payload_get_double(&p, "key2", &val), "get_double() fails");
+ is(val, 0, "value is default");
+
+ is(error_payload_find(&p, "key1")->size, mp_sizeof_double(1.5), "size");
+
+ error_payload_set_double(&p, "key2", DBL_MAX);
+ ok(error_payload_get_double(&p, "key1", &val) && val == 1.5, "value 1");
+ ok(error_payload_get_double(&p, "key2", &val) && val == DBL_MAX,
+ "value 2");
+
+ error_payload_unset(&p, "key2");
+ is(p.count, 1, "--count");
+ ok(error_payload_get_double(&p, "key1", &val) && val == 1.5,
+ "remained value");
+ ok(!error_payload_get_double(&p, "key2", &val) && val == 0,
+ "deleted value");
+
+ error_payload_set_str(&p, "key2", "1");
+ ok(!error_payload_get_double(&p, "key2", &val) && val == 0,
+ "wrong type");
+
+ error_payload_destroy(&p);
+
+ check_plan();
+ footer();
+}
+
+static void
+test_payload_field_bool(void)
+{
+ header();
+ plan(13);
+
+ struct error_payload p;
+ error_payload_create(&p);
+ bool val = true;
+ ok(!error_payload_get_bool(&p, "key", &val) && !val,
+ "get_bool() empty");
+
+ error_payload_set_bool(&p, "key1", true);
+ is(p.count, 1, "++count");
+ ok(error_payload_get_bool(&p, "key1", &val), "get_bool() finds");
+ ok(val, "value match");
+
+ val = true;
+ ok(!error_payload_get_bool(&p, "key2", &val), "get_bool() fails");
+ ok(!val, "value is default");
+
+ error_payload_set_bool(&p, "key2", false);
+ ok(error_payload_get_bool(&p, "key2", &val), "get_bool() finds");
+ ok(!val, "value match");
+
+ is(error_payload_find(&p, "key1")->size, mp_sizeof_bool(true), "size");
+
+ error_payload_unset(&p, "key2");
+ is(p.count, 1, "--count");
+ ok(error_payload_get_bool(&p, "key1", &val) && val, "remained value");
+ ok(!error_payload_get_bool(&p, "key2", &val) && !val, "deleted value");
+
+ error_payload_set_str(&p, "key2", "true");
+ ok(!error_payload_get_bool(&p, "key2", &val) && !val, "wrong type");
+
+ error_payload_destroy(&p);
+
+ check_plan();
+ footer();
+}
+
+static void
+test_payload_field_uuid(void)
+{
+ header();
+ plan(17);
+
+ struct error_payload p;
+ error_payload_create(&p);
+ struct tt_uuid val1;
+ tt_uuid_create(&val1);
+ ok(!error_payload_get_uuid(&p, "key", &val1), "get_uuid() empty");
+ ok(tt_uuid_is_nil(&val1), "default value");
+
+ tt_uuid_create(&val1);
+ error_payload_set_uuid(&p, "key1", &val1);
+ is(p.count, 1, "++count");
+ struct tt_uuid val2;
+ ok(error_payload_get_uuid(&p, "key1", &val2), "get_uuid() finds");
+ ok(tt_uuid_is_equal(&val1, &val2), "value match");
+
+ ok(!error_payload_get_uuid(&p, "key2", &val2), "get_uuid() fails");
+ ok(tt_uuid_is_nil(&val2), "value is default");
+
+ tt_uuid_create(&val2);
+ error_payload_set_uuid(&p, "key2", &val2);
+ struct tt_uuid val3;
+ ok(error_payload_get_uuid(&p, "key2", &val3), "get_uuid() finds");
+ ok(tt_uuid_is_equal(&val3, &val2), "value match");
+
+ is(error_payload_find(&p, "key1")->size, mp_sizeof_uuid(), "size");
+
+ error_payload_unset(&p, "key2");
+ is(p.count, 1, "--count");
+ ok(error_payload_get_uuid(&p, "key1", &val3), "remained value");
+ ok(tt_uuid_is_equal(&val1, &val3), "value match");
+ ok(!error_payload_get_uuid(&p, "key2", &val3), "deleted value");
+ ok(tt_uuid_is_nil(&val3), "value match");
+
+ error_payload_set_str(&p, "key2", "1");
+ ok(!error_payload_get_uuid(&p, "key2", &val3), "wrong type");
+ ok(tt_uuid_is_nil(&val3), "value match");
+
+ error_payload_destroy(&p);
+
+ check_plan();
+ footer();
+}
+
+static void
+test_payload_field_mp(void)
+{
+ header();
+ plan(6);
+ char buf[1024];
+ char *data;
+ const char *cdata;
+ uint32_t size;
+
+ struct error_payload p;
+ error_payload_create(&p);
+
+ data = mp_encode_str(buf, "value1", 6);
+ error_payload_set_mp(&p, "key1", buf, data - buf);
+ is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0, "mp str");
+
+ cdata = error_payload_get_mp(&p, "key1", &size);
+ is(memcmp(cdata, buf, size), 0, "mp str cmp");
+
+ data = mp_encode_uint(buf, 100);
+ error_payload_set_mp(&p, "key2", buf, data - buf);
+ uint64_t val;
+ ok(error_payload_get_uint(&p, "key2", &val) && val == 100, "mp uint");
+
+ cdata = error_payload_get_mp(&p, "key2", &size);
+ is(memcmp(cdata, buf, size), 0, "mp uint cmp");
+
+ data = mp_encode_array(buf, 1);
+ data = mp_encode_uint(data, 2);
+ error_payload_set_mp(&p, "key3", buf, data - buf);
+
+ cdata = error_payload_get_mp(&p, "key3", &size);
+ is(memcmp(cdata, buf, size), 0, "mp array");
+
+ ok(!error_payload_get_uint(&p, "key3", &val) && val == 0,
+ "mp uint from array");
+
+ error_payload_destroy(&p);
+
+ check_plan();
+ footer();
+}
+
+static void
+test_payload_unset(void)
+{
+ header();
+ plan(13);
+
+ struct error_payload p;
+ error_payload_create(&p);
+
+ error_payload_set_uint(&p, "key1", 1);
+ error_payload_set_uint(&p, "key2", 2);
+ error_payload_set_uint(&p, "key3", 3);
+ error_payload_set_uint(&p, "key4", 4);
+ error_payload_set_uint(&p, "key5", 5);
+
+ error_payload_unset(&p, "key5");
+ is(p.count, 4, "unset last, count");
+ is(error_payload_find(&p, "key5"), NULL, "unset last, key");
+
+ error_payload_unset(&p, "key1");
+ is(p.count, 3, "unset first, count");
+ is(error_payload_find(&p, "key1"), NULL, "unset first, key");
+
+ uint64_t val;
+ ok(error_payload_get_uint(&p, "key2", &val) && val == 2, "check key2");
+ ok(error_payload_get_uint(&p, "key3", &val) && val == 3, "check key3");
+ ok(error_payload_get_uint(&p, "key4", &val) && val == 4, "check key4");
+
+ is(strcmp(p.fields[0]->name, "key4"), 0, "deletion is cyclic");
+
+ error_payload_unset(&p, "key2");
+ is(p.count, 2, "unset middle, count");
+ is(error_payload_find(&p, "key2"), NULL, "unset middle, key");
+ ok(error_payload_get_uint(&p, "key3", &val) && val == 3, "check key3");
+ ok(error_payload_get_uint(&p, "key4", &val) && val == 4, "check key4");
+
+ error_payload_unset(&p, "key3");
+ error_payload_unset(&p, "key4");
+
+ is(p.count, 0, "unset all");
+
+ error_payload_destroy(&p);
+
+ check_plan();
+ footer();
+}
+
+static void
+test_payload_move(void)
+{
+ header();
+ plan(7);
+
+ struct error_payload p1, p2;
+ error_payload_create(&p1);
+ error_payload_create(&p2);
+
+ error_payload_move(&p1, &p2);
+ ok(p1.count == 0 && p1.fields == NULL, "empty");
+
+ error_payload_set_str(&p1, "key", "value");
+ error_payload_move(&p1, &p2);
+ ok(p1.count == 0 && p1.fields == NULL, "emptied on move");
+
+ error_payload_set_str(&p1, "key", "value");
+ error_payload_set_str(&p2, "key1", "value1");
+ error_payload_set_str(&p2, "key2", "value2");
+ error_payload_move(&p1, &p2);
+ is(p1.count, 2, "got 2 fields");
+ isnt(p1.fields, NULL, "got 2 fields");
+ is(strcmp(error_payload_get_str(&p1, "key1"), "value1"), 0, "key1");
+ is(strcmp(error_payload_get_str(&p1, "key2"), "value2"), 0, "key2");
+ is(error_payload_get_str(&p1, "key"), NULL, "key");
+
+ error_payload_destroy(&p2);
+ error_payload_destroy(&p1);
+
+ check_plan();
+ footer();
+}
+
+int
+main(void)
+{
+ header();
+ plan(9);
+
+ random_init();
+
+ test_payload_field_str();
+ test_payload_field_uint();
+ test_payload_field_int();
+ test_payload_field_double();
+ test_payload_field_bool();
+ test_payload_field_uuid();
+ test_payload_field_mp();
+ test_payload_unset();
+ test_payload_move();
+
+ random_free();
+
+ footer();
+ return check_plan();
+}
diff --git a/test/unit/error.result b/test/unit/error.result
new file mode 100644
index 000000000..f0c4266e6
--- /dev/null
+++ b/test/unit/error.result
@@ -0,0 +1,160 @@
+ *** main ***
+1..9
+ *** test_payload_field_str ***
+ 1..15
+ ok 1 - no fields in the beginning
+ ok 2 - get_str() empty
+ ok 3 - ++count
+ ok 4 - get_str() finds
+ ok 5 - ++count
+ ok 6 - get_str() finds old
+ ok 7 - get_str() finds new
+ ok 8 - size does not include terminating zero
+ ok 9 - field count is same
+ ok 10 - get_str() finds new value
+ ok 11 - get_str() finds other key old value
+ ok 12 - --count
+ ok 13 - get_str() finds new value
+ ok 14 - get_str() can't find deleted value
+ ok 15 - wrong type
+ok 1 - subtests
+ *** test_payload_field_str: done ***
+ *** test_payload_field_uint ***
+ 1..17
+ ok 1 - get_uint() empty
+ ok 2 - ++count
+ ok 3 - get_uint() finds
+ ok 4 - value match
+ ok 5 - get_uint() fails
+ ok 6 - value is default
+ ok 7 - small number size
+ ok 8 - get_uint() 32 bit
+ ok 9 - value match
+ ok 10 - middle number size
+ ok 11 - field count is same
+ ok 12 - value 1
+ ok 13 - value 2
+ ok 14 - --count
+ ok 15 - remained value
+ ok 16 - deleted value
+ ok 17 - wrong type
+ok 2 - subtests
+ *** test_payload_field_uint: done ***
+ *** test_payload_field_int ***
+ 1..20
+ ok 1 - get_int() empty
+ ok 2 - ++count
+ ok 3 - get_int() finds
+ ok 4 - value match
+ ok 5 - get_int() fails
+ ok 6 - value is default
+ ok 7 - small number size
+ ok 8 - get_int() 32 bit
+ ok 9 - value match
+ ok 10 - middle number size
+ ok 11 - field count is same
+ ok 12 - value 1
+ ok 13 - value 2
+ ok 14 - --count
+ ok 15 - remained value
+ ok 16 - deleted value
+ ok 17 - wrong type
+ ok 18 - overflow
+ ok 19 - conversion
+ ok 20 - min value
+ok 3 - subtests
+ *** test_payload_field_int: done ***
+ *** test_payload_field_double ***
+ 1..13
+ ok 1 - get_double() empty
+ ok 2 - ++count
+ ok 3 - get_double() finds
+ ok 4 - value match
+ ok 5 - get_double() fails
+ ok 6 - value is default
+ ok 7 - size
+ ok 8 - value 1
+ ok 9 - value 2
+ ok 10 - --count
+ ok 11 - remained value
+ ok 12 - deleted value
+ ok 13 - wrong type
+ok 4 - subtests
+ *** test_payload_field_double: done ***
+ *** test_payload_field_bool ***
+ 1..13
+ ok 1 - get_bool() empty
+ ok 2 - ++count
+ ok 3 - get_bool() finds
+ ok 4 - value match
+ ok 5 - get_bool() fails
+ ok 6 - value is default
+ ok 7 - get_bool() finds
+ ok 8 - value match
+ ok 9 - size
+ ok 10 - --count
+ ok 11 - remained value
+ ok 12 - deleted value
+ ok 13 - wrong type
+ok 5 - subtests
+ *** test_payload_field_bool: done ***
+ *** test_payload_field_uuid ***
+ 1..17
+ ok 1 - get_uuid() empty
+ ok 2 - default value
+ ok 3 - ++count
+ ok 4 - get_uuid() finds
+ ok 5 - value match
+ ok 6 - get_uuid() fails
+ ok 7 - value is default
+ ok 8 - get_uuid() finds
+ ok 9 - value match
+ ok 10 - size
+ ok 11 - --count
+ ok 12 - remained value
+ ok 13 - value match
+ ok 14 - deleted value
+ ok 15 - value match
+ ok 16 - wrong type
+ ok 17 - value match
+ok 6 - subtests
+ *** test_payload_field_uuid: done ***
+ *** test_payload_field_mp ***
+ 1..6
+ ok 1 - mp str
+ ok 2 - mp str cmp
+ ok 3 - mp uint
+ ok 4 - mp uint cmp
+ ok 5 - mp array
+ ok 6 - mp uint from array
+ok 7 - subtests
+ *** test_payload_field_mp: done ***
+ *** test_payload_unset ***
+ 1..13
+ ok 1 - unset last, count
+ ok 2 - unset last, key
+ ok 3 - unset first, count
+ ok 4 - unset first, key
+ ok 5 - check key2
+ ok 6 - check key3
+ ok 7 - check key4
+ ok 8 - deletion is cyclic
+ ok 9 - unset middle, count
+ ok 10 - unset middle, key
+ ok 11 - check key3
+ ok 12 - check key4
+ ok 13 - unset all
+ok 8 - subtests
+ *** test_payload_unset: done ***
+ *** test_payload_move ***
+ 1..7
+ ok 1 - empty
+ ok 2 - emptied on move
+ ok 3 - got 2 fields
+ ok 4 - got 2 fields
+ ok 5 - key1
+ ok 6 - key2
+ ok 7 - key
+ok 9 - subtests
+ *** test_payload_move: done ***
+ *** main: done ***
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 2/9] error: introduce error_payload
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 2/9] error: introduce error_payload Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-08 15:14 ` Serge Petrenko via Tarantool-patches
2021-11-11 23:50 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-08 15:14 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, vdavydov
06.11.2021 02:56, Vladislav Shpilevoy пишет:
> It is a dictionary-like struct which stores keys with binary data
> values. The values are supposed to be arbitrary MessagePack blobs:
> number, string, bool, UUID, array, map, anything.
>
> The payload is an array inside instead of a hash table because
> number of keys will be <= 3 in all cases. And even when it will be
> public, it is very unlikely it will be bigger.
>
> Object of error_payload in a future patch will be stored in struct
> error and will allow to extend it dynamically with more members.
>
> This in turn is needed to extend ER_READONLY error with more
> details about why it happened.
>
> Part of #5568
Hi! Thanks for the patch!
Please, find 6 comments below.
> ---
> src/lib/core/CMakeLists.txt | 1 +
> src/lib/core/error_payload.c | 282 +++++++++++++++++++++
> src/lib/core/error_payload.h | 166 +++++++++++++
> src/lib/uuid/mp_uuid.c | 26 ++
> src/lib/uuid/tt_uuid.h | 24 ++
> test/unit/CMakeLists.txt | 2 +
> test/unit/error.c | 461 +++++++++++++++++++++++++++++++++++
> test/unit/error.result | 160 ++++++++++++
> 8 files changed, 1122 insertions(+)
> create mode 100644 src/lib/core/error_payload.c
> create mode 100644 src/lib/core/error_payload.h
> create mode 100644 test/unit/error.c
> create mode 100644 test/unit/error.result
>
> diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
> index 0cc742a1c..860758515 100644
> --- a/src/lib/core/CMakeLists.txt
> +++ b/src/lib/core/CMakeLists.txt
> @@ -21,6 +21,7 @@ set(core_sources
> fio.c
> exception.cc
> errinj.c
> + error_payload.c
> reflection.c
> assoc.c
> util.c
> diff --git a/src/lib/core/error_payload.c b/src/lib/core/error_payload.c
> new file mode 100644
> index 000000000..98ef08ada
> --- /dev/null
> +++ b/src/lib/core/error_payload.c
> @@ -0,0 +1,282 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +#include "error_payload.h"
> +#include "msgpuck.h"
> +
> +void
> +error_payload_create(struct error_payload *p)
> +{
> + p->count = 0;
> + p->fields = NULL;
> +}
> +
> +void
> +error_payload_destroy(struct error_payload *p)
> +{
> + for (int i = 0; i < p->count; ++i) {
> + TRASH(p->fields[i]);
> + free(p->fields[i]);
> + }
> + free(p->fields);
> + TRASH(p);
> +}
> +
> +const char *
> +error_payload_get_str(const struct error_payload *e, const char *name)
> +{
> + const struct error_field *f = error_payload_find(e, name);
> + if (f == NULL)
> + return NULL;
> + const char *data = f->data;
> + if (mp_typeof(*data) != MP_STR)
> + return NULL;
> + uint32_t len;
> + /*
> + * 0 is explicitly written after encoded string to be able to return
> + * them without copying.
> + */
> + data = mp_decode_str(&data, &len);
> + assert(data[len] == 0);
1. Nit: `data[len] == '\0'`. Here and everywhere else. This is up to you,
I'm just more used to '\0'.
> + return data;
> +}
> +
> +void
> +error_payload_set_str(struct error_payload *e, const char *name,
> + const char *value)
> +{
> + uint32_t len = strlen(value);
> + char *data = error_payload_prepare(
> + e, name, mp_sizeof_str(len), 1)->data;
> + /*
> + * 1 extra byte in the end is reserved to append 0 after the encoded
> + * string. To be able to return it from get() without copying.
> + */
2. This comment is almost the same as the one in payload_get_str().
I propose to leave only one of them. Preferably the one in set_str().
> + *mp_encode_str(data, value, len) = 0;
> +}
> +
> +bool
> +error_payload_get_uint(const struct error_payload *e, const char *name,
> + uint64_t *value)
> +{
> + const struct error_field *f = error_payload_find(e, name);
> + if (f == NULL)
> + goto not_found;
> + const char *data = f->data;
> + if (mp_typeof(*data) != MP_UINT)
> + goto not_found;
> + *value = mp_decode_uint(&data);
> + return true;
> +
> +not_found:
> + *value = 0;
> + return false;
> +}
> +
3. Consider this diff here.
(Similar places below do need a label, but this one doesn't).
============================
diff --git a/src/lib/core/error_payload.c b/src/lib/core/error_payload.c
index 98ef08ada..cfa30919e 100644
--- a/src/lib/core/error_payload.c
+++ b/src/lib/core/error_payload.c
@@ -62,17 +62,14 @@ error_payload_get_uint(const struct error_payload
*e, const char *name,
uint64_t *value)
{
const struct error_field *f = error_payload_find(e, name);
+ *value = 0;
if (f == NULL)
- goto not_found;
+ return false;
const char *data = f->data;
if (mp_typeof(*data) != MP_UINT)
- goto not_found;
+ return false;
*value = mp_decode_uint(&data);
return true;
-
-not_found:
- *value = 0;
- return false;
}
void
============================
> +void
> +error_payload_set_uint(struct error_payload *e, const char *name,
> + uint64_t value)
> +{
> + char *data = error_payload_prepare(
> + e, name, mp_sizeof_uint(value), 0)->data;
> + mp_encode_uint(data, value);
> +}
> +
> +bool
> +error_payload_get_int(const struct error_payload *e, const char *name,
> + int64_t *value)
> +{
> + const struct error_field *f = error_payload_find(e, name);
> + if (f == NULL)
> + goto not_found;
> + const char *data = f->data;
> + if (mp_typeof(*data) == MP_UINT) {
> + uint64_t u = mp_decode_uint(&data);
> + if (u > INT64_MAX)
> + goto not_found;
> + *value = u;
> + return true;
> + } else if (mp_typeof(*data) == MP_INT) {
> + *value = mp_decode_int(&data);
> + return true;
> + }
> +not_found:
> + *value = 0;
> + return false;
> +}
> +
> +void
> +error_payload_set_int(struct error_payload *e, const char *name, int64_t value)
> +{
> + if (value >= 0)
> + return error_payload_set_uint(e, name, value);
> + char *data = error_payload_prepare(
> + e, name, mp_sizeof_int(value), 0)->data;
> + mp_encode_int(data, value);
> +}
> +
> +bool
> +error_payload_get_double(const struct error_payload *e, const char *name,
> + double *value)
> +{
> + const struct error_field *f = error_payload_find(e, name);
> + if (f == NULL)
> + goto not_found;
> + const char *data = f->data;
> + if (mp_typeof(*data) == MP_DOUBLE) {
> + *value = mp_decode_double(&data);
> + return true;
> + } else if (mp_typeof(*data) == MP_FLOAT) {
> + *value = mp_decode_float(&data);
> + return true;
> + }
4. Why do you check for both MP_DOUBLE and MP_FLOAT here?
You only encode MP_DOUBLE.
I think we might have set_float() and get_float() one day, but looks
like they're not needed now.
get_float() would only accept MP_FLOAT then, and get_double() should
only accept MP_DOUBLE.
> +not_found:
> + *value = 0;
> + return false;
> +}
> +
> +void
> +error_payload_set_double(struct error_payload *e, const char *name,
> + double value)
> +{
> + char *data = error_payload_prepare(
> + e, name, mp_sizeof_double(value), 0)->data;
> + mp_encode_double(data, value);
> +}
> +
> +bool
> +error_payload_get_bool(const struct error_payload *e, const char *name,
> + bool *value)
> +{
> + const struct error_field *f = error_payload_find(e, name);
> + if (f == NULL)
> + goto not_found;
> + const char *data = f->data;
> + if (mp_typeof(*data) != MP_BOOL)
> + goto not_found;
> + *value = mp_decode_bool(&data);
> + return true;
> +
> +not_found:
> + *value = false;
> + return false;
> +}
> +
> +void
> +error_payload_set_bool(struct error_payload *e, const char *name, bool value)
> +{
> + char *data = error_payload_prepare(
> + e, name, mp_sizeof_bool(value), 0)->data;
> + mp_encode_bool(data, value);
> +}
> +
> +const char *
> +error_payload_get_mp(const struct error_payload *e, const char *name,
> + uint32_t *size)
> +{
> + const struct error_field *f = error_payload_find(e, name);
> + if (f == NULL) {
> + *size = 0;
> + return NULL;
> + }
> + *size = f->size;
> + return f->data;
> +}
> +
> +void
> +error_payload_set_mp(struct error_payload *e, const char *name,
> + const char *src, uint32_t size)
> +{
> + char *data;
> + if (mp_typeof(*src) == MP_STR) {
> + data = error_payload_prepare(e, name, size, 1)->data;
> + /*
> + * Add the terminating zero after the buffer so as get_str()
> + * would work without copying.
> + */
5. Maybe shorten this comment to "\sa error_payload_set_str()" ?
> + data[size] = 0;
> + } else {
> + data = error_payload_prepare(e, name, size, 0)->data;
> + }
> + memcpy(data, src, size);
> +}
> +
> +void
> +error_payload_unset(struct error_payload *e, const char *name)
> +{
> + struct error_field **fields = e->fields;
> + for (int i = 0; i < e->count; ++i) {
> + struct error_field *f = fields[i];
> + if (strcmp(name, f->name) != 0)
> + continue;
> + TRASH(f);
> + free(f);
> + int count = --e->count;
> + if (count == 0) {
> + free(fields);
> + e->fields = NULL;
> + return;
> + }
> + /* Cyclic deletion. Order does not matter in a dictionary. */
> + fields[i] = fields[count];
> + return;
> + }
> +}
> +
> +void
> +error_payload_move(struct error_payload *dst, struct error_payload *src)
> +{
> + for (int i = 0; i < dst->count; ++i) {
> + TRASH(dst->fields[i]);
> + free(dst->fields[i]);
> + }
> + free(dst->fields);
> + dst->fields = src->fields;
> + dst->count = src->count;
> + src->fields = NULL;
> + src->count = 0;
> +}
> +
> +const struct error_field *
> +error_payload_find(const struct error_payload *e, const char *name)
> +{
> + for (int i = 0; i < e->count; ++i) {
> + const struct error_field *f = e->fields[i];
> + if (strcmp(name, f->name) == 0)
> + return f;
> + }
> + return NULL;
> +}
> +
> +struct error_field *
> +error_payload_prepare(struct error_payload *e, const char *name,
> + uint32_t value_size, uint32_t extra)
> +{
> + struct error_field *f;
> + uint32_t name_size = strlen(name) + 1;
> + uint32_t data_offset = name_size + sizeof(*f);
> + uint32_t total = data_offset + value_size + extra;
> +
> + struct error_field **fields = e->fields;
> + int count = e->count;
> + int i;
> + for (i = 0; i < count; ++i) {
> + f = fields[i];
> + if (strcmp(name, f->name) != 0)
> + continue;
> + f = xrealloc(f, total);
> + goto set;
> + }
> + e->count = ++count;
> + fields = xrealloc(fields, sizeof(fields[0]) * count);
> + e->fields = fields;
> + f = xmalloc(total);
> + memcpy(f->name, name, name_size);
> +set:
> + f->data = (char *)f + data_offset;
> + f->size = value_size;
> + fields[i] = f;
> + return f;
> +}
> diff --git a/src/lib/core/error_payload.h b/src/lib/core/error_payload.h
> new file mode 100644
> index 000000000..b6bf9ceee
> --- /dev/null
> +++ b/src/lib/core/error_payload.h
> @@ -0,0 +1,166 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +#pragma once
> +
> +#include "trivia/util.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +/** Single field of error payload. */
> +struct error_field {
> + /** Data size. */
> + uint32_t size;
> + /** MessagePack field value. */
> + char *data;
> + /** Zero terminated field name. */
> + char name[0];
> +};
> +
> +/** An array of key-value pairs. */
> +struct error_payload {
> + /** Number of fields. */
> + int count;
> + /**
> + * Array of field pointers. Not very optimized, but the errors are
> + * supposed to be rare. Also not storing them by values simplifies
> + * addition of new fields and their removal.
> + */
> + struct error_field **fields;
> +};
> +
> +/** Create error payload. */
> +void
> +error_payload_create(struct error_payload *p);
> +
> +/** Destroy error payload. */
> +void
> +error_payload_destroy(struct error_payload *p);
> +
> +/**
> + * Get value of a payload field as a string. If it is not string or is not
> + * found - return NULL.
> + */
> +const char *
> +error_payload_get_str(const struct error_payload *e, const char *name);
> +
> +/**
> + * Set value of a payload field to a string. If the field existed before, it is
> + * overwritten.
> + */
> +void
> +error_payload_set_str(struct error_payload *e, const char *name,
> + const char *value);
> +
> +/**
> + * Get value of a payload field as a uint. If it is not uint or is not found -
> + * return false and the out parameter is set to 0.
> + */
> +bool
> +error_payload_get_uint(const struct error_payload *e, const char *name,
> + uint64_t *value);
> +
> +/**
> + * Set value of a payload field to a uint. If the field existed before, it is
> + * overwritten.
> + */
> +void
> +error_payload_set_uint(struct error_payload *e, const char *name,
> + uint64_t value);
> +
> +/**
> + * Get value of a payload field as an int. If it is not int, or is not found, or
> + * does not fit int64_t - return false and the out parameter is set to 0.
> + */
> +bool
> +error_payload_get_int(const struct error_payload *e, const char *name,
> + int64_t *value);
> +
> +/**
> + * Set value of a payload field to an int. If the field existed before, it is
> + * overwritten.
> + */
> +void
> +error_payload_set_int(struct error_payload *e, const char *name, int64_t value);
> +
> +/**
> + * Get value of a payload field as a double. If it is not double or is not
> + * found - return false and the out parameter is set to 0.
> + */
> +bool
> +error_payload_get_double(const struct error_payload *e, const char *name,
> + double *value);
> +
> +/**
> + * Set value of a payload field to a double. If the field existed before, it is
> + * overwritten.
> + */
> +void
> +error_payload_set_double(struct error_payload *e, const char *name,
> + double value);
> +
> +/**
> + * Get value of a payload field as a bool. If it is not bool or is not found -
> + * return false and the out parameter is set to false.
> + */
> +bool
> +error_payload_get_bool(const struct error_payload *e, const char *name,
> + bool *value);
> +
> +/**
> + * Set value of a payload field to a bool. If the field existed before, it is
> + * overwritten.
> + */
> +void
> +error_payload_set_bool(struct error_payload *e, const char *name, bool value);
> +
> +/**
> + * Get MessagePack value of a payload field. If it is not found - return NULL
> + * and the out parameter is set to 0.
> + */
> +const char *
> +error_payload_get_mp(const struct error_payload *e, const char *name,
> + uint32_t *size);
> +
> +/**
> + * Set value of a payload field to a MessagePack buffer. If the field existed
> + * before, it is overwritten.
> + */
> +void
> +error_payload_set_mp(struct error_payload *e, const char *name,
> + const char *src, uint32_t size);
> +
> +/** Remove the given field from the payload. */
> +void
> +error_payload_unset(struct error_payload *e, const char *name);
> +
> +/**
> + * Move all fields of one payload into another. Old fields of the destination
> + * are all deleted. The source stays valid but empty.
> + */
> +void
> +error_payload_move(struct error_payload *dst, struct error_payload *src);
> +
> +/** Find a payload field by name. */
> +const struct error_field *
> +error_payload_find(const struct error_payload *e, const char *name);
> +
> +/**
> + * Prepare a payload field to get a new value. If the field didn't exist, it is
> + * added. If it existed then it is reallocated if was necessary and should be
> + * overwritten. Name is already filled in the field. Only need to fill the
> + * value.
> + * Extra parameter allows to allocate extra memory for arbitrary usage after the
> + * error field and its value.
> + */
> +struct error_field *
> +error_payload_prepare(struct error_payload *e, const char *name,
> + uint32_t value_size, uint32_t extra);
> +
> +#if defined(__cplusplus)
> +} /* extern "C" */
> +#endif /* defined(__cplusplus) */
> diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c
> index b2341ae36..a60716eb5 100644
> --- a/src/lib/uuid/mp_uuid.c
> +++ b/src/lib/uuid/mp_uuid.c
> @@ -32,6 +32,7 @@
> #include "mp_uuid.h"
> #include "msgpuck.h"
> #include "mp_extension_types.h"
> +#include "error_payload.h"
>
> inline uint32_t
> mp_sizeof_uuid(void)
> @@ -113,3 +114,28 @@ mp_fprint_uuid(FILE *file, const char **data, uint32_t len)
> return -1;
> return fprintf(file, "%s", tt_uuid_str(&uuid));
> }
> +
> +bool
> +error_payload_get_uuid(const struct error_payload *e, const char *name,
> + struct tt_uuid *uuid)
> +{
> + const struct error_field *f = error_payload_find(e, name);
> + if (f == NULL)
> + goto not_found;
> + const char *data = f->data;
> + if (mp_decode_uuid(&data, uuid) == NULL)
> + goto not_found;
> + return true;
> +
> +not_found:
> + *uuid = uuid_nil;
> + return false;
> +}
> +
6. I propose to get rid of the label here and set uuid to nil
unconditionally.
Would it be too expensive to do 2 struct assignments instead of one?
(uuid = uuid_nil; uuid = decoded_value).
> +void
> +error_payload_set_uuid(struct error_payload *e, const char *name,
> + const struct tt_uuid *uuid)
> +{
> + char *data = error_payload_prepare(e, name, mp_sizeof_uuid(), 0)->data;
> + mp_encode_uuid(data, uuid);
> +}
> diff --git a/src/lib/uuid/tt_uuid.h b/src/lib/uuid/tt_uuid.h
> index 70c3b98b1..866764e77 100644
> --- a/src/lib/uuid/tt_uuid.h
> +++ b/src/lib/uuid/tt_uuid.h
> @@ -41,6 +41,8 @@
> extern "C" {
> #endif
>
> +struct error_payload;
> +
> enum { UUID_LEN = 16, UUID_STR_LEN = 36 };
>
> /**
> @@ -182,6 +184,28 @@ tt_uuid_str(const struct tt_uuid *uu);
> int
> tt_uuid_from_strl(const char *in, size_t len, struct tt_uuid *uu);
>
> +/**
> + * Error payload couldn't be implemented in libcore because requires UUID type
> + * and its methods. That would be a cyclic dep. Thus in places where need to use
> + * UUID fields in error payload also need to include libuuid.
> + */
> +
> +/**
> + * Get value of a payload field as a UUID. If it is not UUID or is not found -
> + * return false and the out parameter is set to UUID with zeros.
> + */
> +bool
> +error_payload_get_uuid(const struct error_payload *e, const char *name,
> + struct tt_uuid *uuid);
> +
> +/**
> + * Set value of a payload field to a UUID. If the field existed before, it is
> + * overwritten.
> + */
> +void
> +error_payload_set_uuid(struct error_payload *e, const char *name,
> + const struct tt_uuid *uuid);
> +
> #if defined(__cplusplus)
> } /* extern "C" */
> #endif
> diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
> index ae8b5b9ac..0bb71ccce 100644
> --- a/test/unit/CMakeLists.txt
> +++ b/test/unit/CMakeLists.txt
> @@ -60,6 +60,8 @@ add_executable(xmalloc.test xmalloc.c core_test_utils.c)
> target_link_libraries(xmalloc.test unit)
> add_executable(datetime.test datetime.c)
> target_link_libraries(datetime.test tzcode core cdt unit)
> +add_executable(error.test error.c core_test_utils.c)
> +target_link_libraries(error.test unit core uuid)
>
> add_executable(bps_tree.test bps_tree.cc)
> target_link_libraries(bps_tree.test small misc)
> diff --git a/test/unit/error.c b/test/unit/error.c
> new file mode 100644
> index 000000000..ef17c56ec
> --- /dev/null
> +++ b/test/unit/error.c
> @@ -0,0 +1,461 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +#include "error_payload.h"
> +#include "msgpuck.h"
> +#include "random.h"
> +#include "unit.h"
> +#include "uuid/mp_uuid.h"
> +#include "uuid/tt_uuid.h"
> +
> +#include <float.h>
> +
> +static void
> +test_payload_field_str(void)
> +{
> + header();
> + plan(15);
> +
> + struct error_payload p;
> + error_payload_create(&p);
> + is(p.count, 0, "no fields in the beginning");
> + is(error_payload_get_str(&p, "key"), NULL, "get_str() empty");
> +
> + error_payload_set_str(&p, "key1", "value1");
> + is(p.count, 1, "++count");
> + is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0,
> + "get_str() finds");
> +
> + error_payload_set_str(&p, "key2", "value2");
> + is(p.count, 2, "++count");
> + is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0,
> + "get_str() finds old");
> + is(strcmp(error_payload_get_str(&p, "key2"), "value2"), 0,
> + "get_str() finds new");
> + is(error_payload_find(&p, "key1")->size,
> + mp_sizeof_str(strlen("value1")),
> + "size does not include terminating zero");
> +
> + error_payload_set_str(&p, "key1", "new_value1");
> + is(p.count, 2, "field count is same");
> + is(strcmp(error_payload_get_str(&p, "key1"), "new_value1"), 0,
> + "get_str() finds new value");
> + is(strcmp(error_payload_get_str(&p, "key2"), "value2"), 0,
> + "get_str() finds other key old value");
> +
> + error_payload_unset(&p, "key2");
> + is(p.count, 1, "--count");
> + is(strcmp(error_payload_get_str(&p, "key1"), "new_value1"), 0,
> + "get_str() finds new value");
> + is(error_payload_get_str(&p, "key2"), NULL,
> + "get_str() can't find deleted value");
> +
> + error_payload_set_uint(&p, "key2", 1);
> + is(error_payload_get_str(&p, "key2"), NULL, "wrong type");
> +
> + error_payload_destroy(&p);
> +
> + check_plan();
> + footer();
> +}
> +
> +static void
> +test_payload_field_uint(void)
> +{
> + header();
> + plan(17);
> +
> + struct error_payload p;
> + error_payload_create(&p);
> + uint64_t val = 1;
> + ok(!error_payload_get_uint(&p, "key", &val) && val == 0,
> + "get_uint() empty");
> +
> + error_payload_set_uint(&p, "key1", 1);
> + is(p.count, 1, "++count");
> + ok(error_payload_get_uint(&p, "key1", &val), "get_uint() finds");
> + is(val, 1, "value match");
> +
> + val = 100;
> + ok(!error_payload_get_uint(&p, "key2", &val), "get_uint() fails");
> + is(val, 0, "value is default");
> +
> + is(error_payload_find(&p, "key1")->size, mp_sizeof_uint(1),
> + "small number size");
> +
> + error_payload_set_uint(&p, "key2", UINT32_MAX);
> + ok(error_payload_get_uint(&p, "key2", &val), "get_uint() 32 bit");
> + is(val, UINT32_MAX, "value match");
> + is(error_payload_find(&p, "key2")->size, mp_sizeof_uint(UINT32_MAX),
> + "middle number size");
> + is(p.count, 2, "field count is same");
> +
> + error_payload_set_uint(&p, "key1", UINT64_MAX);
> + ok(error_payload_get_uint(&p, "key1", &val) && val == UINT64_MAX,
> + "value 1");
> + ok(error_payload_get_uint(&p, "key2", &val) && val == UINT32_MAX,
> + "value 2");
> +
> + error_payload_unset(&p, "key2");
> + is(p.count, 1, "--count");
> + ok(error_payload_get_uint(&p, "key1", &val) && val == UINT64_MAX,
> + "remained value");
> + ok(!error_payload_get_uint(&p, "key2", &val) && val == 0,
> + "deleted value");
> +
> + error_payload_set_str(&p, "key2", "1");
> + ok(!error_payload_get_uint(&p, "key2", &val) && val == 0, "wrong type");
> +
> + error_payload_destroy(&p);
> +
> + check_plan();
> + footer();
> +}
> +
> +static void
> +test_payload_field_int(void)
> +{
> + header();
> + plan(20);
> +
> + struct error_payload p;
> + error_payload_create(&p);
> + int64_t val = 1;
> + ok(!error_payload_get_int(&p, "key", &val) && val == 0,
> + "get_int() empty");
> +
> + error_payload_set_int(&p, "key1", 1);
> + is(p.count, 1, "++count");
> + ok(error_payload_get_int(&p, "key1", &val), "get_int() finds");
> + is(val, 1, "value match");
> +
> + val = 100;
> + ok(!error_payload_get_int(&p, "key2", &val), "get_int() fails");
> + is(val, 0, "value is default");
> +
> + is(error_payload_find(&p, "key1")->size, mp_sizeof_uint(1),
> + "small number size");
> +
> + error_payload_set_int(&p, "key2", UINT32_MAX);
> + ok(error_payload_get_int(&p, "key2", &val), "get_int() 32 bit");
> + is(val, UINT32_MAX, "value match");
> + is(error_payload_find(&p, "key2")->size, mp_sizeof_uint(UINT32_MAX),
> + "middle number size");
> + is(p.count, 2, "field count is same");
> +
> + error_payload_set_int(&p, "key1", INT64_MAX);
> + ok(error_payload_get_int(&p, "key1", &val) && val == INT64_MAX,
> + "value 1");
> + ok(error_payload_get_int(&p, "key2", &val) && val == UINT32_MAX,
> + "value 2");
> +
> + error_payload_unset(&p, "key2");
> + is(p.count, 1, "--count");
> + ok(error_payload_get_int(&p, "key1", &val) && val == INT64_MAX,
> + "remained value");
> + ok(!error_payload_get_int(&p, "key2", &val) && val == 0,
> + "deleted value");
> +
> + error_payload_set_str(&p, "key2", "1");
> + ok(!error_payload_get_int(&p, "key2", &val) && val == 0, "wrong type");
> +
> + error_payload_set_uint(&p, "key2", (uint64_t)INT64_MAX + 1);
> + ok(!error_payload_get_int(&p, "key2", &val) && val == 0, "overflow");
> +
> + error_payload_set_uint(&p, "key2", 100);
> + ok(error_payload_get_int(&p, "key2", &val) && val == 100, "conversion");
> +
> + error_payload_set_int(&p, "key2", INT64_MIN);
> + ok(error_payload_get_int(&p, "key2", &val) && val == INT64_MIN,
> + "min value");
> +
> + error_payload_destroy(&p);
> +
> + check_plan();
> + footer();
> +}
> +
> +static void
> +test_payload_field_double(void)
> +{
> + header();
> + plan(13);
> +
> + struct error_payload p;
> + error_payload_create(&p);
> + double val = 1;
> + ok(!error_payload_get_double(&p, "key", &val) && val == 0,
> + "get_double() empty");
> +
> + error_payload_set_double(&p, "key1", 1.5);
> + is(p.count, 1, "++count");
> + ok(error_payload_get_double(&p, "key1", &val), "get_double() finds");
> + is(val, 1.5, "value match");
> +
> + val = 1;
> + ok(!error_payload_get_double(&p, "key2", &val), "get_double() fails");
> + is(val, 0, "value is default");
> +
> + is(error_payload_find(&p, "key1")->size, mp_sizeof_double(1.5), "size");
> +
> + error_payload_set_double(&p, "key2", DBL_MAX);
> + ok(error_payload_get_double(&p, "key1", &val) && val == 1.5, "value 1");
> + ok(error_payload_get_double(&p, "key2", &val) && val == DBL_MAX,
> + "value 2");
> +
> + error_payload_unset(&p, "key2");
> + is(p.count, 1, "--count");
> + ok(error_payload_get_double(&p, "key1", &val) && val == 1.5,
> + "remained value");
> + ok(!error_payload_get_double(&p, "key2", &val) && val == 0,
> + "deleted value");
> +
> + error_payload_set_str(&p, "key2", "1");
> + ok(!error_payload_get_double(&p, "key2", &val) && val == 0,
> + "wrong type");
> +
> + error_payload_destroy(&p);
> +
> + check_plan();
> + footer();
> +}
> +
> +static void
> +test_payload_field_bool(void)
> +{
> + header();
> + plan(13);
> +
> + struct error_payload p;
> + error_payload_create(&p);
> + bool val = true;
> + ok(!error_payload_get_bool(&p, "key", &val) && !val,
> + "get_bool() empty");
> +
> + error_payload_set_bool(&p, "key1", true);
> + is(p.count, 1, "++count");
> + ok(error_payload_get_bool(&p, "key1", &val), "get_bool() finds");
> + ok(val, "value match");
> +
> + val = true;
> + ok(!error_payload_get_bool(&p, "key2", &val), "get_bool() fails");
> + ok(!val, "value is default");
> +
> + error_payload_set_bool(&p, "key2", false);
> + ok(error_payload_get_bool(&p, "key2", &val), "get_bool() finds");
> + ok(!val, "value match");
> +
> + is(error_payload_find(&p, "key1")->size, mp_sizeof_bool(true), "size");
> +
> + error_payload_unset(&p, "key2");
> + is(p.count, 1, "--count");
> + ok(error_payload_get_bool(&p, "key1", &val) && val, "remained value");
> + ok(!error_payload_get_bool(&p, "key2", &val) && !val, "deleted value");
> +
> + error_payload_set_str(&p, "key2", "true");
> + ok(!error_payload_get_bool(&p, "key2", &val) && !val, "wrong type");
> +
> + error_payload_destroy(&p);
> +
> + check_plan();
> + footer();
> +}
> +
> +static void
> +test_payload_field_uuid(void)
> +{
> + header();
> + plan(17);
> +
> + struct error_payload p;
> + error_payload_create(&p);
> + struct tt_uuid val1;
> + tt_uuid_create(&val1);
> + ok(!error_payload_get_uuid(&p, "key", &val1), "get_uuid() empty");
> + ok(tt_uuid_is_nil(&val1), "default value");
> +
> + tt_uuid_create(&val1);
> + error_payload_set_uuid(&p, "key1", &val1);
> + is(p.count, 1, "++count");
> + struct tt_uuid val2;
> + ok(error_payload_get_uuid(&p, "key1", &val2), "get_uuid() finds");
> + ok(tt_uuid_is_equal(&val1, &val2), "value match");
> +
> + ok(!error_payload_get_uuid(&p, "key2", &val2), "get_uuid() fails");
> + ok(tt_uuid_is_nil(&val2), "value is default");
> +
> + tt_uuid_create(&val2);
> + error_payload_set_uuid(&p, "key2", &val2);
> + struct tt_uuid val3;
> + ok(error_payload_get_uuid(&p, "key2", &val3), "get_uuid() finds");
> + ok(tt_uuid_is_equal(&val3, &val2), "value match");
> +
> + is(error_payload_find(&p, "key1")->size, mp_sizeof_uuid(), "size");
> +
> + error_payload_unset(&p, "key2");
> + is(p.count, 1, "--count");
> + ok(error_payload_get_uuid(&p, "key1", &val3), "remained value");
> + ok(tt_uuid_is_equal(&val1, &val3), "value match");
> + ok(!error_payload_get_uuid(&p, "key2", &val3), "deleted value");
> + ok(tt_uuid_is_nil(&val3), "value match");
> +
> + error_payload_set_str(&p, "key2", "1");
> + ok(!error_payload_get_uuid(&p, "key2", &val3), "wrong type");
> + ok(tt_uuid_is_nil(&val3), "value match");
> +
> + error_payload_destroy(&p);
> +
> + check_plan();
> + footer();
> +}
> +
> +static void
> +test_payload_field_mp(void)
> +{
> + header();
> + plan(6);
> + char buf[1024];
> + char *data;
> + const char *cdata;
> + uint32_t size;
> +
> + struct error_payload p;
> + error_payload_create(&p);
> +
> + data = mp_encode_str(buf, "value1", 6);
> + error_payload_set_mp(&p, "key1", buf, data - buf);
> + is(strcmp(error_payload_get_str(&p, "key1"), "value1"), 0, "mp str");
> +
> + cdata = error_payload_get_mp(&p, "key1", &size);
> + is(memcmp(cdata, buf, size), 0, "mp str cmp");
> +
> + data = mp_encode_uint(buf, 100);
> + error_payload_set_mp(&p, "key2", buf, data - buf);
> + uint64_t val;
> + ok(error_payload_get_uint(&p, "key2", &val) && val == 100, "mp uint");
> +
> + cdata = error_payload_get_mp(&p, "key2", &size);
> + is(memcmp(cdata, buf, size), 0, "mp uint cmp");
> +
> + data = mp_encode_array(buf, 1);
> + data = mp_encode_uint(data, 2);
> + error_payload_set_mp(&p, "key3", buf, data - buf);
> +
> + cdata = error_payload_get_mp(&p, "key3", &size);
> + is(memcmp(cdata, buf, size), 0, "mp array");
> +
> + ok(!error_payload_get_uint(&p, "key3", &val) && val == 0,
> + "mp uint from array");
> +
> + error_payload_destroy(&p);
> +
> + check_plan();
> + footer();
> +}
> +
> +static void
> +test_payload_unset(void)
> +{
> + header();
> + plan(13);
> +
> + struct error_payload p;
> + error_payload_create(&p);
> +
> + error_payload_set_uint(&p, "key1", 1);
> + error_payload_set_uint(&p, "key2", 2);
> + error_payload_set_uint(&p, "key3", 3);
> + error_payload_set_uint(&p, "key4", 4);
> + error_payload_set_uint(&p, "key5", 5);
> +
> + error_payload_unset(&p, "key5");
> + is(p.count, 4, "unset last, count");
> + is(error_payload_find(&p, "key5"), NULL, "unset last, key");
> +
> + error_payload_unset(&p, "key1");
> + is(p.count, 3, "unset first, count");
> + is(error_payload_find(&p, "key1"), NULL, "unset first, key");
> +
> + uint64_t val;
> + ok(error_payload_get_uint(&p, "key2", &val) && val == 2, "check key2");
> + ok(error_payload_get_uint(&p, "key3", &val) && val == 3, "check key3");
> + ok(error_payload_get_uint(&p, "key4", &val) && val == 4, "check key4");
> +
> + is(strcmp(p.fields[0]->name, "key4"), 0, "deletion is cyclic");
> +
> + error_payload_unset(&p, "key2");
> + is(p.count, 2, "unset middle, count");
> + is(error_payload_find(&p, "key2"), NULL, "unset middle, key");
> + ok(error_payload_get_uint(&p, "key3", &val) && val == 3, "check key3");
> + ok(error_payload_get_uint(&p, "key4", &val) && val == 4, "check key4");
> +
> + error_payload_unset(&p, "key3");
> + error_payload_unset(&p, "key4");
> +
> + is(p.count, 0, "unset all");
> +
> + error_payload_destroy(&p);
> +
> + check_plan();
> + footer();
> +}
> +
> +static void
> +test_payload_move(void)
> +{
> + header();
> + plan(7);
> +
> + struct error_payload p1, p2;
> + error_payload_create(&p1);
> + error_payload_create(&p2);
> +
> + error_payload_move(&p1, &p2);
> + ok(p1.count == 0 && p1.fields == NULL, "empty");
> +
> + error_payload_set_str(&p1, "key", "value");
> + error_payload_move(&p1, &p2);
> + ok(p1.count == 0 && p1.fields == NULL, "emptied on move");
> +
> + error_payload_set_str(&p1, "key", "value");
> + error_payload_set_str(&p2, "key1", "value1");
> + error_payload_set_str(&p2, "key2", "value2");
> + error_payload_move(&p1, &p2);
> + is(p1.count, 2, "got 2 fields");
> + isnt(p1.fields, NULL, "got 2 fields");
> + is(strcmp(error_payload_get_str(&p1, "key1"), "value1"), 0, "key1");
> + is(strcmp(error_payload_get_str(&p1, "key2"), "value2"), 0, "key2");
> + is(error_payload_get_str(&p1, "key"), NULL, "key");
> +
> + error_payload_destroy(&p2);
> + error_payload_destroy(&p1);
> +
> + check_plan();
> + footer();
> +}
> +
> +int
> +main(void)
> +{
> + header();
> + plan(9);
> +
> + random_init();
> +
> + test_payload_field_str();
> + test_payload_field_uint();
> + test_payload_field_int();
> + test_payload_field_double();
> + test_payload_field_bool();
> + test_payload_field_uuid();
> + test_payload_field_mp();
> + test_payload_unset();
> + test_payload_move();
> +
> + random_free();
> +
> + footer();
> + return check_plan();
> +}
> diff --git a/test/unit/error.result b/test/unit/error.result
> new file mode 100644
> index 000000000..f0c4266e6
> --- /dev/null
> +++ b/test/unit/error.result
> @@ -0,0 +1,160 @@
> + *** main ***
> +1..9
> + *** test_payload_field_str ***
> + 1..15
> + ok 1 - no fields in the beginning
> + ok 2 - get_str() empty
> + ok 3 - ++count
> + ok 4 - get_str() finds
> + ok 5 - ++count
> + ok 6 - get_str() finds old
> + ok 7 - get_str() finds new
> + ok 8 - size does not include terminating zero
> + ok 9 - field count is same
> + ok 10 - get_str() finds new value
> + ok 11 - get_str() finds other key old value
> + ok 12 - --count
> + ok 13 - get_str() finds new value
> + ok 14 - get_str() can't find deleted value
> + ok 15 - wrong type
> +ok 1 - subtests
> + *** test_payload_field_str: done ***
> + *** test_payload_field_uint ***
> + 1..17
> + ok 1 - get_uint() empty
> + ok 2 - ++count
> + ok 3 - get_uint() finds
> + ok 4 - value match
> + ok 5 - get_uint() fails
> + ok 6 - value is default
> + ok 7 - small number size
> + ok 8 - get_uint() 32 bit
> + ok 9 - value match
> + ok 10 - middle number size
> + ok 11 - field count is same
> + ok 12 - value 1
> + ok 13 - value 2
> + ok 14 - --count
> + ok 15 - remained value
> + ok 16 - deleted value
> + ok 17 - wrong type
> +ok 2 - subtests
> + *** test_payload_field_uint: done ***
> + *** test_payload_field_int ***
> + 1..20
> + ok 1 - get_int() empty
> + ok 2 - ++count
> + ok 3 - get_int() finds
> + ok 4 - value match
> + ok 5 - get_int() fails
> + ok 6 - value is default
> + ok 7 - small number size
> + ok 8 - get_int() 32 bit
> + ok 9 - value match
> + ok 10 - middle number size
> + ok 11 - field count is same
> + ok 12 - value 1
> + ok 13 - value 2
> + ok 14 - --count
> + ok 15 - remained value
> + ok 16 - deleted value
> + ok 17 - wrong type
> + ok 18 - overflow
> + ok 19 - conversion
> + ok 20 - min value
> +ok 3 - subtests
> + *** test_payload_field_int: done ***
> + *** test_payload_field_double ***
> + 1..13
> + ok 1 - get_double() empty
> + ok 2 - ++count
> + ok 3 - get_double() finds
> + ok 4 - value match
> + ok 5 - get_double() fails
> + ok 6 - value is default
> + ok 7 - size
> + ok 8 - value 1
> + ok 9 - value 2
> + ok 10 - --count
> + ok 11 - remained value
> + ok 12 - deleted value
> + ok 13 - wrong type
> +ok 4 - subtests
> + *** test_payload_field_double: done ***
> + *** test_payload_field_bool ***
> + 1..13
> + ok 1 - get_bool() empty
> + ok 2 - ++count
> + ok 3 - get_bool() finds
> + ok 4 - value match
> + ok 5 - get_bool() fails
> + ok 6 - value is default
> + ok 7 - get_bool() finds
> + ok 8 - value match
> + ok 9 - size
> + ok 10 - --count
> + ok 11 - remained value
> + ok 12 - deleted value
> + ok 13 - wrong type
> +ok 5 - subtests
> + *** test_payload_field_bool: done ***
> + *** test_payload_field_uuid ***
> + 1..17
> + ok 1 - get_uuid() empty
> + ok 2 - default value
> + ok 3 - ++count
> + ok 4 - get_uuid() finds
> + ok 5 - value match
> + ok 6 - get_uuid() fails
> + ok 7 - value is default
> + ok 8 - get_uuid() finds
> + ok 9 - value match
> + ok 10 - size
> + ok 11 - --count
> + ok 12 - remained value
> + ok 13 - value match
> + ok 14 - deleted value
> + ok 15 - value match
> + ok 16 - wrong type
> + ok 17 - value match
> +ok 6 - subtests
> + *** test_payload_field_uuid: done ***
> + *** test_payload_field_mp ***
> + 1..6
> + ok 1 - mp str
> + ok 2 - mp str cmp
> + ok 3 - mp uint
> + ok 4 - mp uint cmp
> + ok 5 - mp array
> + ok 6 - mp uint from array
> +ok 7 - subtests
> + *** test_payload_field_mp: done ***
> + *** test_payload_unset ***
> + 1..13
> + ok 1 - unset last, count
> + ok 2 - unset last, key
> + ok 3 - unset first, count
> + ok 4 - unset first, key
> + ok 5 - check key2
> + ok 6 - check key3
> + ok 7 - check key4
> + ok 8 - deletion is cyclic
> + ok 9 - unset middle, count
> + ok 10 - unset middle, key
> + ok 11 - check key3
> + ok 12 - check key4
> + ok 13 - unset all
> +ok 8 - subtests
> + *** test_payload_unset: done ***
> + *** test_payload_move ***
> + 1..7
> + ok 1 - empty
> + ok 2 - emptied on move
> + ok 3 - got 2 fields
> + ok 4 - got 2 fields
> + ok 5 - key1
> + ok 6 - key2
> + ok 7 - key
> +ok 9 - subtests
> + *** test_payload_move: done ***
> + *** main: done ***
--
Serge Petrenko
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 2/9] error: introduce error_payload
2021-11-08 15:14 ` Serge Petrenko via Tarantool-patches
@ 2021-11-11 23:50 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-12 6:29 ` Serge Petrenko via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:50 UTC (permalink / raw)
To: Serge Petrenko, tarantool-patches, vdavydov
Hi! Thanks for the review!
Could you please cut the diff not participating in the review when send
next comments? It would greatly help to locate them.
>> diff --git a/src/lib/core/error_payload.c b/src/lib/core/error_payload.c
>> new file mode 100644
>> index 000000000..98ef08ada
>> --- /dev/null
>> +++ b/src/lib/core/error_payload.c
>> @@ -0,0 +1,282 @@
<...>
>> +const char *
>> +error_payload_get_str(const struct error_payload *e, const char *name)
>> +{
>> + const struct error_field *f = error_payload_find(e, name);
>> + if (f == NULL)
>> + return NULL;
>> + const char *data = f->data;
>> + if (mp_typeof(*data) != MP_STR)
>> + return NULL;
>> + uint32_t len;
>> + /*
>> + * 0 is explicitly written after encoded string to be able to return
>> + * them without copying.
>> + */
>> + data = mp_decode_str(&data, &len);
>> + assert(data[len] == 0);
>
> 1. Nit: `data[len] == '\0'`. Here and everywhere else. This is up to you,
> I'm just more used to '\0'.
I like plain 0 more because it is a bit shorter and is highlighted in
an editor as a number, which makes it stand out from regular characters.
>> + return data;
>> +}
>> +
>> +void
>> +error_payload_set_str(struct error_payload *e, const char *name,
>> + const char *value)
>> +{
>> + uint32_t len = strlen(value);
>> + char *data = error_payload_prepare(
>> + e, name, mp_sizeof_str(len), 1)->data;
>> + /*
>> + * 1 extra byte in the end is reserved to append 0 after the encoded
>> + * string. To be able to return it from get() without copying.
>> + */
>
> 2. This comment is almost the same as the one in payload_get_str().
> I propose to leave only one of them. Preferably the one in set_str().
Ok, dropped from get_str():
====================
@@ -34,10 +34,6 @@ error_payload_get_str(const struct error_payload *e, const char *name)
if (mp_typeof(*data) != MP_STR)
return NULL;
uint32_t len;
- /*
- * 0 is explicitly written after encoded string to be able to return
- * them without copying.
- */
====================
> diff --git a/src/lib/core/error_payload.c b/src/lib/core/error_payload.c
> index 98ef08ada..cfa30919e 100644
> --- a/src/lib/core/error_payload.c
> +++ b/src/lib/core/error_payload.c
> @@ -62,17 +62,14 @@ error_payload_get_uint(const struct error_payload *e, const char *name,
> uint64_t *value)
> {
> const struct error_field *f = error_payload_find(e, name);
> + *value = 0;
> if (f == NULL)
> - goto not_found;
> + return false;
> const char *data = f->data;
> if (mp_typeof(*data) != MP_UINT)
> - goto not_found;
> + return false;
> *value = mp_decode_uint(&data);
> return true;
> -
> -not_found:
> - *value = 0;
> - return false;
> }
>
> void
I would apply this diff if not tt_uuid field type - I would
need to make it the same to be consistent. While extra assignment
of an 8-byte number probably can't be noticed on the success path,
nil uuid copying could be something.
Let me know if you still want that applied after this reasoning
and I will do.
>> +bool
>> +error_payload_get_double(const struct error_payload *e, const char *name,
>> + double *value)
>> +{
>> + const struct error_field *f = error_payload_find(e, name);
>> + if (f == NULL)
>> + goto not_found;
>> + const char *data = f->data;
>> + if (mp_typeof(*data) == MP_DOUBLE) {
>> + *value = mp_decode_double(&data);
>> + return true;
>> + } else if (mp_typeof(*data) == MP_FLOAT) {
>> + *value = mp_decode_float(&data);
>> + return true;
>> + }
>
> 4. Why do you check for both MP_DOUBLE and MP_FLOAT here?
> You only encode MP_DOUBLE.
Because the fields could arrive from network from a connector,
for instance. Connectors could encode floats. So I wanted this
code be ready to that. Btw, I realized I didn't have tests for
that. Added now:
====================
@@ -181,7 +181,7 @@ static void
test_payload_field_double(void)
{
header();
- plan(13);
+ plan(14);
struct error_payload p;
error_payload_create(&p);
@@ -216,6 +216,12 @@ test_payload_field_double(void)
ok(!error_payload_get_double(&p, "key2", &val) && val == 0,
"wrong type");
+ char buffer[16];
+ char *data = mp_encode_float(buffer, 0.5);
+ error_payload_set_mp(&p, "key2", buffer, data - buffer);
+ ok(error_payload_get_double(&p, "key2", &val) && val == 0.5,
+ "float 0.5");
+
error_payload_destroy(&p);
====================
> I think we might have set_float() and get_float() one day, but looks
> like they're not needed now.
I added them in a first version, but then faced an issue what to
do in get_float() when you see MP_DOUBLE? It appeared to be quite
complicated to determine when a double could be truncated to float
safely, so I decided not to do float get/set support at all. Only
handle it in scope of getting a double.
> get_float() would only accept MP_FLOAT then, and get_double() should
> only accept MP_DOUBLE.
The reasoning here is more or less similar to why I didn't add
separate get/set_uint32 and only added uint64 called 'uint' - I
didn't want the caller to care what was the subtype.
I also thought about dropping both double and float support but
then realized we use double for time and it is likely we will want
to add timestamps/timeouts/durations to some errors eventually.
<...>
>> +void
>> +error_payload_set_mp(struct error_payload *e, const char *name,
>> + const char *src, uint32_t size)
>> +{
>> + char *data;
>> + if (mp_typeof(*src) == MP_STR) {
>> + data = error_payload_prepare(e, name, size, 1)->data;
>> + /*
>> + * Add the terminating zero after the buffer so as get_str()
>> + * would work without copying.
>> + */
>
> 5. Maybe shorten this comment to "\sa error_payload_set_str()" ?
Done.
====================
data = error_payload_prepare(e, name, size, 1)->data;
- /*
- * Add the terminating zero after the buffer so as get_str()
- * would work without copying.
- */
+ /* @sa error_payload_set_str(). */
data[size] = 0;
====================
>> diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c
>> index b2341ae36..a60716eb5 100644
>> --- a/src/lib/uuid/mp_uuid.c
>> +++ b/src/lib/uuid/mp_uuid.c
>> @@ -113,3 +114,28 @@ mp_fprint_uuid(FILE *file, const char **data, uint32_t len)
>> return -1;
>> return fprintf(file, "%s", tt_uuid_str(&uuid));
>> }
>> +
>> +bool
>> +error_payload_get_uuid(const struct error_payload *e, const char *name,
>> + struct tt_uuid *uuid)
>> +{
>> + const struct error_field *f = error_payload_find(e, name);
>> + if (f == NULL)
>> + goto not_found;
>> + const char *data = f->data;
>> + if (mp_decode_uuid(&data, uuid) == NULL)
>> + goto not_found;
>> + return true;
>> +
>> +not_found:
>> + *uuid = uuid_nil;
>> + return false;
>> +}
>> +
>
> 6. I propose to get rid of the label here and set uuid to nil unconditionally.
>
> Would it be too expensive to do 2 struct assignments instead of one?
> (uuid = uuid_nil; uuid = decoded_value).
Answered in the previous comment about gotos.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 2/9] error: introduce error_payload
2021-11-11 23:50 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-12 6:29 ` Serge Petrenko via Tarantool-patches
0 siblings, 0 replies; 24+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-12 6:29 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, vdavydov
12.11.2021 02:50, Vladislav Shpilevoy пишет:
> Hi! Thanks for the review!
>
> Could you please cut the diff not participating in the review when send
> next comments? It would greatly help to locate them.
Sure. Sorry for the inconvenience.
>
>>> diff --git a/src/lib/core/error_payload.c b/src/lib/core/error_payload.c
>>> new file mode 100644
>>> index 000000000..98ef08ada
>>> --- /dev/null
>>> +++ b/src/lib/core/error_payload.c
>>> @@ -0,0 +1,282 @@
> <...>
>
>>> +const char *
>>> +error_payload_get_str(const struct error_payload *e, const char *name)
>>> +{
>>> + const struct error_field *f = error_payload_find(e, name);
>>> + if (f == NULL)
>>> + return NULL;
>>> + const char *data = f->data;
>>> + if (mp_typeof(*data) != MP_STR)
>>> + return NULL;
>>> + uint32_t len;
>>> + /*
>>> + * 0 is explicitly written after encoded string to be able to return
>>> + * them without copying.
>>> + */
>>> + data = mp_decode_str(&data, &len);
>>> + assert(data[len] == 0);
>> 1. Nit: `data[len] == '\0'`. Here and everywhere else. This is up to you,
>> I'm just more used to '\0'.
> I like plain 0 more because it is a bit shorter and is highlighted in
> an editor as a number, which makes it stand out from regular characters.
Ok
>
>>> + return data;
>>> +}
>>> +
>>> +void
>>> +error_payload_set_str(struct error_payload *e, const char *name,
>>> + const char *value)
>>> +{
>>> + uint32_t len = strlen(value);
>>> + char *data = error_payload_prepare(
>>> + e, name, mp_sizeof_str(len), 1)->data;
>>> + /*
>>> + * 1 extra byte in the end is reserved to append 0 after the encoded
>>> + * string. To be able to return it from get() without copying.
>>> + */
>> 2. This comment is almost the same as the one in payload_get_str().
>> I propose to leave only one of them. Preferably the one in set_str().
> Ok, dropped from get_str():
>
> ====================
> @@ -34,10 +34,6 @@ error_payload_get_str(const struct error_payload *e, const char *name)
> if (mp_typeof(*data) != MP_STR)
> return NULL;
> uint32_t len;
> - /*
> - * 0 is explicitly written after encoded string to be able to return
> - * them without copying.
> - */
> ====================
>
>> diff --git a/src/lib/core/error_payload.c b/src/lib/core/error_payload.c
>> index 98ef08ada..cfa30919e 100644
>> --- a/src/lib/core/error_payload.c
>> +++ b/src/lib/core/error_payload.c
>> @@ -62,17 +62,14 @@ error_payload_get_uint(const struct error_payload *e, const char *name,
>> uint64_t *value)
>> {
>> const struct error_field *f = error_payload_find(e, name);
>> + *value = 0;
>> if (f == NULL)
>> - goto not_found;
>> + return false;
>> const char *data = f->data;
>> if (mp_typeof(*data) != MP_UINT)
>> - goto not_found;
>> + return false;
>> *value = mp_decode_uint(&data);
>> return true;
>> -
>> -not_found:
>> - *value = 0;
>> - return false;
>> }
>>
>> void
> I would apply this diff if not tt_uuid field type - I would
> need to make it the same to be consistent. While extra assignment
> of an 8-byte number probably can't be noticed on the success path,
> nil uuid copying could be something.
>
> Let me know if you still want that applied after this reasoning
> and I will do.
I see, never mind.
>
>>> +bool
>>> +error_payload_get_double(const struct error_payload *e, const char *name,
>>> + double *value)
>>> +{
>>> + const struct error_field *f = error_payload_find(e, name);
>>> + if (f == NULL)
>>> + goto not_found;
>>> + const char *data = f->data;
>>> + if (mp_typeof(*data) == MP_DOUBLE) {
>>> + *value = mp_decode_double(&data);
>>> + return true;
>>> + } else if (mp_typeof(*data) == MP_FLOAT) {
>>> + *value = mp_decode_float(&data);
>>> + return true;
>>> + }
>> 4. Why do you check for both MP_DOUBLE and MP_FLOAT here?
>> You only encode MP_DOUBLE.
> Because the fields could arrive from network from a connector,
> for instance. Connectors could encode floats. So I wanted this
> code be ready to that. Btw, I realized I didn't have tests for
> that. Added now:
>
> ====================
> @@ -181,7 +181,7 @@ static void
> test_payload_field_double(void)
> {
> header();
> - plan(13);
> + plan(14);
>
> struct error_payload p;
> error_payload_create(&p);
> @@ -216,6 +216,12 @@ test_payload_field_double(void)
> ok(!error_payload_get_double(&p, "key2", &val) && val == 0,
> "wrong type");
>
> + char buffer[16];
> + char *data = mp_encode_float(buffer, 0.5);
> + error_payload_set_mp(&p, "key2", buffer, data - buffer);
> + ok(error_payload_get_double(&p, "key2", &val) && val == 0.5,
> + "float 0.5");
> +
> error_payload_destroy(&p);
> ====================
>
>> I think we might have set_float() and get_float() one day, but looks
>> like they're not needed now.
> I added them in a first version, but then faced an issue what to
> do in get_float() when you see MP_DOUBLE? It appeared to be quite
> complicated to determine when a double could be truncated to float
> safely, so I decided not to do float get/set support at all. Only
> handle it in scope of getting a double.
>
>> get_float() would only accept MP_FLOAT then, and get_double() should
>> only accept MP_DOUBLE.
> The reasoning here is more or less similar to why I didn't add
> separate get/set_uint32 and only added uint64 called 'uint' - I
> didn't want the caller to care what was the subtype.
>
> I also thought about dropping both double and float support but
> then realized we use double for time and it is likely we will want
> to add timestamps/timeouts/durations to some errors eventually.
Thanks for the explanation!
> <...>
>
>>> +void
>>> +error_payload_set_mp(struct error_payload *e, const char *name,
>>> + const char *src, uint32_t size)
>>> +{
>>> + char *data;
>>> + if (mp_typeof(*src) == MP_STR) {
>>> + data = error_payload_prepare(e, name, size, 1)->data;
>>> + /*
>>> + * Add the terminating zero after the buffer so as get_str()
>>> + * would work without copying.
>>> + */
>> 5. Maybe shorten this comment to "\sa error_payload_set_str()" ?
> Done.
>
> ====================
> data = error_payload_prepare(e, name, size, 1)->data;
> - /*
> - * Add the terminating zero after the buffer so as get_str()
> - * would work without copying.
> - */
> + /* @sa error_payload_set_str(). */
> data[size] = 0;
> ====================
>
>>> diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c
>>> index b2341ae36..a60716eb5 100644
>>> --- a/src/lib/uuid/mp_uuid.c
>>> +++ b/src/lib/uuid/mp_uuid.c
>>> @@ -113,3 +114,28 @@ mp_fprint_uuid(FILE *file, const char **data, uint32_t len)
>>> return -1;
>>> return fprintf(file, "%s", tt_uuid_str(&uuid));
>>> }
>>> +
>>> +bool
>>> +error_payload_get_uuid(const struct error_payload *e, const char *name,
>>> + struct tt_uuid *uuid)
>>> +{
>>> + const struct error_field *f = error_payload_find(e, name);
>>> + if (f == NULL)
>>> + goto not_found;
>>> + const char *data = f->data;
>>> + if (mp_decode_uuid(&data, uuid) == NULL)
>>> + goto not_found;
>>> + return true;
>>> +
>>> +not_found:
>>> + *uuid = uuid_nil;
>>> + return false;
>>> +}
>>> +
>> 6. I propose to get rid of the label here and set uuid to nil unconditionally.
>>
>> Would it be too expensive to do 2 struct assignments instead of one?
>> (uuid = uuid_nil; uuid = decoded_value).
> Answered in the previous comment about gotos.
--
Serge Petrenko
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Tarantool-patches] [PATCH 3/9] error: move code to struct error from ClientError
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 1/9] diag: return created error from diag_set() Vladislav Shpilevoy via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 2/9] error: introduce error_payload Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-08 15:15 ` Serge Petrenko via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 4/9] error: use error_payload to store optional members Vladislav Shpilevoy via Tarantool-patches
` (6 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
All optional fields soon will be moved into error_payload. Code
was optional too. But it is needed too often, the most used field.
The patch moves it into struct error to make it more accessible.
Also in future it should allow to drop the hack
ClientError::get_errcode() which tries to return error code
depending on error type. But could just store the code right away.
As a consequence of the patch, errors which didn't have an error
code at all before, such as LuajitError, now have it 0 in Lua.
Part of #5568
---
src/box/error.cc | 15 +++++++--------
src/box/error.h | 4 +---
src/box/index.cc | 4 ++--
src/box/mp_error.cc | 7 +++----
src/lib/core/diag.c | 1 +
src/lib/core/diag.h | 2 ++
src/lua/error.lua | 3 ++-
test/engine/func_index.result | 3 ++-
test/unit/mp_error.cc | 2 +-
9 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/src/box/error.cc b/src/box/error.cc
index 225b377b7..bc0d46601 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -76,7 +76,7 @@ box_error_set(const char *file, unsigned line, uint32_t code,
struct error *e = BuildClientError(file, line, ER_UNKNOWN);
ClientError *client_error = type_cast(ClientError, e);
if (client_error) {
- client_error->m_errcode = code;
+ client_error->code = code;
va_list ap;
va_start(ap, fmt);
error_vformat_msg(e, fmt, ap);
@@ -94,7 +94,7 @@ box_error_new_va(const char *file, unsigned line, uint32_t code,
struct error *e = BuildClientError(file, line, ER_UNKNOWN);
ClientError *client_error = type_cast(ClientError, e);
if (client_error != NULL) {
- client_error->m_errcode = code;
+ client_error->code = code;
error_vformat_msg(e, fmt, ap);
}
return e;
@@ -170,7 +170,7 @@ ClientError::ClientError(const type_info *type, const char *file, unsigned line,
uint32_t errcode)
:Exception(type, file, line)
{
- m_errcode = errcode;
+ code = errcode;
if (rmean_error)
rmean_collect(rmean_error, RMEAN_ERROR, 1);
}
@@ -181,7 +181,7 @@ ClientError::ClientError(const char *file, unsigned line,
{
va_list ap;
va_start(ap, errcode);
- error_vformat_msg(this, tnt_errcode_desc(m_errcode), ap);
+ error_vformat_msg(this, tnt_errcode_desc(errcode), ap);
va_end(ap);
}
@@ -194,7 +194,7 @@ BuildClientError(const char *file, unsigned line, uint32_t errcode, ...)
va_start(ap, errcode);
error_vformat_msg(e, tnt_errcode_desc(errcode), ap);
va_end(ap);
- e->m_errcode = errcode;
+ e->code = errcode;
return e;
} catch (OutOfMemory *e) {
return e;
@@ -204,8 +204,7 @@ BuildClientError(const char *file, unsigned line, uint32_t errcode, ...)
void
ClientError::log() const
{
- say_file_line(S_ERROR, file, line, errmsg, "%s",
- tnt_errcode_str(m_errcode));
+ say_file_line(S_ERROR, file, line, errmsg, "%s", tnt_errcode_str(code));
}
@@ -296,7 +295,7 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
bool run_trigers)
:ClientError(&type_AccessDeniedError, file, line, ER_ACCESS_DENIED)
{
- error_format_msg(this, tnt_errcode_desc(m_errcode),
+ error_format_msg(this, tnt_errcode_desc(code),
access_type, object_type, object_name, user_name);
struct on_access_denied_ctx ctx = {access_type, object_type, object_name};
diff --git a/src/box/error.h b/src/box/error.h
index 338121dd9..30ab36a97 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -208,14 +208,12 @@ public:
int
errcode() const
{
- return m_errcode;
+ return code;
}
ClientError(const char *file, unsigned line, uint32_t errcode, ...);
static uint32_t get_errcode(const struct error *e);
- /* client errno code */
- int m_errcode;
protected:
ClientError(const type_info *type, const char *file, unsigned line,
uint32_t errcode);
diff --git a/src/box/index.cc b/src/box/index.cc
index 0651868d3..be14827be 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -47,8 +47,8 @@ UnsupportedIndexFeature::UnsupportedIndexFeature(const char *file,
: ClientError(file, line, ER_UNKNOWN)
{
struct space *space = space_cache_find_xc(index_def->space_id);
- m_errcode = ER_UNSUPPORTED_INDEX_FEATURE;
- error_format_msg(this, tnt_errcode_desc(m_errcode), index_def->name,
+ code = ER_UNSUPPORTED_INDEX_FEATURE;
+ error_format_msg(this, tnt_errcode_desc(code), index_def->name,
index_type_strs[index_def->type],
space->def->name, space->def->engine_name, what);
}
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index 059f9bc85..35c770400 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -261,10 +261,8 @@ missing_fields:
}
if (strcmp(mp_error->type, "ClientError") == 0) {
- ClientError *e = new ClientError(mp_error->file, mp_error->line,
- ER_UNKNOWN);
- e->m_errcode = mp_error->code;
- err = e;
+ err = new ClientError(mp_error->file, mp_error->line,
+ ER_UNKNOWN);
} else if (strcmp(mp_error->type, "CustomError") == 0) {
if (mp_error->custom_type == NULL)
goto missing_fields;
@@ -320,6 +318,7 @@ missing_fields:
err = new ClientError(mp_error->file, mp_error->line,
ER_UNKNOWN);
}
+ err->code = mp_error->code;
err->saved_errno = mp_error->saved_errno;
error_format_msg(err, "%s", mp_error->message);
return err;
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index f872be6a7..b557ca667 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -116,6 +116,7 @@ error_create(struct error *e,
e->type = type;
e->refs = 0;
e->saved_errno = 0;
+ e->code = 0;
if (file != NULL) {
snprintf(e->file, sizeof(e->file), "%s", file);
e->line = line;
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index d7d09f6fb..30b8d2f5d 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -87,6 +87,8 @@ struct error {
* to the standard library, then it is 0.
*/
int saved_errno;
+ /** Error code. Shortest possible description of error's reason. */
+ int code;
/** Line number. */
unsigned line;
/* Source file name. */
diff --git a/src/lua/error.lua b/src/lua/error.lua
index ebc784f07..9fb227df4 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -18,6 +18,7 @@ struct error {
const struct type_info *_type;
int64_t _refs;
int _saved_errno;
+ int code;
/** Line number. */
unsigned _line;
/* Source file name. */
@@ -158,7 +159,7 @@ local function error_unpack(err)
if not ffi.istype('struct error', err) then
error("Usage: error:unpack()")
end
- local result = {}
+ local result = {code = err.code}
for key, getter in pairs(error_fields) do
result[key] = getter(err)
end
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index b689c68ec..d3f44f7be 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -292,7 +292,8 @@ e = e.prev
| ...
e:unpack()
| ---
- | - base_type: LuajitError
+ | - code: 0
+ | base_type: LuajitError
| type: LuajitError
| message: '[string "return function(tuple) local ..."]:1: attempt
| to call global ''require'' (a nil value)'
diff --git a/test/unit/mp_error.cc b/test/unit/mp_error.cc
index e85d282e7..fe0cd49b9 100644
--- a/test/unit/mp_error.cc
+++ b/test/unit/mp_error.cc
@@ -349,7 +349,7 @@ test_decode_unknown_type()
const char *pos = buffer;
struct error *unpacked = error_unpack(&pos, len);
error_ref(unpacked);
- err.code = 0;
+ err.code = 1;
err.type = "ClientError";
ok(error_is_eq_mp_error(unpacked, &err), "check SomeNewError");
error_unref(unpacked);
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 3/9] error: move code to struct error from ClientError
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 3/9] error: move code to struct error from ClientError Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-08 15:15 ` Serge Petrenko via Tarantool-patches
2021-11-11 23:50 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-08 15:15 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, vdavydov
06.11.2021 02:56, Vladislav Shpilevoy пишет:
> All optional fields soon will be moved into error_payload. Code
> was optional too. But it is needed too often, the most used field.
> The patch moves it into struct error to make it more accessible.
>
> Also in future it should allow to drop the hack
> ClientError::get_errcode() which tries to return error code
> depending on error type. But could just store the code right away.
>
> As a consequence of the patch, errors which didn't have an error
> code at all before, such as LuajitError, now have it 0 in Lua.
>
> Part of #5568
Thanks for the patch! Please, find 2 minor comments below.
> ---
> src/box/error.cc | 15 +++++++--------
> src/box/error.h | 4 +---
> src/box/index.cc | 4 ++--
> src/box/mp_error.cc | 7 +++----
> src/lib/core/diag.c | 1 +
> src/lib/core/diag.h | 2 ++
> src/lua/error.lua | 3 ++-
> test/engine/func_index.result | 3 ++-
> test/unit/mp_error.cc | 2 +-
> 9 files changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 225b377b7..bc0d46601 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -76,7 +76,7 @@ box_error_set(const char *file, unsigned line, uint32_t code,
> struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> ClientError *client_error = type_cast(ClientError, e);
> if (client_error) {
> - client_error->m_errcode = code;
> + client_error->code = code;
> va_list ap;
> va_start(ap, fmt);
> error_vformat_msg(e, fmt, ap);
> @@ -94,7 +94,7 @@ box_error_new_va(const char *file, unsigned line, uint32_t code,
> struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> ClientError *client_error = type_cast(ClientError, e);
> if (client_error != NULL) {
> - client_error->m_errcode = code;
> + client_error->code = code;
> error_vformat_msg(e, fmt, ap);
> }
> return e;
> @@ -170,7 +170,7 @@ ClientError::ClientError(const type_info *type, const char *file, unsigned line,
> uint32_t errcode)
> :Exception(type, file, line)
> {
> - m_errcode = errcode;
> + code = errcode;
> if (rmean_error)
> rmean_collect(rmean_error, RMEAN_ERROR, 1);
> }
> @@ -181,7 +181,7 @@ ClientError::ClientError(const char *file, unsigned line,
> {
> va_list ap;
> va_start(ap, errcode);
> - error_vformat_msg(this, tnt_errcode_desc(m_errcode), ap);
> + error_vformat_msg(this, tnt_errcode_desc(errcode), ap);
> va_end(ap);
> }
>
> @@ -194,7 +194,7 @@ BuildClientError(const char *file, unsigned line, uint32_t errcode, ...)
> va_start(ap, errcode);
> error_vformat_msg(e, tnt_errcode_desc(errcode), ap);
> va_end(ap);
> - e->m_errcode = errcode;
> + e->code = errcode;
> return e;
> } catch (OutOfMemory *e) {
> return e;
> @@ -204,8 +204,7 @@ BuildClientError(const char *file, unsigned line, uint32_t errcode, ...)
> void
> ClientError::log() const
> {
> - say_file_line(S_ERROR, file, line, errmsg, "%s",
> - tnt_errcode_str(m_errcode));
> + say_file_line(S_ERROR, file, line, errmsg, "%s", tnt_errcode_str(code));
> }
>
>
> @@ -296,7 +295,7 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
> bool run_trigers)
> :ClientError(&type_AccessDeniedError, file, line, ER_ACCESS_DENIED)
> {
> - error_format_msg(this, tnt_errcode_desc(m_errcode),
> + error_format_msg(this, tnt_errcode_desc(code),
> access_type, object_type, object_name, user_name);
>
> struct on_access_denied_ctx ctx = {access_type, object_type, object_name};
> diff --git a/src/box/error.h b/src/box/error.h
> index 338121dd9..30ab36a97 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -208,14 +208,12 @@ public:
> int
> errcode() const
> {
> - return m_errcode;
> + return code;
> }
>
> ClientError(const char *file, unsigned line, uint32_t errcode, ...);
>
> static uint32_t get_errcode(const struct error *e);
> - /* client errno code */
> - int m_errcode;
> protected:
> ClientError(const type_info *type, const char *file, unsigned line,
> uint32_t errcode);
> diff --git a/src/box/index.cc b/src/box/index.cc
> index 0651868d3..be14827be 100644
> --- a/src/box/index.cc
> +++ b/src/box/index.cc
> @@ -47,8 +47,8 @@ UnsupportedIndexFeature::UnsupportedIndexFeature(const char *file,
> : ClientError(file, line, ER_UNKNOWN)
> {
> struct space *space = space_cache_find_xc(index_def->space_id);
> - m_errcode = ER_UNSUPPORTED_INDEX_FEATURE;
> - error_format_msg(this, tnt_errcode_desc(m_errcode), index_def->name,
> + code = ER_UNSUPPORTED_INDEX_FEATURE;
> + error_format_msg(this, tnt_errcode_desc(code), index_def->name,
> index_type_strs[index_def->type],
> space->def->name, space->def->engine_name, what);
> }
> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
> index 059f9bc85..35c770400 100644
> --- a/src/box/mp_error.cc
> +++ b/src/box/mp_error.cc
> @@ -261,10 +261,8 @@ missing_fields:
> }
>
> if (strcmp(mp_error->type, "ClientError") == 0) {
> - ClientError *e = new ClientError(mp_error->file, mp_error->line,
> - ER_UNKNOWN);
> - e->m_errcode = mp_error->code;
> - err = e;
> + err = new ClientError(mp_error->file, mp_error->line,
> + ER_UNKNOWN);
> } else if (strcmp(mp_error->type, "CustomError") == 0) {
> if (mp_error->custom_type == NULL)
> goto missing_fields;
> @@ -320,6 +318,7 @@ missing_fields:
> err = new ClientError(mp_error->file, mp_error->line,
> ER_UNKNOWN);
> }
> + err->code = mp_error->code;
> err->saved_errno = mp_error->saved_errno;
> error_format_msg(err, "%s", mp_error->message);
> return err;
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index f872be6a7..b557ca667 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -116,6 +116,7 @@ error_create(struct error *e,
> e->type = type;
> e->refs = 0;
> e->saved_errno = 0;
> + e->code = 0;
> if (file != NULL) {
> snprintf(e->file, sizeof(e->file), "%s", file);
> e->line = line;
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index d7d09f6fb..30b8d2f5d 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -87,6 +87,8 @@ struct error {
> * to the standard library, then it is 0.
> */
> int saved_errno;
> + /** Error code. Shortest possible description of error's reason. */
> + int code;
> /** Line number. */
> unsigned line;
> /* Source file name. */
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index ebc784f07..9fb227df4 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -18,6 +18,7 @@ struct error {
> const struct type_info *_type;
> int64_t _refs;
> int _saved_errno;
> + int code;
1. All the other fields are prefixed with "_" for some reason.
Let's do the same for "code", just to comply.
> /** Line number. */
> unsigned _line;
> /* Source file name. */
> @@ -158,7 +159,7 @@ local function error_unpack(err)
> if not ffi.istype('struct error', err) then
> error("Usage: error:unpack()")
> end
> - local result = {}
> + local result = {code = err.code}
2. Why not add 'code' to 'error_fields' ?
> for key, getter in pairs(error_fields) do
> result[key] = getter(err)
> end
> diff --git a/test/engine/func_index.result b/test/engine/func_index.result
> index b689c68ec..d3f44f7be 100644
> --- a/test/engine/func_index.result
> +++ b/test/engine/func_index.result
> @@ -292,7 +292,8 @@ e = e.prev
> | ...
> e:unpack()
> | ---
> - | - base_type: LuajitError
> + | - code: 0
> + | base_type: LuajitError
> | type: LuajitError
> | message: '[string "return function(tuple) local ..."]:1: attempt
> | to call global ''require'' (a nil value)'
> diff --git a/test/unit/mp_error.cc b/test/unit/mp_error.cc
> index e85d282e7..fe0cd49b9 100644
> --- a/test/unit/mp_error.cc
> +++ b/test/unit/mp_error.cc
> @@ -349,7 +349,7 @@ test_decode_unknown_type()
> const char *pos = buffer;
> struct error *unpacked = error_unpack(&pos, len);
> error_ref(unpacked);
> - err.code = 0;
> + err.code = 1;
> err.type = "ClientError";
> ok(error_is_eq_mp_error(unpacked, &err), "check SomeNewError");
> error_unref(unpacked);
--
Serge Petrenko
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 3/9] error: move code to struct error from ClientError
2021-11-08 15:15 ` Serge Petrenko via Tarantool-patches
@ 2021-11-11 23:50 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-12 6:31 ` Serge Petrenko via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:50 UTC (permalink / raw)
To: Serge Petrenko, tarantool-patches, vdavydov
Thanks for the review!
>> diff --git a/src/lua/error.lua b/src/lua/error.lua
>> index ebc784f07..9fb227df4 100644
>> --- a/src/lua/error.lua
>> +++ b/src/lua/error.lua
>> @@ -18,6 +18,7 @@ struct error {
>> const struct type_info *_type;
>> int64_t _refs;
>> int _saved_errno;
>> + int code;
>
> 1. All the other fields are prefixed with "_" for some reason.
> Let's do the same for "code", just to comply.
I named it without '_' intentionally. Because then Lua will see
this field right away when you do `err.code`. That also allows
me not to define a getter in `error_fields` table.
>> /** Line number. */
>> unsigned _line;
>> /* Source file name. */
>> @@ -158,7 +159,7 @@ local function error_unpack(err)
>> if not ffi.istype('struct error', err) then
>> error("Usage: error:unpack()")
>> end
>> - local result = {}
>> + local result = {code = err.code}
>
> 2. Why not add 'code' to 'error_fields' ?
Because it is visible without a getter anyway. On the downside, here
in unpack() I need to add it explicitly.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 3/9] error: move code to struct error from ClientError
2021-11-11 23:50 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-12 6:31 ` Serge Petrenko via Tarantool-patches
0 siblings, 0 replies; 24+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-12 6:31 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, vdavydov
12.11.2021 02:50, Vladislav Shpilevoy пишет:
> Thanks for the review!
>
>>> diff --git a/src/lua/error.lua b/src/lua/error.lua
>>> index ebc784f07..9fb227df4 100644
>>> --- a/src/lua/error.lua
>>> +++ b/src/lua/error.lua
>>> @@ -18,6 +18,7 @@ struct error {
>>> const struct type_info *_type;
>>> int64_t _refs;
>>> int _saved_errno;
>>> + int code;
>> 1. All the other fields are prefixed with "_" for some reason.
>> Let's do the same for "code", just to comply.
> I named it without '_' intentionally. Because then Lua will see
> this field right away when you do `err.code`. That also allows
> me not to define a getter in `error_fields` table.
>
>>> /** Line number. */
>>> unsigned _line;
>>> /* Source file name. */
>>> @@ -158,7 +159,7 @@ local function error_unpack(err)
>>> if not ffi.istype('struct error', err) then
>>> error("Usage: error:unpack()")
>>> end
>>> - local result = {}
>>> + local result = {code = err.code}
>> 2. Why not add 'code' to 'error_fields' ?
> Because it is visible without a getter anyway. On the downside, here
> in unpack() I need to add it explicitly.
Ok. Thanks for the explanation!
--
Serge Petrenko
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Tarantool-patches] [PATCH 4/9] error: use error_payload to store optional members
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
` (2 preceding siblings ...)
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 3/9] error: move code to struct error from ClientError Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 5/9] error: use error_payload in MessagePack codecs Vladislav Shpilevoy via Tarantool-patches
` (5 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
error_payload is a part of struct error now. All the fields stored
in C++ classes on top of struct error are moved into the payload.
Part of #5568
---
src/box/error.cc | 11 +++----
src/box/error.h | 34 +++++---------------
src/lib/core/diag.c | 2 ++
src/lib/core/diag.h | 70 +++++++++++++++++++++++++++++++++++++++++
src/lib/uuid/mp_uuid.c | 16 +++++++++-
src/lib/uuid/tt_uuid.h | 9 ++++++
src/lua/error.lua | 12 +++++++
test/box/error.result | 4 +--
test/box/error.test.lua | 2 +-
9 files changed, 124 insertions(+), 36 deletions(-)
diff --git a/src/box/error.cc b/src/box/error.cc
index bc0d46601..629ec703f 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -305,9 +305,9 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
*/
if (run_trigers)
trigger_run(&on_access_denied, (void *) &ctx);
- m_object_type = strdup(object_type);
- m_access_type = strdup(access_type);
- m_object_name = strdup(object_name);
+ error_field_set_str(this, "object_type", object_type);
+ error_field_set_str(this, "object_name", object_name);
+ error_field_set_str(this, "access_type", access_type);
}
struct error *
@@ -337,15 +337,14 @@ CustomError::CustomError(const char *file, unsigned int line,
const char *custom_type, uint32_t errcode)
:ClientError(&type_CustomError, file, line, errcode)
{
- strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
- m_custom_type[sizeof(m_custom_type) - 1] = '\0';
+ error_field_set_str(this, "custom_type", custom_type);
}
void
CustomError::log() const
{
say_file_line(S_ERROR, file, line, errmsg,
- "Custom type %s", m_custom_type);
+ "Custom type %s", custom_type());
}
struct error *
diff --git a/src/box/error.h b/src/box/error.h
index 30ab36a97..43faaea70 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -243,38 +243,23 @@ public:
const char *object_name, const char *user_name,
bool run_trigers = true);
- ~AccessDeniedError()
- {
- free(m_object_name);
- free(m_object_type);
- free(m_access_type);
- }
-
const char *
- object_type()
+ object_type() const
{
- return m_object_type;
+ return error_field_get_str(this, "object_type");
}
const char *
- object_name()
+ object_name() const
{
- return m_object_name?:"(nil)";
+ return error_field_get_str(this, "object_name");
}
const char *
- access_type()
+ access_type() const
{
- return m_access_type;
+ return error_field_get_str(this, "access_type");
}
-
-private:
- /** Type of object the required access was denied to */
- char *m_object_type;
- /** Name of object the required access was denied to */
- char *m_object_name;
- /** Type of declined access */
- char *m_access_type;
};
/**
@@ -319,13 +304,10 @@ public:
virtual void log() const;
const char*
- custom_type()
+ custom_type() const
{
- return m_custom_type;
+ return error_field_get_str(this, "custom_type");
}
-private:
- /** Custom type name. */
- char m_custom_type[64];
};
#endif /* defined(__cplusplus) */
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index b557ca667..217c3ae7e 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -53,6 +53,7 @@ error_unref(struct error *e)
to_delete->cause->effect = NULL;
to_delete->cause = NULL;
}
+ error_payload_destroy(&to_delete->payload);
to_delete->destroy(to_delete);
if (cause == NULL)
return;
@@ -117,6 +118,7 @@ error_create(struct error *e,
e->refs = 0;
e->saved_errno = 0;
e->code = 0;
+ error_payload_create(&e->payload);
if (file != NULL) {
snprintf(e->file, sizeof(e->file), "%s", file);
e->line = line;
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 30b8d2f5d..03bd223e1 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -36,6 +36,8 @@
#include <stdint.h>
#include <stdbool.h>
#include <assert.h>
+
+#include "error_payload.h"
#include "say.h"
#if defined(__cplusplus)
@@ -89,6 +91,8 @@ struct error {
int saved_errno;
/** Error code. Shortest possible description of error's reason. */
int code;
+ /** Key-value storage with error's dynamic fields. */
+ struct error_payload payload;
/** Line number. */
unsigned line;
/* Source file name. */
@@ -120,6 +124,72 @@ error_ref(struct error *e);
void
error_unref(struct error *e);
+static inline const char *
+error_field_get_str(const struct error *e, const char *name)
+{
+ return error_payload_get_str(&e->payload, name);
+}
+
+static inline void
+error_field_set_str(struct error *e, const char *name, const char *value)
+{
+ error_payload_set_str(&e->payload, name, value);
+}
+
+static inline bool
+error_field_get_uint(const struct error *e, const char *name, uint64_t *value)
+{
+ return error_payload_get_uint(&e->payload, name, value);
+}
+
+static inline void
+error_field_set_uint(struct error *e, const char *name, uint64_t value)
+{
+ error_payload_set_uint(&e->payload, name, value);
+}
+
+static inline bool
+error_field_get_int(const struct error *e, const char *name, int64_t *value)
+{
+ return error_payload_get_int(&e->payload, name, value);
+}
+
+static inline void
+error_field_set_int(struct error *e, const char *name, int64_t value)
+{
+ error_payload_set_int(&e->payload, name, value);
+}
+
+static inline bool
+error_field_get_double(const struct error *e, const char *name, double *value)
+{
+ return error_payload_get_double(&e->payload, name, value);
+}
+
+static inline void
+error_field_set_double(struct error *e, const char *name, double value)
+{
+ error_payload_set_double(&e->payload, name, value);
+}
+
+static inline bool
+error_field_get_bool(const struct error *e, const char *name, bool *value)
+{
+ return error_payload_get_bool(&e->payload, name, value);
+}
+
+static inline void
+error_field_set_bool(struct error *e, const char *name, bool value)
+{
+ error_payload_set_bool(&e->payload, name, value);
+}
+
+static inline void
+error_field_unset(struct error *e, const char *name)
+{
+ error_payload_unset(&e->payload, name);
+}
+
/**
* Unlink error from its effect. For instance:
* e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c
index a60716eb5..51f0465ac 100644
--- a/src/lib/uuid/mp_uuid.c
+++ b/src/lib/uuid/mp_uuid.c
@@ -32,7 +32,7 @@
#include "mp_uuid.h"
#include "msgpuck.h"
#include "mp_extension_types.h"
-#include "error_payload.h"
+#include "diag.h"
inline uint32_t
mp_sizeof_uuid(void)
@@ -139,3 +139,17 @@ error_payload_set_uuid(struct error_payload *e, const char *name,
char *data = error_payload_prepare(e, name, mp_sizeof_uuid(), 0)->data;
mp_encode_uuid(data, uuid);
}
+
+bool
+error_field_get_uuid(const struct error *e, const char *name,
+ struct tt_uuid *uuid)
+{
+ return error_payload_get_uuid(&e->payload, name, uuid);
+}
+
+void
+error_field_set_uuid(struct error *e, const char *name,
+ const struct tt_uuid *uuid)
+{
+ error_payload_set_uuid(&e->payload, name, uuid);
+}
diff --git a/src/lib/uuid/tt_uuid.h b/src/lib/uuid/tt_uuid.h
index 866764e77..eb457c1f1 100644
--- a/src/lib/uuid/tt_uuid.h
+++ b/src/lib/uuid/tt_uuid.h
@@ -41,6 +41,7 @@
extern "C" {
#endif
+struct error;
struct error_payload;
enum { UUID_LEN = 16, UUID_STR_LEN = 36 };
@@ -206,6 +207,14 @@ void
error_payload_set_uuid(struct error_payload *e, const char *name,
const struct tt_uuid *uuid);
+bool
+error_field_get_uuid(const struct error *e, const char *name,
+ struct tt_uuid *uuid);
+
+void
+error_field_set_uuid(struct error *e, const char *name,
+ const struct tt_uuid *uuid);
+
#if defined(__cplusplus)
} /* extern "C" */
#endif
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 9fb227df4..a5d1d2a43 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -11,6 +11,17 @@ enum {
typedef void (*error_f)(struct error *e);
+struct error_field {
+ uint32_t _size;
+ const char *_data;
+ char _name[0];
+};
+
+struct error_payload {
+ int _count;
+ struct error_field **_fields;
+};
+
struct error {
error_f _destroy;
error_f _raise;
@@ -19,6 +30,7 @@ struct error {
int64_t _refs;
int _saved_errno;
int code;
+ struct error_payload _payload;
/** Line number. */
unsigned _line;
/* Source file name. */
diff --git a/test/box/error.result b/test/box/error.result
index 19ea53873..7583306a6 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -899,13 +899,13 @@ e:unpack()
| - file: '[string "e = box.error.new({type = ''TestType''}) "]'
| line: 1
| ...
--- Try too long type name.
+-- Try long type name.
e = box.error.new({type = string.rep('a', 128)})
| ---
| ...
#e.type
| ---
- | - 63
+ | - 128
| ...
-- gh-4887: accessing 'prev' member also refs it so that after
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index ed95d1d41..068212d96 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -241,7 +241,7 @@ e:unpack()
-- Try to omit message.
e = box.error.new({type = 'TestType'})
e:unpack()
--- Try too long type name.
+-- Try long type name.
e = box.error.new({type = string.rep('a', 128)})
#e.type
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* [Tarantool-patches] [PATCH 5/9] error: use error_payload in MessagePack codecs
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
` (3 preceding siblings ...)
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 4/9] error: use error_payload to store optional members Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 6/9] error: use error_payload in Lua Vladislav Shpilevoy via Tarantool-patches
` (4 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
Before this patch mp_error API could only encode and decode
hardcoded fields from the C++ classes inheriting struct error.
The fields are gone from the classes in a previous patch - moved
into error_payload. Now to be able to support arbitrary fields in
the payload the MessagePack encoding/decoding must use its content
instead of hardcoded fields depending on error type.
Part of #5568
---
src/box/error.cc | 7 --
src/box/error.h | 28 +++++-
src/box/mp_error.cc | 194 +++++++++++---------------------------
src/lib/core/diag.c | 17 ++--
src/lib/core/diag.h | 9 ++
src/lib/core/exception.h | 66 +++++++++++++
test/unit/mp_error.cc | 76 ++++++++++++++-
test/unit/mp_error.result | 27 +++++-
8 files changed, 262 insertions(+), 162 deletions(-)
diff --git a/src/box/error.cc b/src/box/error.cc
index 629ec703f..be3b3b6f5 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -256,13 +256,6 @@ XlogGapError::XlogGapError(const char *file, unsigned line,
(long long) vclock_sum(to), s_to ? s_to : "");
}
-XlogGapError::XlogGapError(const char *file, unsigned line,
- const char *msg)
- : XlogError(&type_XlogGapError, file, line)
-{
- error_format_msg(this, "%s", msg);
-}
-
struct error *
BuildXlogGapError(const char *file, unsigned line,
const struct vclock *from, const struct vclock *to)
diff --git a/src/box/error.h b/src/box/error.h
index 43faaea70..c2ccf48bb 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -213,6 +213,11 @@ public:
ClientError(const char *file, unsigned line, uint32_t errcode, ...);
+ ClientError()
+ :Exception(&type_ClientError, NULL, 0)
+ {
+ }
+
static uint32_t get_errcode(const struct error *e);
protected:
ClientError(const type_info *type, const char *file, unsigned line,
@@ -243,6 +248,11 @@ public:
const char *object_name, const char *user_name,
bool run_trigers = true);
+ AccessDeniedError()
+ :ClientError(&type_AccessDeniedError, NULL, 0, 0)
+ {
+ }
+
const char *
object_type() const
{
@@ -276,6 +286,12 @@ struct XlogError: public Exception
{
error_vformat_msg(this, format, ap);
}
+
+ XlogError()
+ :Exception(&type_XlogError, NULL, 0)
+ {
+ }
+
XlogError(const struct type_info *type, const char *file,
unsigned line)
:Exception(type, file, line)
@@ -289,8 +305,11 @@ struct XlogGapError: public XlogError
{
XlogGapError(const char *file, unsigned line,
const struct vclock *from, const struct vclock *to);
- XlogGapError(const char *file, unsigned line,
- const char *msg);
+
+ XlogGapError()
+ :XlogError(&type_XlogGapError, NULL, 0)
+ {
+ }
virtual void raise() { throw this; }
};
@@ -301,6 +320,11 @@ public:
CustomError(const char *file, unsigned int line,
const char *custom_type, uint32_t errcode);
+ CustomError()
+ :ClientError(&type_CustomError, NULL, 0, 0)
+ {
+ }
+
virtual void log() const;
const char*
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index 35c770400..fba562a84 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -118,35 +118,31 @@ struct mp_error {
const char *type;
const char *file;
const char *message;
- const char *custom_type;
- const char *ad_object_type;
- const char *ad_object_name;
- const char *ad_access_type;
+ struct error_payload payload;
};
static void
mp_error_create(struct mp_error *mp_error)
{
memset(mp_error, 0, sizeof(*mp_error));
+ error_payload_create(&mp_error->payload);
+}
+
+static void
+mp_error_destroy(struct mp_error *mp_error)
+{
+ error_payload_destroy(&mp_error->payload);
}
static uint32_t
mp_sizeof_error_one(const struct error *error)
{
uint32_t errcode = box_error_code(error);
-
- bool is_custom = false;
- bool is_access_denied = false;
-
- if (strcmp(error->type->name, "CustomError") == 0) {
- is_custom = true;
- } else if (strcmp(error->type->name, "AccessDeniedError") == 0) {
- is_access_denied = true;
- }
-
- uint32_t details_num = 6;
+ int field_count = error->payload.count;
+ uint32_t map_size = 6 + (field_count > 0);
uint32_t data_size = 0;
+ data_size += mp_sizeof_map(map_size);
data_size += mp_sizeof_uint(MP_ERROR_TYPE);
data_size += mp_sizeof_str(strlen(error->type->name));
data_size += mp_sizeof_uint(MP_ERROR_LINE);
@@ -160,28 +156,15 @@ mp_sizeof_error_one(const struct error *error)
data_size += mp_sizeof_uint(MP_ERROR_CODE);
data_size += mp_sizeof_uint(errcode);
- if (is_access_denied) {
- ++details_num;
- data_size += mp_sizeof_uint(MP_ERROR_FIELDS);
- data_size += mp_sizeof_map(3);
- AccessDeniedError *ad_err = type_cast(AccessDeniedError, error);
- data_size += mp_sizeof_str(strlen("object_type"));
- data_size += mp_sizeof_str(strlen(ad_err->object_type()));
- data_size += mp_sizeof_str(strlen("object_name"));
- data_size += mp_sizeof_str(strlen(ad_err->object_name()));
- data_size += mp_sizeof_str(strlen("access_type"));
- data_size += mp_sizeof_str(strlen(ad_err->access_type()));
- } else if (is_custom) {
- ++details_num;
+ if (field_count > 0) {
data_size += mp_sizeof_uint(MP_ERROR_FIELDS);
- data_size += mp_sizeof_map(1);
- data_size += mp_sizeof_str(strlen("custom_type"));
- data_size +=
- mp_sizeof_str(strlen(box_error_custom_type(error)));
+ data_size += mp_sizeof_map(field_count);
+ for (int i = 0; i < field_count; ++i) {
+ const struct error_field *f = error->payload.fields[i];
+ data_size += mp_sizeof_str(strlen(f->name));
+ data_size += f->size;
+ }
}
-
- data_size += mp_sizeof_map(details_num);
-
return data_size;
}
@@ -195,21 +178,10 @@ static char *
mp_encode_error_one(char *data, const struct error *error)
{
uint32_t errcode = box_error_code(error);
+ int field_count = error->payload.count;
+ uint32_t map_size = 6 + (field_count > 0);
- bool is_custom = false;
- bool is_access_denied = false;
-
- if (strcmp(error->type->name, "CustomError") == 0) {
- is_custom = true;
- } else if (strcmp(error->type->name, "AccessDeniedError") == 0) {
- is_access_denied = true;
- }
-
- uint32_t details_num = 6;
- if (is_access_denied || is_custom)
- ++details_num;
-
- data = mp_encode_map(data, details_num);
+ data = mp_encode_map(data, map_size);
data = mp_encode_uint(data, MP_ERROR_TYPE);
data = mp_encode_str0(data, error->type->name);
data = mp_encode_uint(data, MP_ERROR_LINE);
@@ -223,21 +195,15 @@ mp_encode_error_one(char *data, const struct error *error)
data = mp_encode_uint(data, MP_ERROR_CODE);
data = mp_encode_uint(data, errcode);
- if (is_access_denied) {
- data = mp_encode_uint(data, MP_ERROR_FIELDS);
- data = mp_encode_map(data, 3);
- AccessDeniedError *ad_err = type_cast(AccessDeniedError, error);
- data = mp_encode_str0(data, "object_type");
- data = mp_encode_str0(data, ad_err->object_type());
- data = mp_encode_str0(data, "object_name");
- data = mp_encode_str0(data, ad_err->object_name());
- data = mp_encode_str0(data, "access_type");
- data = mp_encode_str0(data, ad_err->access_type());
- } else if (is_custom) {
+ if (field_count > 0) {
data = mp_encode_uint(data, MP_ERROR_FIELDS);
- data = mp_encode_map(data, 1);
- data = mp_encode_str0(data, "custom_type");
- data = mp_encode_str0(data, box_error_custom_type(error));
+ data = mp_encode_map(data, field_count);
+ for (int i = 0; i < field_count; ++i) {
+ const struct error_field *f = error->payload.fields[i];
+ data = mp_encode_str0(data, f->name);
+ memcpy(data, f->data, f->size);
+ data += f->size;
+ }
}
return data;
}
@@ -254,72 +220,50 @@ error_build_xc(struct mp_error *mp_error)
struct error *err = NULL;
if (mp_error->type == NULL || mp_error->message == NULL ||
mp_error->file == NULL) {
-missing_fields:
diag_set(ClientError, ER_INVALID_MSGPACK,
"Missing mandatory error fields");
return NULL;
}
if (strcmp(mp_error->type, "ClientError") == 0) {
- err = new ClientError(mp_error->file, mp_error->line,
- ER_UNKNOWN);
+ err = new ClientError();
} else if (strcmp(mp_error->type, "CustomError") == 0) {
- if (mp_error->custom_type == NULL)
- goto missing_fields;
- err = new CustomError(mp_error->file, mp_error->line,
- mp_error->custom_type, mp_error->code);
+ err = new CustomError();
} else if (strcmp(mp_error->type, "AccessDeniedError") == 0) {
- if (mp_error->ad_access_type == NULL ||
- mp_error->ad_object_type == NULL ||
- mp_error->ad_object_name == NULL)
- goto missing_fields;
- err = new AccessDeniedError(mp_error->file, mp_error->line,
- mp_error->ad_access_type,
- mp_error->ad_object_type,
- mp_error->ad_object_name, "",
- false);
+ err = new AccessDeniedError();
} else if (strcmp(mp_error->type, "XlogError") == 0) {
- err = new XlogError(&type_XlogError, mp_error->file,
- mp_error->line);
+ err = new XlogError();
} else if (strcmp(mp_error->type, "XlogGapError") == 0) {
- err = new XlogGapError(mp_error->file, mp_error->line,
- mp_error->message);
+ err = new XlogGapError();
} else if (strcmp(mp_error->type, "SystemError") == 0) {
- err = new SystemError(mp_error->file, mp_error->line,
- "%s", mp_error->message);
+ err = new SystemError();
} else if (strcmp(mp_error->type, "SocketError") == 0) {
- err = new SocketError(mp_error->file, mp_error->line, "", "");
- error_format_msg(err, "%s", mp_error->message);
+ err = new SocketError();
} else if (strcmp(mp_error->type, "OutOfMemory") == 0) {
- err = new OutOfMemory(mp_error->file, mp_error->line,
- 0, "", "");
+ err = new OutOfMemory();
} else if (strcmp(mp_error->type, "TimedOut") == 0) {
- err = new TimedOut(mp_error->file, mp_error->line);
+ err = new TimedOut();
} else if (strcmp(mp_error->type, "ChannelIsClosed") == 0) {
- err = new ChannelIsClosed(mp_error->file, mp_error->line);
+ err = new ChannelIsClosed();
} else if (strcmp(mp_error->type, "FiberIsCancelled") == 0) {
- err = new FiberIsCancelled(mp_error->file, mp_error->line);
+ err = new FiberIsCancelled();
} else if (strcmp(mp_error->type, "LuajitError") == 0) {
- err = new LuajitError(mp_error->file, mp_error->line,
- mp_error->message);
+ err = new LuajitError();
} else if (strcmp(mp_error->type, "IllegalParams") == 0) {
- err = new IllegalParams(mp_error->file, mp_error->line,
- "%s", mp_error->message);
+ err = new IllegalParams();
} else if (strcmp(mp_error->type, "CollationError") == 0) {
- err = new CollationError(mp_error->file, mp_error->line,
- "%s", mp_error->message);
+ err = new CollationError();
} else if (strcmp(mp_error->type, "SwimError") == 0) {
- err = new SwimError(mp_error->file, mp_error->line,
- "%s", mp_error->message);
+ err = new SwimError();
} else if (strcmp(mp_error->type, "CryptoError") == 0) {
- err = new CryptoError(mp_error->file, mp_error->line,
- "%s", mp_error->message);
+ err = new CryptoError();
} else {
- err = new ClientError(mp_error->file, mp_error->line,
- ER_UNKNOWN);
+ err = new ClientError();
}
err->code = mp_error->code;
err->saved_errno = mp_error->saved_errno;
+ error_set_location(err, mp_error->file, mp_error->line);
+ error_move_payload(err, &mp_error->payload);
error_format_msg(err, "%s", mp_error->message);
return err;
}
@@ -350,12 +294,6 @@ mp_decode_and_copy_str(const char **data, struct region *region)
return region_strdup(region, str, str_len);;
}
-static inline bool
-str_nonterm_is_eq(const char *l, const char *r, uint32_t r_len)
-{
- return r_len == strlen(l) && memcmp(l, r, r_len) == 0;
-}
-
static int
mp_decode_error_fields(const char **data, struct mp_error *mp_err,
struct region *region)
@@ -363,35 +301,16 @@ mp_decode_error_fields(const char **data, struct mp_error *mp_err,
if (mp_typeof(**data) != MP_MAP)
return -1;
uint32_t map_sz = mp_decode_map(data);
- const char *key;
- uint32_t key_len;
for (uint32_t i = 0; i < map_sz; ++i) {
- if (mp_typeof(**data) != MP_STR)
+ uint32_t svp = region_used(region);
+ const char *key = mp_decode_and_copy_str(data, region);
+ if (key == NULL)
return -1;
- key = mp_decode_str(data, &key_len);
- if (str_nonterm_is_eq("object_type", key, key_len)) {
- mp_err->ad_object_type =
- mp_decode_and_copy_str(data, region);
- if (mp_err->ad_object_type == NULL)
- return -1;
- } else if (str_nonterm_is_eq("object_name", key, key_len)) {
- mp_err->ad_object_name =
- mp_decode_and_copy_str(data, region);
- if (mp_err->ad_object_name == NULL)
- return -1;
- } else if (str_nonterm_is_eq("access_type", key, key_len)) {
- mp_err->ad_access_type =
- mp_decode_and_copy_str(data, region);
- if (mp_err->ad_access_type == NULL)
- return -1;
- } else if (str_nonterm_is_eq("custom_type", key, key_len)) {
- mp_err->custom_type =
- mp_decode_and_copy_str(data, region);
- if (mp_err->custom_type == NULL)
- return -1;
- } else {
- mp_next(data);
- }
+ const char *value = *data;
+ mp_next(data);
+ uint32_t value_len = *data - value;
+ error_payload_set_mp(&mp_err->payload, key, value, value_len);
+ region_truncate(region, svp);
}
return 0;
}
@@ -462,6 +381,7 @@ mp_decode_error_one(const char **data)
}
finish:
region_truncate(region, region_svp);
+ mp_error_destroy(&mp_err);
return err;
error:
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index 217c3ae7e..b6fa1f5bb 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -119,18 +119,21 @@ error_create(struct error *e,
e->saved_errno = 0;
e->code = 0;
error_payload_create(&e->payload);
- if (file != NULL) {
- snprintf(e->file, sizeof(e->file), "%s", file);
- e->line = line;
- } else {
- e->file[0] = '\0';
- e->line = 0;
- }
+ if (file == NULL)
+ file = "";
+ error_set_location(e, file, line);
e->errmsg[0] = '\0';
e->cause = NULL;
e->effect = NULL;
}
+void
+error_set_location(struct error *e, const char *file, int line)
+{
+ snprintf(e->file, sizeof(e->file), "%s", file);
+ e->line = line;
+}
+
struct diag *
diag_get(void)
{
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 03bd223e1..40d934c19 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -190,6 +190,12 @@ error_field_unset(struct error *e, const char *name)
error_payload_unset(&e->payload, name);
}
+static inline void
+error_move_payload(struct error *e, struct error_payload *src)
+{
+ error_payload_move(&e->payload, src);
+}
+
/**
* Unlink error from its effect. For instance:
* e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
@@ -243,6 +249,9 @@ error_create(struct error *e,
const struct type_info *type, const char *file,
unsigned line);
+void
+error_set_location(struct error *e, const char *file, int line);
+
void
error_format_msg(struct error *e, const char *format, ...);
diff --git a/src/lib/core/exception.h b/src/lib/core/exception.h
index 7277b2784..28bc7ef05 100644
--- a/src/lib/core/exception.h
+++ b/src/lib/core/exception.h
@@ -91,6 +91,12 @@ public:
SystemError(const char *file, unsigned line,
const char *format, ...);
+
+ SystemError()
+ :Exception(&type_SystemError, NULL, 0)
+ {
+ }
+
protected:
SystemError(const struct type_info *type, const char *file, unsigned line);
};
@@ -100,6 +106,12 @@ class SocketError: public SystemError {
public:
SocketError(const char *file, unsigned line, const char *socketname,
const char *format, ...);
+
+ SocketError()
+ :SystemError(&type_SocketError, file, line)
+ {
+ }
+
virtual void raise()
{
throw this;
@@ -111,18 +123,36 @@ public:
OutOfMemory(const char *file, unsigned line,
size_t amount, const char *allocator,
const char *object);
+
+ OutOfMemory()
+ :SystemError(&type_OutOfMemory, NULL, 0)
+ {
+ }
+
virtual void raise() { throw this; }
};
class TimedOut: public SystemError {
public:
TimedOut(const char *file, unsigned line);
+
+ TimedOut()
+ :SystemError(&type_TimedOut, NULL, 0)
+ {
+ }
+
virtual void raise() { throw this; }
};
class ChannelIsClosed: public Exception {
public:
ChannelIsClosed(const char *file, unsigned line);
+
+ ChannelIsClosed()
+ :Exception(&type_ChannelIsClosed, NULL, 0)
+ {
+ }
+
virtual void raise() { throw this; }
};
@@ -133,6 +163,12 @@ public:
class FiberIsCancelled: public Exception {
public:
FiberIsCancelled(const char *file, unsigned line);
+
+ FiberIsCancelled()
+ :Exception(&type_FiberIsCancelled, NULL, 0)
+ {
+ }
+
virtual void log() const;
virtual void raise() { throw this; }
};
@@ -141,12 +177,24 @@ class LuajitError: public Exception {
public:
LuajitError(const char *file, unsigned line,
const char *msg);
+
+ LuajitError()
+ :Exception(&type_LuajitError, NULL, 0)
+ {
+ }
+
virtual void raise() { throw this; }
};
class IllegalParams: public Exception {
public:
IllegalParams(const char *file, unsigned line, const char *format, ...);
+
+ IllegalParams()
+ :Exception(&type_IllegalParams, NULL, 0)
+ {
+ }
+
virtual void raise() { throw this; }
};
@@ -154,18 +202,36 @@ class CollationError: public Exception {
public:
CollationError(const char *file, unsigned line, const char *format,
...);
+
+ CollationError()
+ :Exception(&type_CollationError, NULL, 0)
+ {
+ }
+
virtual void raise() { throw this; }
};
class SwimError: public Exception {
public:
SwimError(const char *file, unsigned line, const char *format, ...);
+
+ SwimError()
+ :Exception(&type_SwimError, NULL, 0)
+ {
+ }
+
virtual void raise() { throw this; }
};
class CryptoError: public Exception {
public:
CryptoError(const char *file, unsigned line, const char *format, ...);
+
+ CryptoError()
+ :Exception(&type_CryptoError, NULL, 0)
+ {
+ }
+
virtual void raise() { throw this; }
};
diff --git a/test/unit/mp_error.cc b/test/unit/mp_error.cc
index fe0cd49b9..e6560883e 100644
--- a/test/unit/mp_error.cc
+++ b/test/unit/mp_error.cc
@@ -34,9 +34,11 @@
#include "memory.h"
#include "msgpuck.h"
#include "mp_extension_types.h"
+#include "random.h"
#include "tt_static.h"
#include "small/ibuf.h"
#include "mpstream/mpstream.h"
+#include "uuid/tt_uuid.h"
#include "box/error.h"
#include "box/mp_error.h"
@@ -362,7 +364,7 @@ void
test_fail_not_enough_fields()
{
header();
- plan(2);
+ plan(4);
char buffer[2048];
memset(buffer, 0, sizeof(buffer));
@@ -383,8 +385,12 @@ test_fail_not_enough_fields()
const char *pos = buffer;
struct error *unpacked = error_unpack(&pos, len);
- is(unpacked, NULL, "check not enough additional fields");
- ok(!diag_is_empty(diag_get()), "error about parsing problem is set");
+ isnt(unpacked, NULL, "check lack of fields");
+ is(strcmp(error_field_get_str(unpacked, "object_type"),
+ err.ad_object_type), 0, "object type");
+ is(strcmp(error_field_get_str(unpacked, "access_type"),
+ err.ad_access_type), 0, "access type");
+ is(error_field_get_str(unpacked, "object_name"), NULL, "object name");
check_plan();
footer();
}
@@ -455,6 +461,65 @@ test_unknown_additional_fields()
footer();
}
+static void
+test_payload(void)
+{
+ header();
+ plan(11);
+ char buffer[2048];
+ memset(buffer, 0, sizeof(buffer));
+
+ struct error *e = new ClientError();
+ error_ref(e);
+ e->code = 42;
+ e->saved_errno = 1;
+ error_format_msg(e, "msg");
+ error_set_location(e, "file", 2);
+ error_field_set_str(e, "key1", "1");
+ error_field_set_uint(e, "key2", 1);
+ error_field_set_int(e, "key3", -1);
+ error_field_set_double(e, "key4", 1.5);
+ error_field_set_bool(e, "key5", true);
+ struct tt_uuid uuid;
+ tt_uuid_create(&uuid);
+ error_field_set_uuid(e, "key6", &uuid);
+
+ mp_encode_error(buffer, e);
+ error_unref(e);
+
+ int8_t type;
+ const char *data = buffer;
+ mp_decode_extl(&data, &type);
+ e = error_unpack_unsafe(&data);
+ error_ref(e);
+
+ is(e->code, 42, "code");
+ is(e->saved_errno, 1, "errno");
+ is(strcmp(e->errmsg, "msg"), 0, "msg");
+ is(e->line, 2, "line");
+ is(strcmp(e->file, "file"), 0, "file");
+ is(strcmp(error_field_get_str(e, "key1"), "1"), 0, "key str");
+ uint64_t val_uint;
+ ok(error_field_get_uint(e, "key2", &val_uint) && val_uint == 1,
+ "key uint");
+ int64_t val_int;
+ ok(error_field_get_int(e, "key3", &val_int) && val_int == -1,
+ "key int");
+ double val_dbl;
+ ok(error_field_get_double(e, "key4", &val_dbl) && val_dbl == 1.5,
+ "key double");
+ bool val_bool;
+ ok(error_field_get_bool(e, "key5", &val_bool) && val_bool, "key bool");
+ struct tt_uuid val_uuid;
+ ok(error_field_get_uuid(e, "key6", &val_uuid) &&
+ tt_uuid_is_equal(&uuid, &val_uuid), "key uuid");
+
+ error_unref(e);
+
+ check_plan();
+ footer();
+}
+
static int
mp_fprint_ext_test(FILE *file, const char **data, int depth)
{
@@ -723,7 +788,8 @@ int
main(void)
{
header();
- plan(6);
+ plan(7);
+ random_init();
memory_init();
fiber_init(fiber_c_invoke);
@@ -732,10 +798,12 @@ main(void)
test_fail_not_enough_fields();
test_unknown_fields();
test_unknown_additional_fields();
+ test_payload();
test_mp_print();
fiber_free();
memory_free();
+ random_free();
footer();
return check_plan();
}
diff --git a/test/unit/mp_error.result b/test/unit/mp_error.result
index 0582458d3..356d2dc97 100644
--- a/test/unit/mp_error.result
+++ b/test/unit/mp_error.result
@@ -1,5 +1,5 @@
*** main ***
-1..6
+1..7
*** test_stack_error_decode ***
1..17
ok 1 - check CustomError
@@ -27,9 +27,11 @@ ok 1 - subtests
ok 2 - subtests
*** test_decode_unknown_type: done ***
*** test_fail_not_enough_fields ***
- 1..2
- ok 1 - check not enough additional fields
- ok 2 - error about parsing problem is set
+ 1..4
+ ok 1 - check lack of fields
+ ok 2 - object type
+ ok 3 - access type
+ ok 4 - object name
ok 3 - subtests
*** test_fail_not_enough_fields: done ***
*** test_unknown_fields ***
@@ -41,6 +43,21 @@ ok 4 - subtests
ok 1 - check unknown additional field
ok 5 - subtests
*** test_unknown_additional_fields: done ***
+ *** test_payload ***
+ 1..11
+ ok 1 - code
+ ok 2 - errno
+ ok 3 - msg
+ ok 4 - line
+ ok 5 - file
+ ok 6 - key str
+ ok 7 - key uint
+ ok 8 - key int
+ ok 9 - key double
+ ok 10 - key bool
+ ok 11 - key uuid
+ok 6 - subtests
+ *** test_payload: done ***
*** test_mp_print ***
1..60
# zero depth, normal error
@@ -109,6 +126,6 @@ ok 5 - subtests
ok 58 - mp_fprint depth 0 correct length
ok 59 - mp_fprint depth 0 correct prefix and suffix
ok 60 - mp_fprint depth 0 correct object in the middle
-ok 6 - subtests
+ok 7 - subtests
*** test_mp_print: done ***
*** main: done ***
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* [Tarantool-patches] [PATCH 6/9] error: use error_payload in Lua
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
` (4 preceding siblings ...)
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 5/9] error: use error_payload in MessagePack codecs Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 7/9] luatest: copy config in cluster:build_server() Vladislav Shpilevoy via Tarantool-patches
` (3 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
In Lua struct error used RTTI to return members of the error
depending on its type. If a field was added to error's payload, it
wasn't visible. The patch makes Lua use error_payload instead of
RTTI. Now if the payload gets a new field, it becomes
automatically visible in Lua without need to introduce a new
method for it.
Part of #5568
---
| 3 +-
src/box/error.cc | 24 ++------------
src/lib/core/diag.c | 6 ++++
src/lib/core/diag.h | 3 ++
src/lib/core/exception.cc | 8 +----
src/lua/error.lua | 69 +++++++++------------------------------
src/lua/init.lua | 24 --------------
7 files changed, 29 insertions(+), 108 deletions(-)
--git a/extra/exports b/extra/exports
index 19e23f5db..4e9cea2eb 100644
--- a/extra/exports
+++ b/extra/exports
@@ -151,12 +151,11 @@ csv_next
csv_setopt
decimal_from_string
decimal_unpack
+error_field_find
error_ref
error_set_prev
error_unpack_unsafe
error_unref
-exception_get_int
-exception_get_string
fiber_attr_delete
fiber_attr_getstacksize
fiber_attr_new
diff --git a/src/box/error.cc b/src/box/error.cc
index be3b3b6f5..e0a1894e1 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -158,13 +158,8 @@ const char *rmean_error_strings[RMEAN_ERROR_LAST] = {
"ERROR"
};
-static struct method_info clienterror_methods[] = {
- make_method(&type_ClientError, "code", &ClientError::errcode),
- METHODS_SENTINEL
-};
-
const struct type_info type_ClientError =
- make_type("ClientError", &type_Exception, clienterror_methods);
+ make_type("ClientError", &type_Exception);
ClientError::ClientError(const type_info *type, const char *file, unsigned line,
uint32_t errcode)
@@ -269,16 +264,8 @@ BuildXlogGapError(const char *file, unsigned line,
struct rlist on_access_denied = RLIST_HEAD_INITIALIZER(on_access_denied);
-static struct method_info accessdeniederror_methods[] = {
- make_method(&type_AccessDeniedError, "access_type", &AccessDeniedError::access_type),
- make_method(&type_AccessDeniedError, "object_type", &AccessDeniedError::object_type),
- make_method(&type_AccessDeniedError, "object_name", &AccessDeniedError::object_name),
- METHODS_SENTINEL
-};
-
const struct type_info type_AccessDeniedError =
- make_type("AccessDeniedError", &type_ClientError,
- accessdeniederror_methods);
+ make_type("AccessDeniedError", &type_ClientError);
AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
const char *access_type,
@@ -318,13 +305,8 @@ BuildAccessDeniedError(const char *file, unsigned int line,
}
}
-static struct method_info customerror_methods[] = {
- make_method(&type_CustomError, "custom_type", &CustomError::custom_type),
- METHODS_SENTINEL
-};
-
const struct type_info type_CustomError =
- make_type("CustomError", &type_ClientError, customerror_methods);
+ make_type("CustomError", &type_ClientError);
CustomError::CustomError(const char *file, unsigned int line,
const char *custom_type, uint32_t errcode)
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index b6fa1f5bb..f48ea231f 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -61,6 +61,12 @@ error_unref(struct error *e)
}
}
+const struct error_field *
+error_field_find(const struct error *e, const char *name)
+{
+ return error_payload_find(&e->payload, name);
+}
+
int
error_set_prev(struct error *e, struct error *prev)
{
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 40d934c19..c754fc737 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -196,6 +196,9 @@ error_move_payload(struct error *e, struct error_payload *src)
error_payload_move(&e->payload, src);
}
+const struct error_field *
+error_field_find(const struct error *e, const char *name);
+
/**
* Unlink error from its effect. For instance:
* e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
index 395baff6f..1895c50b8 100644
--- a/src/lib/core/exception.cc
+++ b/src/lib/core/exception.cc
@@ -85,13 +85,7 @@ exception_get_int(struct error *e, const struct method_info *method)
static OutOfMemory out_of_memory(__FILE__, __LINE__,
sizeof(OutOfMemory), "malloc", "exception");
-static const struct method_info exception_methods[] = {
- make_method(&type_Exception, "message", &Exception::get_errmsg),
- make_method(&type_Exception, "log", &Exception::log),
- METHODS_SENTINEL
-};
-const struct type_info type_Exception = make_type("Exception", NULL,
- exception_methods);
+const struct type_info type_Exception = make_type("Exception", NULL);
void *
Exception::operator new(size_t size)
diff --git a/src/lua/error.lua b/src/lua/error.lua
index a5d1d2a43..c339de7d4 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -1,6 +1,10 @@
-- error.lua (internal file)
local ffi = require('ffi')
+local msgpack = require('msgpack')
+
+local mp_decode = msgpack.decode_unchecked
+
ffi.cdef[[
struct type_info;
@@ -41,11 +45,6 @@ struct error {
struct error *_effect;
};
-char *
-exception_get_string(struct error *e, const struct method_info *method);
-int
-exception_get_int(struct error *e, const struct method_info *method);
-
int
error_set_prev(struct error *e, struct error *prev);
@@ -57,45 +56,10 @@ error_ref(struct error *e);
void
error_unref(struct error *e);
-]]
-
-local REFLECTION_CACHE = {}
-local function reflection_enumerate(err)
- local key = tostring(err._type)
- local result = REFLECTION_CACHE[key]
- if result ~= nil then
- return result
- end
- result = {}
- -- See type_foreach_method() in reflection.h
- local t = err._type
- while t ~= nil do
- local m = t.methods
- while m.name ~= nil do
- result[ffi.string(m.name)] = m
- m = m + 1
- end
- t = t.parent
- end
- REFLECTION_CACHE[key] = result
- return result
-end
-
-local function reflection_get(err, method)
- if method.nargs ~= 0 then
- return nil -- NYI
- end
- if method.rtype == ffi.C.CTYPE_INT then
- return tonumber(ffi.C.exception_get_int(err, method))
- elseif method.rtype == ffi.C.CTYPE_CONST_CHAR_PTR then
- local str = ffi.C.exception_get_string(err, method)
- if str == nil then
- return nil
- end
- return ffi.string(str)
- end
-end
+const struct error_field *
+error_field_find(const struct error *e, const char *name);
+]]
local function error_base_type(err)
return ffi.string(err._type.name)
@@ -175,11 +139,11 @@ local function error_unpack(err)
for key, getter in pairs(error_fields) do
result[key] = getter(err)
end
- for key, getter in pairs(reflection_enumerate(err)) do
- local value = reflection_get(err, getter)
- if value ~= nil then
- result[key] = value
- end
+ local payload = err._payload
+ local fields = payload._fields
+ for i = 0, payload._count - 1 do
+ local f = fields[i]
+ result[ffi.string(f._name)] = mp_decode(f._data)
end
return result
end
@@ -216,12 +180,9 @@ local function error_index(err, key)
if getter ~= nil then
return getter(err)
end
- getter = reflection_enumerate(err)[key]
- if getter ~= nil and getter.nargs == 0 then
- local val = reflection_get(err, getter)
- if val ~= nil then
- return val
- end
+ local f = ffi.C.error_field_find(err, key)
+ if f ~= nil then
+ return mp_decode(f._data)
end
return error_methods[key]
end
diff --git a/src/lua/init.lua b/src/lua/init.lua
index 9e3c813c3..f808a5c32 100644
--- a/src/lua/init.lua
+++ b/src/lua/init.lua
@@ -3,37 +3,13 @@
local ffi = require('ffi')
ffi.cdef[[
-struct type_info;
struct method_info;
-enum ctype {
- CTYPE_VOID = 0,
- CTYPE_INT,
- CTYPE_CONST_CHAR_PTR
-};
-
struct type_info {
const char *name;
const struct type_info *parent;
const struct method_info *methods;
};
-
-enum { METHOD_ARG_MAX = 8 };
-
-struct method_info {
- const struct type_info *owner;
- const char *name;
- enum ctype rtype;
- enum ctype atype[METHOD_ARG_MAX];
- int nargs;
- bool isconst;
-
- union {
- /* Add extra space to get proper struct size in C */
- void *_spacer[2];
- };
-};
-
double
tarantool_uptime(void);
typedef int32_t pid_t;
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* [Tarantool-patches] [PATCH 7/9] luatest: copy config in cluster:build_server()
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
` (5 preceding siblings ...)
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 6/9] error: use error_payload in Lua Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 8/9] luatest: add new helpers for 'server' object Vladislav Shpilevoy via Tarantool-patches
` (2 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
It takes box.cfg config as an argument. And changes the argument
by adding a new key 'command'. If the caller wants to pass the
same box.cfg or slightly modified to several build_server() calls,
it won't work - all options will be the same on all instances.
For example:
local cfg = {...}
cfg.replication = {url1}
cluster:build_server(cfg)
cfg.replication = {url2}
cluster:build_server(cfg)
It will not work. Both servers will get the same 'command' and the
same 'replication'.
---
test/luatest_helpers/cluster.lua | 1 +
1 file changed, 1 insertion(+)
diff --git a/test/luatest_helpers/cluster.lua b/test/luatest_helpers/cluster.lua
index 01291b43c..43e3479f7 100644
--- a/test/luatest_helpers/cluster.lua
+++ b/test/luatest_helpers/cluster.lua
@@ -93,6 +93,7 @@ end
function Cluster:build_server(server_config, instance_file)
instance_file = instance_file or 'default.lua'
+ server_config = table.deepcopy(server_config)
server_config.command = fio.pathjoin(root, 'test/instances/', instance_file)
assert(server_config.alias, 'Either replicaset.alias or server.alias must be given')
local server = Server:new(server_config)
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* [Tarantool-patches] [PATCH 8/9] luatest: add new helpers for 'server' object
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
` (6 preceding siblings ...)
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 7/9] luatest: copy config in cluster:build_server() Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-08 15:16 ` Serge Petrenko via Tarantool-patches
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details Vladislav Shpilevoy via Tarantool-patches
2021-11-08 14:25 ` [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladimir Davydov via Tarantool-patches
9 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
-- Wait until the instance becomes an elected leader.
server:wait_election_leader()
-- Wait until an election leader is found.
server:wait_election_leader_found()
-- Get numeric ID of the instance like in box.info.id.
server:instance_id()
-- Get UUID of the instance like in box.info.uuid.
server:instance_uuid()
These are going to be used in a new test in a next commit.
---
test/luatest_helpers/server.lua | 64 ++++++++++++++++++++++++++-------
1 file changed, 52 insertions(+), 12 deletions(-)
diff --git a/test/luatest_helpers/server.lua b/test/luatest_helpers/server.lua
index b6d6f1400..9959bca6c 100644
--- a/test/luatest_helpers/server.lua
+++ b/test/luatest_helpers/server.lua
@@ -75,28 +75,45 @@ function Server:build_env()
return res
end
-function Server:wait_for_readiness()
- local alias = self.alias
- local id = self.id
- local pid = self.process.pid
+local function wait_cond(cond_name, server, func, ...)
+ local alias = server.alias
+ local id = server.id
+ local pid = server.process.pid
local deadline = clock.time() + WAIT_TIMEOUT
while true do
- local ok, is_ready = pcall(function()
- self:connect_net_box()
- return self.net_box:eval('return _G.ready') == true
- end)
- if ok and is_ready then
- break
+ if func(...) then
+ return
end
if clock.time() > deadline then
- error(('Starting of server %s-%s (PID %d) was timed out'):format(
- alias, id, pid))
+ error(('Waiting for "%s" on server %s-%s (PID %d) timed out')
+ :format(alias, id, pid))
end
fiber.sleep(WAIT_DELAY)
end
end
+function Server:wait_for_readiness()
+ return wait_cond('readiness', self, function()
+ local ok, is_ready = pcall(function()
+ self:connect_net_box()
+ return self.net_box:eval('return _G.ready') == true
+ end)
+ return ok and is_ready
+ end)
+end
+
+function Server:wait_election_leader()
+ return wait_cond('election leader', self, self.exec, self, function()
+ return box.info.election.state == 'leader'
+ end)
+end
+
+function Server:wait_election_leader_found()
+ return wait_cond('election leader is found', self, self.exec, self,
+ function() return box.info.election.leader ~= 0 end)
+end
+
-- Unlike the original luatest.Server function it waits for
-- starting the server.
function Server:start(opts)
@@ -116,6 +133,29 @@ function Server:start(opts)
end
end
+function Server:instance_id()
+ -- Cache the value when found it first time.
+ if self.instance_id_value then
+ return self.instance_id_value
+ end
+ local id = self:exec(function() return box.info.id end)
+ -- But do not cache 0 - it is an anon instance, its ID might change.
+ if id ~= 0 then
+ self.instance_id_value = id
+ end
+ return id
+end
+
+function Server:instance_uuid()
+ -- Cache the value when found it first time.
+ if self.instance_uuid_value then
+ return self.instance_uuid_value
+ end
+ local uuid = self:exec(function() return box.info.uuid end)
+ self.instance_uuid_value = uuid
+ return uuid
+end
+
-- TODO: Add the 'wait_for_readiness' parameter for the restart()
-- method.
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 8/9] luatest: add new helpers for 'server' object
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 8/9] luatest: add new helpers for 'server' object Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-08 15:16 ` Serge Petrenko via Tarantool-patches
2021-11-11 23:51 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-08 15:16 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, vdavydov
06.11.2021 02:56, Vladislav Shpilevoy пишет:
> -- Wait until the instance becomes an elected leader.
> server:wait_election_leader()
>
> -- Wait until an election leader is found.
> server:wait_election_leader_found()
>
> -- Get numeric ID of the instance like in box.info.id.
> server:instance_id()
>
> -- Get UUID of the instance like in box.info.uuid.
> server:instance_uuid()
>
> These are going to be used in a new test in a next commit.
> ---
> test/luatest_helpers/server.lua | 64 ++++++++++++++++++++++++++-------
> 1 file changed, 52 insertions(+), 12 deletions(-)
Hi! Thanks for working on this!
Please, find a couple of small comments below.
>
> diff --git a/test/luatest_helpers/server.lua b/test/luatest_helpers/server.lua
> index b6d6f1400..9959bca6c 100644
> --- a/test/luatest_helpers/server.lua
> +++ b/test/luatest_helpers/server.lua
> @@ -75,28 +75,45 @@ function Server:build_env()
> return res
> end
>
> -function Server:wait_for_readiness()
> - local alias = self.alias
> - local id = self.id
> - local pid = self.process.pid
> +local function wait_cond(cond_name, server, func, ...)
> + local alias = server.alias
> + local id = server.id
> + local pid = server.process.pid
>
> local deadline = clock.time() + WAIT_TIMEOUT
> while true do
> - local ok, is_ready = pcall(function()
> - self:connect_net_box()
> - return self.net_box:eval('return _G.ready') == true
> - end)
> - if ok and is_ready then
> - break
> + if func(...) then
> + return
> end
> if clock.time() > deadline then
> - error(('Starting of server %s-%s (PID %d) was timed out'):format(
> - alias, id, pid))
> + error(('Waiting for "%s" on server %s-%s (PID %d) timed out')
> + :format(alias, id, pid))
1. Looks like you forgot to use "cond_name" in the error message.
> end
> fiber.sleep(WAIT_DELAY)
> end
> end
>
> +function Server:wait_for_readiness()
> + return wait_cond('readiness', self, function()
> + local ok, is_ready = pcall(function()
> + self:connect_net_box()
> + return self.net_box:eval('return _G.ready') == true
> + end)
> + return ok and is_ready
> + end)
> +end
> +
> +function Server:wait_election_leader()
> + return wait_cond('election leader', self, self.exec, self, function()
> + return box.info.election.state == 'leader'
> + end)
> +end
> +
> +function Server:wait_election_leader_found()
> + return wait_cond('election leader is found', self, self.exec, self,
> + function() return box.info.election.leader ~= 0 end)
> +end
> +
> -- Unlike the original luatest.Server function it waits for
> -- starting the server.
> function Server:start(opts)
> @@ -116,6 +133,29 @@ function Server:start(opts)
> end
> end
>
> +function Server:instance_id()
> + -- Cache the value when found it first time.
> + if self.instance_id_value then
> + return self.instance_id_value
> + end
> + local id = self:exec(function() return box.info.id end)
> + -- But do not cache 0 - it is an anon instance, its ID might change.
> + if id ~= 0 then
> + self.instance_id_value = id
> + end
> + return id
> +end
2. Let's reset cached instance id in Server:cleanup().
One might want to call Server:cleanup() and then restart the server
making it receive a new instance id.
> +
> +function Server:instance_uuid()
> + -- Cache the value when found it first time.
> + if self.instance_uuid_value then
> + return self.instance_uuid_value
> + end
> + local uuid = self:exec(function() return box.info.uuid end)
> + self.instance_uuid_value = uuid
> + return uuid
> +end
> +
> -- TODO: Add the 'wait_for_readiness' parameter for the restart()
> -- method.
>
--
Serge Petrenko
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 8/9] luatest: add new helpers for 'server' object
2021-11-08 15:16 ` Serge Petrenko via Tarantool-patches
@ 2021-11-11 23:51 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:51 UTC (permalink / raw)
To: Serge Petrenko, tarantool-patches, vdavydov
Thanks for the review!
>> diff --git a/test/luatest_helpers/server.lua b/test/luatest_helpers/server.lua
>> index b6d6f1400..9959bca6c 100644
>> --- a/test/luatest_helpers/server.lua
>> +++ b/test/luatest_helpers/server.lua
>> @@ -75,28 +75,45 @@ function Server:build_env()
>> return res
>> end
>> -function Server:wait_for_readiness()
>> - local alias = self.alias
>> - local id = self.id
>> - local pid = self.process.pid
>> +local function wait_cond(cond_name, server, func, ...)
>> + local alias = server.alias
>> + local id = server.id
>> + local pid = server.process.pid
>> local deadline = clock.time() + WAIT_TIMEOUT
>> while true do
>> - local ok, is_ready = pcall(function()
>> - self:connect_net_box()
>> - return self.net_box:eval('return _G.ready') == true
>> - end)
>> - if ok and is_ready then
>> - break
>> + if func(...) then
>> + return
>> end
>> if clock.time() > deadline then
>> - error(('Starting of server %s-%s (PID %d) was timed out'):format(
>> - alias, id, pid))
>> + error(('Waiting for "%s" on server %s-%s (PID %d) timed out')
>> + :format(alias, id, pid))
>
> 1. Looks like you forgot to use "cond_name" in the error message.
====================
Yes indeed, thanks for noticing:
error(('Waiting for "%s" on server %s-%s (PID %d) timed out')
- :format(alias, id, pid))
+ :format(cond_name, alias, id, pid))
end
====================
>> @@ -116,6 +133,29 @@ function Server:start(opts)
>> end
>> end
>> +function Server:instance_id()
>> + -- Cache the value when found it first time.
>> + if self.instance_id_value then
>> + return self.instance_id_value
>> + end
>> + local id = self:exec(function() return box.info.id end)
>> + -- But do not cache 0 - it is an anon instance, its ID might change.
>> + if id ~= 0 then
>> + self.instance_id_value = id
>> + end
>> + return id
>> +end
>
> 2. Let's reset cached instance id in Server:cleanup().
> One might want to call Server:cleanup() and then restart the server
> making it receive a new instance id.
Thanks, done:
====================
@@ -186,6 +186,8 @@ function Server:cleanup()
for _, pattern in ipairs(DEFAULT_CHECKPOINT_PATTERNS) do
fio.rmtree(('%s/%s'):format(self.workdir, pattern))
end
+ self.instance_id_value = nil
+ self.instance_uuid_value = nil
end
====================
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
` (7 preceding siblings ...)
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 8/9] luatest: add new helpers for 'server' object Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-05 23:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-06 19:30 ` Cyrill Gorcunov via Tarantool-patches
2021-11-08 15:18 ` Serge Petrenko via Tarantool-patches
2021-11-08 14:25 ` [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladimir Davydov via Tarantool-patches
9 siblings, 2 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-05 23:56 UTC (permalink / raw)
To: tarantool-patches, sergepetrenko, vdavydov
ER_READONLY used not to have any details about the exact reason
why the instance is read-only. The patch changes that by adding
new fields into the error which explain why the error happened and
even help to avoid it for next requests.
Now from the error object can tell whether it was raised because
of box.cfg.read_only = true, or the instance is an orphan, or it
has election enabled but is not a leader, or the transaction limbo
belongs to another instance.
The alternative to ClientError alteration via error_payload was
not to touch struct error and introduce a new error type
specifically for ER_READONLY via a new C++ class like
ReadOnlyError. But it had drawbacks:
- There may be clients who expect ER_READONLY to have ClientError
type. For instance, they check err.code == ER_READONLY only for
err.type == 'ClientError';
- Having to introduce a new C++ class each time when want to add a
new field into an error has to end. Rather sooner than later.
Closes #5568
@TarantoolBot document
Title: box.error.READONLY new attributes
Users could see the error code as `box.error.READONLY` in `.code`
field of an error object. The error didn't have any other
attributes except common ones like 'type'.
Now from the `box.error.READONLY` error users can see why it
happened. The reasons can be the following:
* The instance has `box.cfg.read_only = true`. Then the `READONLY`
error has at least these fields:
```
tarantool> err:unpack()
---
- state: read_only
reason: state
code: 7
type: ClientError
...
```
* The instance is an orphan. It enters that state if number of
connected replicas is < `box.cfg.replication_connect_quorum`. Then
`READONLY` error has at least these fields:
```
tarantool> err:unpack()
---
- state: orphan
reason: state
code: 7
type: ClientError
...
```
* The synchro queue has an owner which is not the given instance.
It usually happens if synchronous replication is used and there is
another instance who called `box.ctl.promote()`. Then `READONLY`
error has at least these fields:
```
tarantool> err:unpack()
---
- queue_owner_id: <box.info.id of the queue owner>
queue_owner_uuid: <box.info.uuid of the queue owner>
reason: synchro
term: <box.info.election.term of this instance>
code: 7
type: ClientError
...
```
Note than `queue_owner_uuid` sometimes might be not present.
* The instance has `box.cfg.election_mode` not `off` and it is not
a leader. Then `READONLY` error has at least these fields:
```
tarantool> err:unpack()
---
- state: <box.info.election.state of this instance>
leader_id: <box.info.id of the leader>
leader_uuid: <box.info.uuid of the leader>
reason: election
term: <box.info.election.term of this instance>
code: 7
type: ClientError
...
```
`leader_id` and `leader_uuid` might be absent if the leader is not
known. For example, an election is still in progress. Note, than
`leader_uuid` sometimes might be not present even if `leader_id`
is.
If multiple reasons are true at the same time, then only one is
returned in order reversed from how they are listed above. IOW,
election, synchro, box.cfg.read_only, orphan.
---
There are 2 test groups because the 'unsafe' one sometimes crashes if try to
make a cleanup. Instances of the second test group can only be recreated.
.../unreleased/gh-5568-readonly-reason.md | 24 ++
src/box/box.cc | 56 +++-
.../gh_5568_read_only_reason_test.lua | 287 ++++++++++++++++++
3 files changed, 359 insertions(+), 8 deletions(-)
create mode 100644 changelogs/unreleased/gh-5568-readonly-reason.md
create mode 100644 test/replication-luatest/gh_5568_read_only_reason_test.lua
diff --git a/changelogs/unreleased/gh-5568-readonly-reason.md b/changelogs/unreleased/gh-5568-readonly-reason.md
new file mode 100644
index 000000000..f90b53bdf
--- /dev/null
+++ b/changelogs/unreleased/gh-5568-readonly-reason.md
@@ -0,0 +1,24 @@
+## feature/core
+
+* Error objects with the code `box.error.READONLY` now have additional fields
+ explaining why the error happened (gh-5568).
+
+ For instance, if the error was due to `box.cfg{read_only = true}`, then the
+ error will have fields:
+ ```
+ {
+ reason = 'state',
+ state = 'read_only'
+ }
+ ```
+ Or if it was due to not being a leader with `box.cfg{election_mode = ...}` set
+ not to `off`, then:
+ ```
+ {
+ state: <box.info.election.state of this instance>,
+ leader_id: <box.info.id of the leader>,
+ leader_uuid: <box.info.uuid of the leader>,
+ reason: election,
+ term: <box.info.election.term of this instance>
+ }
+ ```
\ No newline at end of file
diff --git a/src/box/box.cc b/src/box/box.cc
index 1ed1ce3f8..236c02533 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -177,16 +177,56 @@ box_update_ro_summary(void)
static int
box_check_writable(void)
{
- if (is_ro_summary) {
+ if (!is_ro_summary)
+ return 0;
+ struct error *e = diag_set(ClientError, ER_READONLY);
+ struct raft *raft = box_raft();
+ /*
+ * In case of multiple reasons at the same time only one is reported.
+ * But the order is important. For example, if the instance has election
+ * enabled, for the client it is better to see that it is a 'follower'
+ * and who is the leader than just see cfg 'read_only' is true.
+ */
+ if (raft_is_ro(raft)) {
+ error_field_set_str(e, "reason", "election");
+ error_field_set_str(e, "state", raft_state_str(raft->state));
+ error_field_set_uint(e, "term", raft->volatile_term);
+ uint32_t id = raft->leader;
+ if (id != REPLICA_ID_NIL) {
+ error_field_set_uint(e, "leader_id", id);
+ struct replica *r = replica_by_id(id);
+ /*
+ * XXX: when the leader is dropped from _cluster, it
+ * is not reported to Raft.
+ */
+ if (r != NULL) {
+ error_field_set_uuid(e, "leader_uuid",
+ &r->uuid);
+ }
+ }
+ } else if (txn_limbo_is_ro(&txn_limbo)) {
+ error_field_set_str(e, "reason", "synchro");
+ uint32_t id = txn_limbo.owner_id;
+ error_field_set_uint(e, "queue_owner_id", id);
+ error_field_set_uint(e, "term", raft->volatile_term);
+ struct replica *r = replica_by_id(id);
/*
- * XXX: return a special error when the node is not a leader to
- * reroute to the leader node.
+ * XXX: when an instance is deleted from _cluster, its limbo's
+ * ownership is not cleared.
*/
- diag_set(ClientError, ER_READONLY);
- diag_log();
- return -1;
- }
- return 0;
+ if (r != NULL)
+ error_field_set_uuid(e, "queue_owner_uuid", &r->uuid);
+ } else {
+ error_field_set_str(e, "reason", "state");
+ if (is_ro)
+ error_field_set_str(e, "state", "read_only");
+ else if (is_orphan)
+ error_field_set_str(e, "state", "orphan");
+ else
+ assert(false);
+ }
+ diag_log();
+ return -1;
}
static void
diff --git a/test/replication-luatest/gh_5568_read_only_reason_test.lua b/test/replication-luatest/gh_5568_read_only_reason_test.lua
new file mode 100644
index 000000000..d4f7ff957
--- /dev/null
+++ b/test/replication-luatest/gh_5568_read_only_reason_test.lua
@@ -0,0 +1,287 @@
+local t = require('luatest')
+local cluster = require('test.luatest_helpers.cluster')
+local helpers = require('test.luatest_helpers')
+
+--
+-- gh-5568: ER_READONLY needs to carry additional info explaining the reason
+-- why the error happened. It should help the clients to do better error logging
+-- and to use the provided info to find the actual leader if the reason is in
+-- being not a leader.
+--
+
+--
+-- Make functions which will create and destroy a cluster. Usage of the closures
+-- is a workaround for luatest hooks not accepting their group as a parameter.
+--
+local function make_create_cluster(g) return function(param)
+ g.cluster = cluster:new({})
+ local master_uri = helpers.instance_uri('master')
+ local replica_uri = helpers.instance_uri('replica')
+ local replication = {master_uri, replica_uri}
+ local box_cfg = {
+ listen = master_uri,
+ replication = replication,
+ -- To speed up new term when try to elect a new leader.
+ replication_timeout = 0.1,
+ }
+ g.master = g.cluster:build_server({alias = 'master', box_cfg = box_cfg})
+
+ box_cfg.listen = replica_uri
+ g.replica = g.cluster:build_server({alias = 'replica', box_cfg = box_cfg})
+
+ g.cluster:add_server(g.master)
+ g.cluster:add_server(g.replica)
+ g.cluster:start()
+end end
+
+local function make_destroy_cluster(g) return function()
+ g.cluster:drop()
+end end
+
+--
+-- This group's test cases leave instances in a valid state after which they can
+-- be reused.
+--
+local g = t.group('gh-5568-read-only-reason1')
+
+g.before_all(make_create_cluster(g))
+g.after_all(make_destroy_cluster(g))
+
+--
+-- Read-only because of box.cfg{read_only = true}.
+--
+g.test_read_only_reason_cfg = function(g)
+ local ok, err = g.master:exec(function()
+ box.cfg{read_only = true}
+ local ok, err = pcall(box.schema.create_space, 'test')
+ return ok, err:unpack()
+ end)
+ t.assert(not ok, 'fail ddl')
+ t.assert_covers(err, {
+ reason = 'state',
+ state = 'read_only',
+ code = box.error.READONLY,
+ type = 'ClientError'
+ }, 'reason is read_only')
+
+ -- Cleanup.
+ g.master:exec(function()
+ box.cfg{read_only = false}
+ end)
+end
+
+--
+-- Read-only because is an orphan.
+--
+g.test_read_only_reason_orphan = function(g)
+ local fake_uri = helpers.instance_uri('fake')
+ local old_timeout, ok, err = g.master:exec(function(fake_uri)
+ -- Make connect-quorum impossible to satisfy using a fake instance.
+ local old_timeout = box.cfg.replication_connect_timeout
+ local repl = table.copy(box.cfg.replication)
+ table.insert(repl, fake_uri)
+ box.cfg{
+ replication = repl,
+ replication_connect_timeout = 0.001,
+ }
+ local ok, err = pcall(box.schema.create_space, 'test')
+ return old_timeout, ok, err:unpack()
+ end, {fake_uri})
+ t.assert(not ok, 'fail ddl')
+ t.assert_covers(err, {
+ reason = 'state',
+ state = 'orphan',
+ code = box.error.READONLY,
+ type = 'ClientError'
+ }, 'reason is orphan')
+
+ -- Cleanup.
+ g.master:exec(function(old_timeout)
+ local repl = table.copy(box.cfg.replication)
+ table.remove(repl)
+ box.cfg{
+ replication = repl,
+ replication_connect_timeout = old_timeout,
+ }
+ end, {old_timeout})
+end
+
+--
+-- Read-only because is not a leader. Does not know the leader.
+--
+g.test_read_only_reason_election_no_leader = function(g)
+ local ok, err = g.master:exec(function()
+ box.cfg{election_mode = 'voter'}
+ local ok, err = pcall(box.schema.create_space, 'test')
+ return ok, err:unpack()
+ end)
+ t.assert(not ok, 'fail ddl')
+ t.assert(err.term, 'has term')
+ t.assert_covers(err, {
+ reason = 'election',
+ state = 'follower',
+ code = box.error.READONLY,
+ type = 'ClientError'
+ }, 'reason is election, no leader')
+ t.assert(not err.leader_id, 'no leader id')
+ t.assert(not err.leader_uuid, 'no leader uuid')
+
+ -- Cleanup.
+ g.master:exec(function()
+ box.cfg{election_mode = 'off'}
+ end)
+end
+
+--
+-- Read-only because is not a leader. Knows the leader.
+--
+g.test_read_only_reason_election_has_leader = function(g)
+ g.master:exec(function()
+ box.cfg{election_mode = 'candidate'}
+ end)
+ g.master:wait_election_leader()
+ g.replica:wait_election_leader_found()
+ local ok, err = g.replica:exec(function()
+ box.cfg{election_mode = 'voter'}
+ local ok, err = pcall(box.schema.create_space, 'test')
+ return ok, err:unpack()
+ end)
+ t.assert(not ok, 'fail ddl')
+ t.assert(err.term, 'has term')
+ t.assert_covers(err, {
+ reason = 'election',
+ state = 'follower',
+ leader_id = g.master:instance_id(),
+ leader_uuid = g.master:instance_uuid(),
+ code = box.error.READONLY,
+ type = 'ClientError'
+ }, 'reason is election, has leader')
+
+ -- Cleanup.
+ g.master:exec(function()
+ box.cfg{election_mode = 'off'}
+ box.ctl.demote()
+ end)
+ g.replica:exec(function()
+ box.cfg{election_mode = 'off'}
+ end)
+end
+
+--
+-- Read-only because does not own the limbo. Knows the owner.
+--
+g.test_read_only_reason_synchro = function(g)
+ g.master:exec(function()
+ box.schema.create_space('test', {is_sync = true}):create_index('pk')
+ box.ctl.promote()
+ end)
+
+ t.helpers.retrying({}, function()
+ assert(g.replica:exec(function()
+ return box.info.synchro.queue.owner ~= 0
+ end))
+ end)
+
+ local ok, err = g.replica:exec(function()
+ local ok, err = pcall(box.schema.create_space, 'test2')
+ return ok, err:unpack()
+ end)
+ t.assert(not ok, 'fail ddl')
+ t.assert(err.term, 'has term')
+ t.assert_covers(err, {
+ reason = 'synchro',
+ queue_owner_id = g.master:instance_id(),
+ queue_owner_uuid = g.master:instance_uuid(),
+ code = box.error.READONLY,
+ type = 'ClientError'
+ }, 'reason is synchro, has owner')
+
+ -- Cleanup.
+ g.master:exec(function()
+ box.space.test:drop()
+ box.ctl.demote()
+ end)
+end
+
+--
+-- This group's test cases leave instances in an invalid state after which they
+-- should be re-created.
+--
+g = t.group('gh-5568-read-only-reason2')
+
+g.before_each(make_create_cluster(g))
+g.after_each(make_destroy_cluster(g))
+
+--
+-- Read-only because is not a leader. Knows the leader, but not its UUID.
+--
+g.test_read_only_reason_election_has_leader_no_uuid = function(g)
+ g.replica:exec(function()
+ box.cfg{election_mode = 'voter'}
+ end)
+ g.master:exec(function()
+ box.cfg{election_mode = 'candidate'}
+ end)
+ g.master:wait_election_leader()
+ g.replica:wait_election_leader_found()
+ local leader_id = g.master:instance_id()
+
+ g.master:exec(function()
+ box.space._cluster:run_triggers(false)
+ box.space._cluster:delete{box.info.id}
+ end)
+
+ t.helpers.retrying({}, function()
+ assert(g.replica:exec(function(leader_id)
+ return box.space._cluster:get{leader_id} == nil
+ end, {leader_id}))
+ end)
+
+ local ok, err = g.replica:exec(function()
+ local ok, err = pcall(box.schema.create_space, 'test')
+ return ok, err:unpack()
+ end)
+ t.assert(not ok, 'fail ddl')
+ t.assert(err.term, 'has term')
+ t.assert(not err.leader_uuid, 'has no leader uuid')
+ t.assert_covers(err, {
+ reason = 'election',
+ state = 'follower',
+ leader_id = leader_id,
+ code = box.error.READONLY,
+ type = 'ClientError'
+ }, 'reason is election, has leader but no uuid')
+end
+
+--
+-- Read-only because does not own the limbo. Knows the owner, but now its UUID.
+--
+g.test_read_only_reason_synchro_no_uuid = function(g)
+ g.master:exec(function()
+ box.ctl.promote()
+ box.space._cluster:run_triggers(false)
+ box.space._cluster:delete{box.info.id}
+ end)
+
+ local leader_id = g.master:instance_id()
+ t.helpers.retrying({}, function()
+ assert(g.replica:exec(function(leader_id)
+ return box.info.synchro.queue.owner ~= 0 and
+ box.space._cluster:get{leader_id} == nil
+ end, {leader_id}))
+ end)
+
+ local ok, err = g.replica:exec(function()
+ local ok, err = pcall(box.schema.create_space, 'test')
+ return ok, err:unpack()
+ end)
+ t.assert(not ok, 'fail ddl')
+ t.assert(err.term, 'has term')
+ t.assert(not err.queue_owner_uuid)
+ t.assert_covers(err, {
+ reason = 'synchro',
+ queue_owner_id = leader_id,
+ code = box.error.READONLY,
+ type = 'ClientError'
+ }, 'reason is synchro, has owner but no uuid')
+end
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-06 19:30 ` Cyrill Gorcunov via Tarantool-patches
2021-11-07 16:45 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-08 15:18 ` Serge Petrenko via Tarantool-patches
1 sibling, 1 reply; 24+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-11-06 19:30 UTC (permalink / raw)
To: Vladislav Shpilevoy via Tarantool-patches
On Sat, Nov 06, 2021 at 12:56:40AM +0100, Vladislav Shpilevoy via Tarantool-patches wrote:
> + if (raft_is_ro(raft)) {
> + error_field_set_str(e, "reason", "election");
> + error_field_set_str(e, "state", raft_state_str(raft->state));
> + error_field_set_uint(e, "term", raft->volatile_term);
> + uint32_t id = raft->leader;
> + if (id != REPLICA_ID_NIL) {
> + error_field_set_uint(e, "leader_id", id);
> + struct replica *r = replica_by_id(id);
Why do we need an additional variable here instead of raft->leader?
To shrink code left?
> + } else if (txn_limbo_is_ro(&txn_limbo)) {
> + error_field_set_str(e, "reason", "synchro");
> + uint32_t id = txn_limbo.owner_id;
> + error_field_set_uint(e, "queue_owner_id", id);
> + error_field_set_uint(e, "term", raft->volatile_term);
> + struct replica *r = replica_by_id(id);
And here too. Just wondering.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details
2021-11-06 19:30 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-11-07 16:45 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-07 20:19 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-07 16:45 UTC (permalink / raw)
To: Cyrill Gorcunov, Vladislav Shpilevoy via Tarantool-patches
On 06.11.2021 20:30, Cyrill Gorcunov via Tarantool-patches wrote:
> On Sat, Nov 06, 2021 at 12:56:40AM +0100, Vladislav Shpilevoy via Tarantool-patches wrote:
>> + if (raft_is_ro(raft)) {
>> + error_field_set_str(e, "reason", "election");
>> + error_field_set_str(e, "state", raft_state_str(raft->state));
>> + error_field_set_uint(e, "term", raft->volatile_term);
>> + uint32_t id = raft->leader;
>> + if (id != REPLICA_ID_NIL) {
>> + error_field_set_uint(e, "leader_id", id);
>> + struct replica *r = replica_by_id(id);
>
> Why do we need an additional variable here instead of raft->leader?
> To shrink code left?
>
>> + } else if (txn_limbo_is_ro(&txn_limbo)) {
>> + error_field_set_str(e, "reason", "synchro");
>> + uint32_t id = txn_limbo.owner_id;
>> + error_field_set_uint(e, "queue_owner_id", id);
>> + error_field_set_uint(e, "term", raft->volatile_term);
>> + struct replica *r = replica_by_id(id);
>
> And here too. Just wondering.
I like to avoid '->' when it is used for the same member
more than once.
Also it shrinks the code a bit, yes.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details Vladislav Shpilevoy via Tarantool-patches
2021-11-06 19:30 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-11-08 15:18 ` Serge Petrenko via Tarantool-patches
2021-11-11 23:52 ` Vladislav Shpilevoy via Tarantool-patches
1 sibling, 1 reply; 24+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-11-08 15:18 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, vdavydov
06.11.2021 02:56, Vladislav Shpilevoy пишет:
> ER_READONLY used not to have any details about the exact reason
> why the instance is read-only. The patch changes that by adding
> new fields into the error which explain why the error happened and
> even help to avoid it for next requests.
>
> Now from the error object can tell whether it was raised because
> of box.cfg.read_only = true, or the instance is an orphan, or it
> has election enabled but is not a leader, or the transaction limbo
> belongs to another instance.
>
> The alternative to ClientError alteration via error_payload was
> not to touch struct error and introduce a new error type
> specifically for ER_READONLY via a new C++ class like
> ReadOnlyError. But it had drawbacks:
>
> - There may be clients who expect ER_READONLY to have ClientError
> type. For instance, they check err.code == ER_READONLY only for
> err.type == 'ClientError';
>
> - Having to introduce a new C++ class each time when want to add a
> new field into an error has to end. Rather sooner than later.
>
> Closes #5568
Thanks for the patch!
Looks nice, generally, with 3 minor comments.
>
> @TarantoolBot document
> Title: box.error.READONLY new attributes
>
> Users could see the error code as `box.error.READONLY` in `.code`
> field of an error object. The error didn't have any other
> attributes except common ones like 'type'.
>
> Now from the `box.error.READONLY` error users can see why it
> happened. The reasons can be the following:
>
> * The instance has `box.cfg.read_only = true`. Then the `READONLY`
> error has at least these fields:
> ```
> tarantool> err:unpack()
> ---
> - state: read_only
> reason: state
> code: 7
> type: ClientError
> ...
> ```
>
> * The instance is an orphan. It enters that state if number of
> connected replicas is < `box.cfg.replication_connect_quorum`. Then
> `READONLY` error has at least these fields:
> ```
> tarantool> err:unpack()
> ---
> - state: orphan
> reason: state
> code: 7
> type: ClientError
> ...
> ```
>
> * The synchro queue has an owner which is not the given instance.
> It usually happens if synchronous replication is used and there is
> another instance who called `box.ctl.promote()`. Then `READONLY`
> error has at least these fields:
> ```
> tarantool> err:unpack()
> ---
> - queue_owner_id: <box.info.id of the queue owner>
> queue_owner_uuid: <box.info.uuid of the queue owner>
> reason: synchro
> term: <box.info.election.term of this instance>
> code: 7
> type: ClientError
> ...
> ```
> Note than `queue_owner_uuid` sometimes might be not present.
>
> * The instance has `box.cfg.election_mode` not `off` and it is not
> a leader. Then `READONLY` error has at least these fields:
> ```
> tarantool> err:unpack()
> ---
> - state: <box.info.election.state of this instance>
> leader_id: <box.info.id of the leader>
> leader_uuid: <box.info.uuid of the leader>
> reason: election
> term: <box.info.election.term of this instance>
> code: 7
> type: ClientError
> ...
> ```
> `leader_id` and `leader_uuid` might be absent if the leader is not
> known. For example, an election is still in progress. Note, than
> `leader_uuid` sometimes might be not present even if `leader_id`
> is.
>
> If multiple reasons are true at the same time, then only one is
> returned in order reversed from how they are listed above. IOW,
> election, synchro, box.cfg.read_only, orphan.
> ---
> There are 2 test groups because the 'unsafe' one sometimes crashes if try to
> make a cleanup. Instances of the second test group can only be recreated.
>
> .../unreleased/gh-5568-readonly-reason.md | 24 ++
> src/box/box.cc | 56 +++-
> .../gh_5568_read_only_reason_test.lua | 287 ++++++++++++++++++
> 3 files changed, 359 insertions(+), 8 deletions(-)
> create mode 100644 changelogs/unreleased/gh-5568-readonly-reason.md
> create mode 100644 test/replication-luatest/gh_5568_read_only_reason_test.lua
>
> diff --git a/changelogs/unreleased/gh-5568-readonly-reason.md b/changelogs/unreleased/gh-5568-readonly-reason.md
> new file mode 100644
> index 000000000..f90b53bdf
> --- /dev/null
> +++ b/changelogs/unreleased/gh-5568-readonly-reason.md
> @@ -0,0 +1,24 @@
> +## feature/core
> +
> +* Error objects with the code `box.error.READONLY` now have additional fields
> + explaining why the error happened (gh-5568).
> +
> + For instance, if the error was due to `box.cfg{read_only = true}`, then the
> + error will have fields:
> + ```
> + {
> + reason = 'state',
> + state = 'read_only'
> + }
> + ```
> + Or if it was due to not being a leader with `box.cfg{election_mode = ...}` set
> + not to `off`, then:
> + ```
> + {
> + state: <box.info.election.state of this instance>,
> + leader_id: <box.info.id of the leader>,
> + leader_uuid: <box.info.uuid of the leader>,
> + reason: election,
> + term: <box.info.election.term of this instance>
> + }
> + ```
1. Let's duplicate the additional error information in the error message.
If it's hard to log every field, let's at least encode reason and state,
so that the user knows what's wrong by simply looking at the error
message.
> \ No newline at end of file
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1ed1ce3f8..236c02533 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -177,16 +177,56 @@ box_update_ro_summary(void)
> static int
> box_check_writable(void)
> {
> - if (is_ro_summary) {
> + if (!is_ro_summary)
> + return 0;
> + struct error *e = diag_set(ClientError, ER_READONLY);
> + struct raft *raft = box_raft();
> + /*
> + * In case of multiple reasons at the same time only one is reported.
> + * But the order is important. For example, if the instance has election
> + * enabled, for the client it is better to see that it is a 'follower'
> + * and who is the leader than just see cfg 'read_only' is true.
> + */
> + if (raft_is_ro(raft)) {
> + error_field_set_str(e, "reason", "election");
> + error_field_set_str(e, "state", raft_state_str(raft->state));
> + error_field_set_uint(e, "term", raft->volatile_term);
> + uint32_t id = raft->leader;
> + if (id != REPLICA_ID_NIL) {
> + error_field_set_uint(e, "leader_id", id);
> + struct replica *r = replica_by_id(id);
> + /*
> + * XXX: when the leader is dropped from _cluster, it
> + * is not reported to Raft.
> + */
> + if (r != NULL) {
> + error_field_set_uuid(e, "leader_uuid",
> + &r->uuid);
> + }
> + }
> + } else if (txn_limbo_is_ro(&txn_limbo)) {
> + error_field_set_str(e, "reason", "synchro");
> + uint32_t id = txn_limbo.owner_id;
> + error_field_set_uint(e, "queue_owner_id", id);
> + error_field_set_uint(e, "term", raft->volatile_term);
> + struct replica *r = replica_by_id(id);
> /*
> - * XXX: return a special error when the node is not a leader to
> - * reroute to the leader node.
> + * XXX: when an instance is deleted from _cluster, its limbo's
> + * ownership is not cleared.
> */
> - diag_set(ClientError, ER_READONLY);
> - diag_log();
> - return -1;
> - }
> - return 0;
> + if (r != NULL)
> + error_field_set_uuid(e, "queue_owner_uuid", &r->uuid);
> + } else {
> + error_field_set_str(e, "reason", "state");
> + if (is_ro)
> + error_field_set_str(e, "state", "read_only");
> + else if (is_orphan)
> + error_field_set_str(e, "state", "orphan");
> + else
> + assert(false);
> + }
> + diag_log();
> + return -1;
> }
>
> static void
> diff --git a/test/replication-luatest/gh_5568_read_only_reason_test.lua b/test/replication-luatest/gh_5568_read_only_reason_test.lua
> new file mode 100644
> index 000000000..d4f7ff957
> --- /dev/null
> +++ b/test/replication-luatest/gh_5568_read_only_reason_test.lua
> @@ -0,0 +1,287 @@
> +local t = require('luatest')
> +local cluster = require('test.luatest_helpers.cluster')
> +local helpers = require('test.luatest_helpers')
> +
> +--
> +-- gh-5568: ER_READONLY needs to carry additional info explaining the reason
> +-- why the error happened. It should help the clients to do better error logging
> +-- and to use the provided info to find the actual leader if the reason is in
> +-- being not a leader.
> +--
> +
> +--
> +-- Make functions which will create and destroy a cluster. Usage of the closures
> +-- is a workaround for luatest hooks not accepting their group as a parameter.
> +--
> +local function make_create_cluster(g) return function(param)
> + g.cluster = cluster:new({})
> + local master_uri = helpers.instance_uri('master')
> + local replica_uri = helpers.instance_uri('replica')
> + local replication = {master_uri, replica_uri}
> + local box_cfg = {
> + listen = master_uri,
> + replication = replication,
> + -- To speed up new term when try to elect a new leader.
> + replication_timeout = 0.1,
> + }
> + g.master = g.cluster:build_server({alias = 'master', box_cfg = box_cfg})
> +
> + box_cfg.listen = replica_uri
> + g.replica = g.cluster:build_server({alias = 'replica', box_cfg = box_cfg})
> +
> + g.cluster:add_server(g.master)
> + g.cluster:add_server(g.replica)
> + g.cluster:start()
> +end end
> +
> +local function make_destroy_cluster(g) return function()
> + g.cluster:drop()
> +end end
> +
> +--
> +-- This group's test cases leave instances in a valid state after which they can
> +-- be reused.
> +--
> +local g = t.group('gh-5568-read-only-reason1')
> +
> +g.before_all(make_create_cluster(g))
> +g.after_all(make_destroy_cluster(g))
> +
> +--
> +-- Read-only because of box.cfg{read_only = true}.
> +--
> +g.test_read_only_reason_cfg = function(g)
> + local ok, err = g.master:exec(function()
> + box.cfg{read_only = true}
> + local ok, err = pcall(box.schema.create_space, 'test')
> + return ok, err:unpack()
> + end)
> + t.assert(not ok, 'fail ddl')
> + t.assert_covers(err, {
> + reason = 'state',
> + state = 'read_only',
> + code = box.error.READONLY,
> + type = 'ClientError'
> + }, 'reason is read_only')
> +
> + -- Cleanup.
> + g.master:exec(function()
> + box.cfg{read_only = false}
> + end)
> +end
> +
> +--
> +-- Read-only because is an orphan.
> +--
> +g.test_read_only_reason_orphan = function(g)
> + local fake_uri = helpers.instance_uri('fake')
> + local old_timeout, ok, err = g.master:exec(function(fake_uri)
> + -- Make connect-quorum impossible to satisfy using a fake instance.
> + local old_timeout = box.cfg.replication_connect_timeout
> + local repl = table.copy(box.cfg.replication)
> + table.insert(repl, fake_uri)
> + box.cfg{
> + replication = repl,
> + replication_connect_timeout = 0.001,
> + }
> + local ok, err = pcall(box.schema.create_space, 'test')
> + return old_timeout, ok, err:unpack()
> + end, {fake_uri})
> + t.assert(not ok, 'fail ddl')
> + t.assert_covers(err, {
> + reason = 'state',
> + state = 'orphan',
> + code = box.error.READONLY,
> + type = 'ClientError'
> + }, 'reason is orphan')
> +
> + -- Cleanup.
> + g.master:exec(function(old_timeout)
> + local repl = table.copy(box.cfg.replication)
> + table.remove(repl)
> + box.cfg{
> + replication = repl,
> + replication_connect_timeout = old_timeout,
> + }
> + end, {old_timeout})
> +end
> +
> +--
> +-- Read-only because is not a leader. Does not know the leader.
> +--
> +g.test_read_only_reason_election_no_leader = function(g)
> + local ok, err = g.master:exec(function()
> + box.cfg{election_mode = 'voter'}
> + local ok, err = pcall(box.schema.create_space, 'test')
> + return ok, err:unpack()
> + end)
> + t.assert(not ok, 'fail ddl')
> + t.assert(err.term, 'has term')
> + t.assert_covers(err, {
> + reason = 'election',
> + state = 'follower',
> + code = box.error.READONLY,
> + type = 'ClientError'
> + }, 'reason is election, no leader')
> + t.assert(not err.leader_id, 'no leader id')
> + t.assert(not err.leader_uuid, 'no leader uuid')
> +
> + -- Cleanup.
> + g.master:exec(function()
> + box.cfg{election_mode = 'off'}
> + end)
> +end
> +
> +--
> +-- Read-only because is not a leader. Knows the leader.
> +--
> +g.test_read_only_reason_election_has_leader = function(g)
> + g.master:exec(function()
> + box.cfg{election_mode = 'candidate'}
> + end)
> + g.master:wait_election_leader()
> + g.replica:wait_election_leader_found()
> + local ok, err = g.replica:exec(function()
> + box.cfg{election_mode = 'voter'}
> + local ok, err = pcall(box.schema.create_space, 'test')
> + return ok, err:unpack()
> + end)
> + t.assert(not ok, 'fail ddl')
> + t.assert(err.term, 'has term')
> + t.assert_covers(err, {
> + reason = 'election',
> + state = 'follower',
> + leader_id = g.master:instance_id(),
> + leader_uuid = g.master:instance_uuid(),
> + code = box.error.READONLY,
> + type = 'ClientError'
> + }, 'reason is election, has leader')
> +
> + -- Cleanup.
> + g.master:exec(function()
> + box.cfg{election_mode = 'off'}
> + box.ctl.demote()
> + end)
> + g.replica:exec(function()
> + box.cfg{election_mode = 'off'}
> + end)
> +end
> +
> +--
> +-- Read-only because does not own the limbo. Knows the owner.
> +--
> +g.test_read_only_reason_synchro = function(g)
> + g.master:exec(function()
> + box.schema.create_space('test', {is_sync = true}):create_index('pk')
2. The space creation isn't necessary for this test.
> + box.ctl.promote()
> + end)
> +
> + t.helpers.retrying({}, function()
> + assert(g.replica:exec(function()
> + return box.info.synchro.queue.owner ~= 0
> + end))
> + end)
> +
> + local ok, err = g.replica:exec(function()
> + local ok, err = pcall(box.schema.create_space, 'test2')
> + return ok, err:unpack()
> + end)
> + t.assert(not ok, 'fail ddl')
> + t.assert(err.term, 'has term')
> + t.assert_covers(err, {
> + reason = 'synchro',
> + queue_owner_id = g.master:instance_id(),
> + queue_owner_uuid = g.master:instance_uuid(),
> + code = box.error.READONLY,
> + type = 'ClientError'
> + }, 'reason is synchro, has owner')
> +
> + -- Cleanup.
> + g.master:exec(function()
> + box.space.test:drop()
> + box.ctl.demote()
> + end)
> +end
> +
> +--
> +-- This group's test cases leave instances in an invalid state after which they
> +-- should be re-created.
> +--
> +g = t.group('gh-5568-read-only-reason2')
> +
> +g.before_each(make_create_cluster(g))
> +g.after_each(make_destroy_cluster(g))
> +
> +--
> +-- Read-only because is not a leader. Knows the leader, but not its UUID.
> +--
> +g.test_read_only_reason_election_has_leader_no_uuid = function(g)
> + g.replica:exec(function()
> + box.cfg{election_mode = 'voter'}
> + end)
> + g.master:exec(function()
> + box.cfg{election_mode = 'candidate'}
> + end)
> + g.master:wait_election_leader()
> + g.replica:wait_election_leader_found()
> + local leader_id = g.master:instance_id()
> +
> + g.master:exec(function()
> + box.space._cluster:run_triggers(false)
> + box.space._cluster:delete{box.info.id}
> + end)
> +
> + t.helpers.retrying({}, function()
> + assert(g.replica:exec(function(leader_id)
> + return box.space._cluster:get{leader_id} == nil
> + end, {leader_id}))
> + end)
> +
> + local ok, err = g.replica:exec(function()
> + local ok, err = pcall(box.schema.create_space, 'test')
> + return ok, err:unpack()
> + end)
> + t.assert(not ok, 'fail ddl')
> + t.assert(err.term, 'has term')
> + t.assert(not err.leader_uuid, 'has no leader uuid')
> + t.assert_covers(err, {
> + reason = 'election',
> + state = 'follower',
> + leader_id = leader_id,
> + code = box.error.READONLY,
> + type = 'ClientError'
> + }, 'reason is election, has leader but no uuid')
> +end
> +
> +--
> +-- Read-only because does not own the limbo. Knows the owner, but now its UUID.
> +--
3. Typo: now -> not.
> +g.test_read_only_reason_synchro_no_uuid = function(g)
> + g.master:exec(function()
> + box.ctl.promote()
> + box.space._cluster:run_triggers(false)
> + box.space._cluster:delete{box.info.id}
> + end)
> +
> + local leader_id = g.master:instance_id()
> + t.helpers.retrying({}, function()
> + assert(g.replica:exec(function(leader_id)
> + return box.info.synchro.queue.owner ~= 0 and
> + box.space._cluster:get{leader_id} == nil
> + end, {leader_id}))
> + end)
> +
> + local ok, err = g.replica:exec(function()
> + local ok, err = pcall(box.schema.create_space, 'test')
> + return ok, err:unpack()
> + end)
> + t.assert(not ok, 'fail ddl')
> + t.assert(err.term, 'has term')
> + t.assert(not err.queue_owner_uuid)
> + t.assert_covers(err, {
> + reason = 'synchro',
> + queue_owner_id = leader_id,
> + code = box.error.READONLY,
> + type = 'ClientError'
> + }, 'reason is synchro, has owner but no uuid')
> +end
--
Serge Petrenko
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details
2021-11-08 15:18 ` Serge Petrenko via Tarantool-patches
@ 2021-11-11 23:52 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 0 replies; 24+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-11 23:52 UTC (permalink / raw)
To: Serge Petrenko, tarantool-patches, vdavydov
Thanks for the review!
>> diff --git a/changelogs/unreleased/gh-5568-readonly-reason.md b/changelogs/unreleased/gh-5568-readonly-reason.md
>> new file mode 100644
>> index 000000000..f90b53bdf
>> --- /dev/null
>> +++ b/changelogs/unreleased/gh-5568-readonly-reason.md
>> @@ -0,0 +1,24 @@
>> +## feature/core
>> +
>> +* Error objects with the code `box.error.READONLY` now have additional fields
>> + explaining why the error happened (gh-5568).
>> +
>> + For instance, if the error was due to `box.cfg{read_only = true}`, then the
>> + error will have fields:
>> + ```
>> + {
>> + reason = 'state',
>> + state = 'read_only'
>> + }
>> + ```
>> + Or if it was due to not being a leader with `box.cfg{election_mode = ...}` set
>> + not to `off`, then:
>> + ```
>> + {
>> + state: <box.info.election.state of this instance>,
>> + leader_id: <box.info.id of the leader>,
>> + leader_uuid: <box.info.uuid of the leader>,
>> + reason: election,
>> + term: <box.info.election.term of this instance>
>> + }
>> + ```
>
> 1. Let's duplicate the additional error information in the error message.
>
> If it's hard to log every field, let's at least encode reason and state,
> so that the user knows what's wrong by simply looking at the error message.
I added this in a new commit on top of the branch.
>> \ No newline at end of file
I forgot a new line, added one:
====================
@@ -21,4 +21,4 @@
reason: election,
term: <box.info.election.term of this instance>
}
- ```
\ No newline at end of file
+ ```
====================
>> diff --git a/test/replication-luatest/gh_5568_read_only_reason_test.lua b/test/replication-luatest/gh_5568_read_only_reason_test.lua
>> new file mode 100644
>> index 000000000..d4f7ff957
>> --- /dev/null
>> +++ b/test/replication-luatest/gh_5568_read_only_reason_test.lua
<...>
>> +
>> +--
>> +-- Read-only because does not own the limbo. Knows the owner.
>> +--
>> +g.test_read_only_reason_synchro = function(g)
>> + g.master:exec(function()
>> + box.schema.create_space('test', {is_sync = true}):create_index('pk')
>
> 2. The space creation isn't necessary for this test.
Indeed, dropped:
====================
@@ -172,7 +172,6 @@ end
--
g.test_read_only_reason_synchro = function(g)
g.master:exec(function()
- box.schema.create_space('test', {is_sync = true}):create_index('pk')
box.ctl.promote()
end)
@@ -198,7 +197,6 @@ g.test_read_only_reason_synchro = function(g)
-- Cleanup.
g.master:exec(function()
- box.space.test:drop()
box.ctl.demote()
end)
end
====================
<...>
>> +
>> + local ok, err = g.replica:exec(function()
>> + local ok, err = pcall(box.schema.create_space, 'test')
>> + return ok, err:unpack()
>> + end)
>> + t.assert(not ok, 'fail ddl')
>> + t.assert(err.term, 'has term')
>> + t.assert(not err.leader_uuid, 'has no leader uuid')
>> + t.assert_covers(err, {
>> + reason = 'election',
>> + state = 'follower',
>> + leader_id = leader_id,
>> + code = box.error.READONLY,
>> + type = 'ClientError'
>> + }, 'reason is election, has leader but no uuid')
>> +end
>> +
>> +--
>> +-- Read-only because does not own the limbo. Knows the owner, but now its UUID.
>> +--
>
> 3. Typo: now -> not.
Done:
====================
--
--- Read-only because does not own the limbo. Knows the owner, but now its UUID.
+-- Read-only because does not own the limbo. Knows the owner, but not its UUID.
--
g.test_read_only_reason_synchro_no_uuid = function(g)
====================
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/9] ER_READONLY reason
2021-11-05 23:56 [Tarantool-patches] [PATCH 0/9] ER_READONLY reason Vladislav Shpilevoy via Tarantool-patches
` (8 preceding siblings ...)
2021-11-05 23:56 ` [Tarantool-patches] [PATCH 9/9] box: enrich ER_READONLY with new details Vladislav Shpilevoy via Tarantool-patches
@ 2021-11-08 14:25 ` Vladimir Davydov via Tarantool-patches
9 siblings, 0 replies; 24+ messages in thread
From: Vladimir Davydov via Tarantool-patches @ 2021-11-08 14:25 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
I'll review the PR instead:
https://github.com/tarantool/tarantool/pull/6591
^ permalink raw reply [flat|nested] 24+ messages in thread