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

[Nikita Pettik <korablev@tarantool.org>]

On 23 Feb 18:44, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 2 comments below.
> 
> On 19/02/2020 15:16, Nikita Pettik wrote:
> > From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> > 
> > +}
> > +
> >  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.

Didn't really get what you mean here, so just in case dropped this
comment at all :)
 
> > +	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;
> >  }