From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A5BF86B962; Wed, 14 Apr 2021 12:12:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A5BF86B962 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618391574; bh=fm18DbdVDPHSAHARzcLcVwytlizpGnOp0po9yfj73pw=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=FBJ3o+vIc90a4PMIkjmrUAFSvBvvmnUZz+IF9xAVO+RyeMZvOHd0vjS6cke8ZdL5M jTM8HYC3xeKWOmJdBXmqoC3C22mNvYDhOJzShVfCZ6xOriWPQ9y6Ke4G2edu4/88mk e9aaHFcBur6gJKmO5Ijc4kd+emaw6DrJ1sX1Xizo= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 551546BD23 for ; Wed, 14 Apr 2021 12:12:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 551546BD23 Received: by smtp42.i.mail.ru with esmtpa (envelope-from ) id 1lWbZs-0001Im-52; Wed, 14 Apr 2021 12:12:52 +0300 To: Cyrill Gorcunov Cc: v.shpilevoy@tarantool.org, tarantool-patches@dev.tarantool.org References: <6641b0b1f98ff233ccb3a121833cb6363c45edcb.1618256019.git.sergepetrenko@tarantool.org> Message-ID: <77f903ff-2820-653c-e386-98e0704b189b@tarantool.org> Date: Wed, 14 Apr 2021 12:12:51 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480257C85EA0BB7A95D5E28B957962BB550182A05F5380850401ACB7ED55DEDB8FF2245B2B96503236EAF9B81D0D5DC0BD7448A70F39B006A47 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F2EC3597058CFA6DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C1CDCB5E4A85220F8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2820849603A40A4B682DFEF21C9BC9C23CB629EEF1311BF91D2E47CDBA5A96583C09775C1D3CA48CFED8438A78DFE0A9E117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE70F3DDF2BBF19B93A9FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE77B21160C898DCF9CD32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C00CBC1925EB6DB8D6CD04E86FAF290E2D7E9C4E3C761E06A71DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3C9EEE74C166EF7BC35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5F1025A10C238CB85D6097F6C52C43897F2292E62D583F578D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D342F9EE325F82A28A87DFE59798EA1A9EA5DC8AF62B918E99E4FCF73DDDE658D6A0FEF338383B4DE011D7E09C32AA3244CF0511C65CDEA195EECCC4AB35FFFCF0E97FE24653F78E668FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXEkWM7i8F33bQ== X-Mailru-Sender: 583F1D7ACE8F49BDD2846D59FC20E9F8D15F61E8DFEC9F170154C8A7C6BF1A52896073F5B321078E424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 2/9] xrow: introduce a PROMOTE entry X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 13.04.2021 17:15, Cyrill Gorcunov пишет: > On Mon, Apr 12, 2021 at 10:40:15PM +0300, Serge Petrenko wrote: >> diff --git a/src/box/xrow.c b/src/box/xrow.c >> index cc8e43ed4..70ba075f8 100644 >> --- a/src/box/xrow.c >> +++ b/src/box/xrow.c >> @@ -890,11 +890,11 @@ xrow_encode_synchro(struct xrow_header *row, >> const struct synchro_request *req) >> { >> /* >> - * A map with two elements. We don't compress >> + * A map with two or three elements. We don't compress >> * numbers to have this structure constant in size, >> * which allows us to preallocate it on stack. >> */ >> - body->m_body = 0x80 | 2; >> + body->m_body = 0x80 | (req->type == IPROTO_PROMOTE ? 3 : 2); >> body->k_replica_id = IPROTO_REPLICA_ID; >> body->m_replica_id = 0xce; >> body->v_replica_id = mp_bswap_u32(req->replica_id); >> @@ -903,10 +903,24 @@ xrow_encode_synchro(struct xrow_header *row, >> body->v_lsn = mp_bswap_u64(req->lsn); >> >> memset(row, 0, sizeof(*row)); >> - >> row->type = req->type; >> - row->body[0].iov_base = (void *)body; >> - row->body[0].iov_len = sizeof(*body); >> + >> + /* Promote body is longer. It has an additional IPROTO_TERM field. */ >> + if (req->type == IPROTO_PROMOTE) { >> + struct promote_body_bin *promote_body = >> + (struct promote_body_bin *)body; >> + >> + promote_body->k_term = IPROTO_TERM; >> + promote_body->m_term = 0xcf; >> + promote_body->v_term = mp_bswap_u64(req->term); >> + >> + row->body[0].iov_base = (void *)promote_body; >> + row->body[0].iov_len = sizeof(*promote_body); >> + } else { >> + row->body[0].iov_base = (void *)body; >> + row->body[0].iov_len = sizeof(*body); >> + } >> + >> row->bodycnt = 1; >> } > You know, while I understand that we're trying to reuse code flow > here but I really don't like that this function unaware of type passed. > IOW the function may easily overwire caller's stack if you occasionally > pass synchro_body_bin instead of promote request. That's true and I've even caught that bug during the patch development. > > Actually I've sevaral options: > > 1) make the caller to provide a size and use assert() inside > this encoder to make sure the caller passer proper amount > of data from stack; > 2) provide own helper for promote packet encoding (see below). > > Still both approaches somehow *ugly* I think. Since there only a > few use of this encodings it is easy to remember where and what > and don't make a mistake. I think your second option looks better than what we have now. So, thanks for the suggestion! I've taken your diff with a couple of changes, please, take a look. The rest of the diff's in the reply for the 4th patch, because the changes to txn_limbo.c belong there. =============================================== diff --git a/src/box/xrow.c b/src/box/xrow.c index 70ba075f8..5d515ce92 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -884,10 +884,9 @@ xrow_encode_dml(const struct request *request, struct region *region,      return iovcnt;  } -void -xrow_encode_synchro(struct xrow_header *row, -            struct synchro_body_bin *body, -            const struct synchro_request *req) +static void +xrow_encode_synchro_body(struct synchro_body_bin *body, +                 const struct synchro_request *req)  {      /*       * A map with two or three elements. We don't compress @@ -901,26 +900,48 @@ xrow_encode_synchro(struct xrow_header *row,      body->k_lsn = IPROTO_LSN;      body->m_lsn = 0xcf;      body->v_lsn = mp_bswap_u64(req->lsn); +} + +void +xrow_encode_synchro(struct xrow_header *row, +            struct synchro_body_bin *body, +            const struct synchro_request *req) +{ +    assert(req->type == IPROTO_CONFIRM || req->type == IPROTO_ROLLBACK); + +    xrow_encode_synchro_body(body, req);      memset(row, 0, sizeof(*row));      row->type = req->type; +    row->body[0].iov_base = body; +    row->body[0].iov_len = sizeof(*body); +    row->bodycnt = 1; +} -    /* Promote body is longer. It has an additional IPROTO_TERM field. */ -    if (req->type == IPROTO_PROMOTE) { -        struct promote_body_bin *promote_body = -            (struct promote_body_bin *)body; +static inline void +xrow_encode_promote_body(struct promote_body_bin *body, +             const struct synchro_request *req) +{ +    xrow_encode_synchro_body(&body->base, req); -        promote_body->k_term = IPROTO_TERM; -        promote_body->m_term = 0xcf; -        promote_body->v_term = mp_bswap_u64(req->term); +    body->k_term = IPROTO_TERM; +    body->m_term = 0xcf; +    body->v_term = mp_bswap_u64(req->term); +} -        row->body[0].iov_base = (void *)promote_body; -        row->body[0].iov_len = sizeof(*promote_body); -    } else { -        row->body[0].iov_base = (void *)body; -        row->body[0].iov_len = sizeof(*body); -    } +void +xrow_encode_promote(struct xrow_header *row, struct promote_body_bin *body, +            const struct synchro_request *req) +{ +    assert(req->type == IPROTO_PROMOTE); + +    xrow_encode_promote_body(body, req); + +    memset(row, 0, sizeof(*row)); +    row->type = req->type; +    row->body[0].iov_base = body; +    row->body[0].iov_len = sizeof(*body);      row->bodycnt = 1;  } diff --git a/src/box/xrow.h b/src/box/xrow.h index af4ad0d12..d75ab0cc9 100644 --- a/src/box/xrow.h +++ b/src/box/xrow.h @@ -290,6 +290,16 @@ xrow_encode_synchro(struct xrow_header *row,              struct synchro_body_bin *body,              const struct synchro_request *req); +/** + * Encode a promote request. + * @param row xrow header. + * @param body A place to encode promote body. + * @param req Request parameters. + */ +void +xrow_encode_promote(struct xrow_header *row, struct promote_body_bin *body, +            const struct synchro_request *req); +  /**   * Decode synchronous replication request.   * @param row xrow header. -- Serge Petrenko