From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <korablev@tarantool.org>
Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58])
 (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 D93EC4696C3
 for <tarantool-patches@dev.tarantool.org>;
 Wed,  1 Apr 2020 18:54:22 +0300 (MSK)
Date: Wed, 1 Apr 2020 15:54:22 +0000
From: Nikita Pettik <korablev@tarantool.org>
Message-ID: <20200401155422.GB26447@tarantool.org>
References: <cover.1585097339.git.korablev@tarantool.org>
 <bb90530b87c1ed2d9898e555866ab8178cd36016.1585097339.git.korablev@tarantool.org>
 <b1541e1b-ea0e-8473-1277-7ba76c34de9e@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <b1541e1b-ea0e-8473-1277-7ba76c34de9e@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v2 09/10] iproto: refactor error
	encoding with mpstream
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

On 31 Mar 01:24, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/xrow.c b/src/box/xrow.c
> > index 5e3cb0709..256dd4d91 100644
> > --- a/src/box/xrow.c
> > +++ b/src/box/xrow.c
> > @@ -478,46 +475,77 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot,
> >  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;
> >  
> > +	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;
> 
> The function is supposed to return 0/-1. So better keep this
> convention. You can declare is_error as 'int rc', and set it to
> -1 in mpstream_error_handler(). Should be fine. Or make it
> 
>     return is_error ? -1 : 0;

I prefer the latter option. Fixed.