Re: [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 2 comments below.

On 19/02/2020 15:16, Nikita Pettik wrote:
> From: Kirill Shcherbatov <kshcherbatov@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
>