Re: [Tarantool-patches] [PATCH v2 2/9] xrow: introduce a PROMOTE entry

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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