[Tarantool-patches] [PATCH 2/9] error: introduce error_payload
Serge Petrenko
sergepetrenko at tarantool.org
Fri Nov 12 09:29:45 MSK 2021
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
More information about the Tarantool-patches
mailing list