[Tarantool-patches] [PATCH 2/9] error: introduce error_payload
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Nov 12 02:50:33 MSK 2021
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.
More information about the Tarantool-patches
mailing list