[Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Feb 23 20:44:22 MSK 2020
Thanks for the patch!
See 2 comments below.
On 19/02/2020 15:16, Nikita Pettik wrote:
> From: Kirill Shcherbatov <kshcherbatov at tarantool.org>
>
> 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
>
More information about the Tarantool-patches
mailing list