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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.