From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 3E38D6FC82; Fri, 12 Nov 2021 02:50:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3E38D6FC82 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1636674637; bh=v4qJ8hadDCm0d5X6gdgubgxb8y9CvLUd3qOvqFT4780=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=jk5OO4EbbaSildit0acAU8uCcWB9rNEyOGzcH8omkhgfPUKRqwp3eY4egqacJtIVD THQsViVCD1S6L1ZtDO0FWVLmvJcPZwe5gAStfgnKr5lJNkKVMT5X5sqCNJh95tlCo5 F9xA+a22V55LD0nWh3c7BBCButGXgCG9Mzv05AFk= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 601CF6FC82 for ; Fri, 12 Nov 2021 02:50:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 601CF6FC82 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mlJpy-0002YP-3i; Fri, 12 Nov 2021 02:50:34 +0300 Message-ID: <4dbb6e33-104e-67b5-8c8f-7b32a443d8be@tarantool.org> Date: Fri, 12 Nov 2021 00:50:33 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 To: Serge Petrenko , tarantool-patches@dev.tarantool.org, vdavydov@tarantool.org References: <0fa9f99b412387269401bed410c480b456eeb604.1636156453.git.v.shpilevoy@tarantool.org> <68ebfc9b-5e2c-91c0-106a-8c309a31c1b3@tarantool.org> Content-Language: en-US In-Reply-To: <68ebfc9b-5e2c-91c0-106a-8c309a31c1b3@tarantool.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9731B3922EC063979B1F6335FDD7DA46529643274CFE4D7E800894C459B0CD1B9F1A65180966C148286C5AF40B55702A97F76FE7EF67E38FF5AA816C561F801B6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7C27E92EFAD44F80DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D7C7E03580C3DB018638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8DD06B4F2BB8D5792D81DBBF04E12270D117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD1828451B159A507268D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6300D3B61E77C8D3B089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B50537BF44C8E1EC058A0BF816FB72D1F5FD X-C1DE0DAB: 0D63561A33F958A51B6974B22478118D3719802AF96A16E0E060D29CB026FC53D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA754E2A133C74F7AB4F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A5112A9AECFE11B1800A72D985824EE73706740EAF7CBEA4479ED27D142F5BD36553808011F0B2A61D7E09C32AA3244CE313C38C3090ED81B98645C7811E0D1439C99C45E8D137E9FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj4t8MBgWr8bL8aHjDqOvHwA== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DD9E6EAE0C2B97A43366D55BA705802403841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 2/9] error: introduce error_payload X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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.