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 647876FC82; Fri, 12 Nov 2021 09:29:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 647876FC82 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1636698588; bh=j93Od/3/V2NwYi0rQK5xGwSmsbS+6xDkONbz6zpAfaQ=; 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=Px35tCzZS3lMS1IVnld8D5FJH/sWv96w0jqBn3W919cJwf7f+iGBrl8LLoPheOn0V 8zpIqvEsAMUgL0Vg//ksnTn2uz1JnTf2FT7gFqXl6bK0mViOsuXdPRL5G0IhORCqy/ rNiGCZxOjf2zDH6NXGga+Hzxpd4cluxYOEprIav4= Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 DDDF66FC82 for ; Fri, 12 Nov 2021 09:29:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DDDF66FC82 Received: by smtp32.i.mail.ru with esmtpa (envelope-from ) id 1mlQ4H-0007DE-PC; Fri, 12 Nov 2021 09:29:46 +0300 Message-ID: Date: Fri, 12 Nov 2021 09:29:45 +0300 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 Content-Language: en-GB To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, vdavydov@tarantool.org References: <0fa9f99b412387269401bed410c480b456eeb604.1636156453.git.v.shpilevoy@tarantool.org> <68ebfc9b-5e2c-91c0-106a-8c309a31c1b3@tarantool.org> <4dbb6e33-104e-67b5-8c8f-7b32a443d8be@tarantool.org> In-Reply-To: <4dbb6e33-104e-67b5-8c8f-7b32a443d8be@tarantool.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9731B3922EC063979DD5E01717F989B5F47BAD767D6B13F3A00894C459B0CD1B97BF4FEB5505D2B3BFE4292FE8B164690B22E602945078FB110FFD2E5AA185B68 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7EC0B1A4921CAE631EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C904E3CF4B5CD3198638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8F054360BFC6E614F7F02DE76DCCB88A3117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751FC26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EED76C6ED7039589DEAD7EC71F1DB884274AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3963AE2AFEF160AEBBA3038C0950A5D36B5C8C57E37DE458B0BC6067A898B09E46D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE778B471BB9634AD8A731C566533BA786AA5CC5B56E945C8DA X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5057ABAA89EDCD96AF92305A255A689E8CF X-C1DE0DAB: 0D63561A33F958A5437C77C01947D5E627EBF5251F5DC788A5771F17834FD914D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA754E2A133C74F7AB4F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34324A9840C798D5004195ACCE3E744890B31583A77CE1D41346E36FBCCA00E1CDED72803B06EE95111D7E09C32AA3244C6127A271217653567ED060279620BD3095A9E0DC41E9A4CFFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj4t8MBgWr8bKHXTFRlCBgjg== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE4078DEBE7069D16E40DEF47773D8CB88122FFC60F4E7AFB3DC76BB2E709EA627F343C7DDD459B58856F0E45BC603594F5A135B915D4279FF0579437F6177E88F7363CDA0F3B3F5B9367 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: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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