From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 15A1A4696C3 for ; Sun, 23 Feb 2020 20:44:24 +0300 (MSK) References: <8d47b2263156c470382e264ae712bd2a3da8afe5.1582119629.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <8e5c5ce4-69a3-4a8b-cb4e-8071e89564e3@tarantool.org> Date: Sun, 23 Feb 2020 18:44:22 +0100 MIME-Version: 1.0 In-Reply-To: <8d47b2263156c470382e264ae712bd2a3da8afe5.1582119629.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Thanks for the patch! See 2 comments below. On 19/02/2020 15:16, Nikita Pettik wrote: > From: Kirill Shcherbatov > > Refactor iproto_reply_error and iproto_write_error with a new > mpstream-based helper mpstream_iproto_encode_error that encodes > error object for iproto protocol on a given stream object. > Previously each routine implemented an own error encoding, but > with the increasing complexity of encode operation with following > patches we need a uniform way to do it. > > The iproto_write_error routine starts using region location > to use region-based mpstream. It is not a problem itself, because > errors reporting is not really performance-critical path. > > Needed for #1148 > --- > src/box/xrow.c | 72 +++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 51 insertions(+), 21 deletions(-) > > diff --git a/src/box/xrow.c b/src/box/xrow.c > index 968c3a202..3f1c90c87 100644 > --- a/src/box/xrow.c > +++ b/src/box/xrow.c > @@ -36,12 +36,14 @@ > #include "third_party/base64.h" > > #include "fiber.h" > +#include "reflection.h" 1. I removed this header and nothing changed. So why do you need it? > #include "version.h" > #include "tt_static.h" > #include "error.h" > #include "vclock.h" > #include "scramble.h" > #include "iproto_constants.h" > +#include "mpstream.h" > > static_assert(IPROTO_DATA < 0x7f && IPROTO_METADATA < 0x7f && > IPROTO_SQL_INFO < 0x7f, "encoded IPROTO_BODY keys must fit into "\ > @@ -478,46 +476,78 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot, > return 0; > } > > +static void > +mpstream_error_handler(void *error_ctx) > +{ > + *(bool *)error_ctx = true; > +} > + > +static void > +mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error) > +{ > + mpstream_encode_map(stream, 2); > + mpstream_encode_uint(stream, IPROTO_ERROR); > + mpstream_encode_str(stream, error->errmsg); > +} > + > int > iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync, > uint32_t schema_version) > { > - uint32_t msg_len = strlen(e->errmsg); > - uint32_t errcode = box_error_code(e); > - > - struct iproto_body_bin body = iproto_error_bin; > char *header = (char *)obuf_alloc(out, IPROTO_HEADER_LEN); > if (header == NULL) > return -1; > > + /* The obuf-based stream has reserved area for header. */ 2. No, you reserved space for the header just a few lines above. Mpstream does not know anything about headers. > + bool is_error = false; > + struct mpstream stream; > + mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb, > + mpstream_error_handler, &is_error); > + > + uint32_t used = obuf_size(out); > + mpstream_iproto_encode_error(&stream, e); > + mpstream_flush(&stream); > + > + uint32_t errcode = box_error_code(e); > iproto_header_encode(header, iproto_encode_error(errcode), sync, > - schema_version, sizeof(body) + msg_len); > - body.v_data_len = mp_bswap_u32(msg_len); > + schema_version, obuf_size(out) - used); > + > /* Malformed packet appears to be a lesser evil than abort. */ > - return obuf_dup(out, &body, sizeof(body)) != sizeof(body) || > - obuf_dup(out, e->errmsg, msg_len) != msg_len ? -1 : 0; > + return is_error; > } > > void > iproto_write_error(int fd, const struct error *e, uint32_t schema_version, > uint64_t sync) > { > - uint32_t msg_len = strlen(e->errmsg); > - uint32_t errcode = box_error_code(e); > + bool is_error = false; > + struct mpstream stream; > + struct region *region = &fiber()->gc; > + mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb, > + mpstream_error_handler, &is_error); > + > + size_t region_svp = region_used(region); > + mpstream_iproto_encode_error(&stream, e); > + mpstream_flush(&stream); > + if (is_error) > + goto cleanup; > + > + size_t payload_size = region_used(region) - region_svp; > + char *payload = region_join(region, payload_size); > + if (payload == NULL) > + goto cleanup; > > + uint32_t errcode = box_error_code(e); > char header[IPROTO_HEADER_LEN]; > - struct iproto_body_bin body = iproto_error_bin; > - > iproto_header_encode(header, iproto_encode_error(errcode), sync, > - schema_version, sizeof(body) + msg_len); > - > - body.v_data_len = mp_bswap_u32(msg_len); > + schema_version, payload_size); > > ssize_t unused; > unused = write(fd, header, sizeof(header)); > - unused = write(fd, &body, sizeof(body)); > - unused = write(fd, e->errmsg, msg_len); > + unused = write(fd, payload, payload_size); > (void) unused; > +cleanup: > + region_truncate(region, region_svp); > } > > int >