Re: [Tarantool-patches] [PATCH 2/9] error: introduce error_payload

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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