From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 D2EAD445322 for ; Fri, 24 Jul 2020 01:10:53 +0300 (MSK) References: <20200723122942.196011-1-gorcunov@gmail.com> <20200723122942.196011-5-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Fri, 24 Jul 2020 00:10:52 +0200 MIME-Version: 1.0 In-Reply-To: <20200723122942.196011-5-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 4/7] qsync: provide a binary form of CONFIRM/ROLLBACK entries List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml 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 > --- > 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; >