Re: [Tarantool-patches] [PATCH v2 4/7] qsync: provide a binary form of CONFIRM/ROLLBACK entries

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

Thanks for the patch!

See 2 commits below.

On 23.07.2020 14:29, Cyrill Gorcunov wrote:
> These msgpack entries will be needed to write them
> down to a journal without involving txn engine.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/iproto_constants.h | 27 +++++++++++++++++++++++++++
>  src/box/xrow.c             | 23 ++++++++---------------
>  2 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
> index 6b850f101..1e6566e0f 100644
> --- a/src/box/iproto_constants.h
> +++ b/src/box/iproto_constants.h
> @@ -328,6 +328,33 @@ iproto_type_is_synchro_request(uint32_t type)
>  	return type == IPROTO_CONFIRM || type == IPROTO_ROLLBACK;
>  }
>  
> +/** CONFIRM/ROLLBACK entries encoded in MsgPack. */
> +struct PACKED request_synchro_body {

1. Please, call it _bin, not _body. Look at the examples iproto_header_bin,
iproto_body_bin. _body_bin also would be fine. But should end with _bin
anyway.

> +	uint8_t m_body;
> +	uint8_t k_replica_id;
> +	uint8_t m_replica_id;
> +	uint32_t v_replica_id;
> +	uint8_t k_lsn;
> +	uint8_t m_lsn;
> +	uint64_t v_lsn;
> +};
> +
> +/**
> + * Creates CONFIRM/ROLLBACK entry.
> + */
> +static inline void
> +request_synchro_body_create(struct request_synchro_body *body,
> +			    uint32_t replica_id, int64_t lsn)
> +{
> +	body->m_body = 0x80 | 2;
> +	body->k_replica_id = IPROTO_REPLICA_ID;
> +	body->m_replica_id = 0xce;
> +	body->v_replica_id = mp_bswap_u32(replica_id);
> +	body->k_lsn = IPROTO_LSN;
> +	body->m_lsn = 0xcf;
> +	body->v_lsn = mp_bswap_u64(lsn);
> +}
> +
>  /** This is an error. */
>  static inline bool
>  iproto_type_is_error(uint32_t type)
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index 0c797a9d5..c2f4ba40a 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -897,26 +897,19 @@ static int
>  xrow_encode_confirm_rollback(struct xrow_header *row, struct region *region,
>  			     uint32_t replica_id, int64_t lsn, int type)
>  {
> -	size_t len = mp_sizeof_map(2) + mp_sizeof_uint(IPROTO_REPLICA_ID) +
> -		     mp_sizeof_uint(replica_id) + mp_sizeof_uint(IPROTO_LSN) +
> -		     mp_sizeof_uint(lsn);
> -	char *buf = (char *)region_alloc(region, len);
> -	if (buf == NULL) {
> -		diag_set(OutOfMemory, len, "region_alloc", "buf");
> +	struct request_synchro_body *body;
> +
> +	body = region_alloc(region, sizeof(*body));

2. I see you omit explicit type casts. And use (void *) in the next commits,
when need to cast between 2 non-void types. Both ways are not used in Tarantool.
At least they are not part of the code style. Personally I am fine if we will
adopt your way. It is not too conflicting with the old code (don't need to
change the old code then), is more compact. But you need to raise that question
with other teammates. So as we could write down that way in the doc, if everyone
agrees. And force it in the newer commits.

> +	if (body == NULL) {
> +		diag_set(OutOfMemory, sizeof(*body), "region_alloc", "body");
>  		return -1;
>  	}
> -	char *pos = buf;
> -
> -	pos = mp_encode_map(pos, 2);
> -	pos = mp_encode_uint(pos, IPROTO_REPLICA_ID);
> -	pos = mp_encode_uint(pos, replica_id);
> -	pos = mp_encode_uint(pos, IPROTO_LSN);
> -	pos = mp_encode_uint(pos, lsn);
> +	request_synchro_body_create(body, replica_id, lsn);
>  
>  	memset(row, 0, sizeof(*row));
>  
> -	row->body[0].iov_base = buf;
> -	row->body[0].iov_len = len;
> +	row->body[0].iov_base = (void *)body;
> +	row->body[0].iov_len = sizeof(*body);
>  	row->bodycnt = 1;
>  
>  	row->type = type;
>