From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3])
 (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 42A384696C5
 for <tarantool-patches@dev.tarantool.org>;
 Thu, 23 Apr 2020 03:14:58 +0300 (MSK)
References: <cover.1583689251.git.v.shpilevoy@tarantool.org>
 <145c14e137171e6ee017da22aa579b369f7ccee8.1583689251.git.v.shpilevoy@tarantool.org>
 <20200403141237.GS22874@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <5fee88e7-b0ab-1423-3751-7e7e8331bac4@tarantool.org>
Date: Thu, 23 Apr 2020 02:14:56 +0200
MIME-Version: 1.0
In-Reply-To: <20200403141237.GS22874@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH 2/3] box: introduce port_c
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: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Hi! Thanks for the review!

>> diff --git a/src/box/port.c b/src/box/port.c
>> index 6e2fe3a6e..925a4b2d5 100644
>> --- a/src/box/port.c
>> +++ b/src/box/port.c
>> @@ -127,28 +132,194 @@ port_tuple_dump_msgpack(struct port *base, struct obuf *out)
>>  extern void
>>  port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat);
>>  
> 
> <snipped>
> 
>> +
>> +static void
>> +port_c_destroy(struct port *base)
>> +{
>> +	struct port_c *port = (struct port_c *)base;
>> +	struct port_c_entry *pe = port->first;
>> +	if (pe == NULL)
>> +		return;
>> +	port_c_destroy_entry(pe);
> 
> Minor: It's worth to drop a few words here that the port->first points
> to port->first_entry and it is released with the port_c structure.

No problem:

====================
diff --git a/src/box/port.c b/src/box/port.c
index 925a4b2d5..124625c7e 100644
--- a/src/box/port.c
+++ b/src/box/port.c
@@ -151,6 +151,11 @@ port_c_destroy(struct port *base)
 	if (pe == NULL)
 		return;
 	port_c_destroy_entry(pe);
+	/*
+	 * Port->first is skipped, it is pointing at
+	 * port_c.first_entry, and is freed together with the
+	 * port.
+	 */
 	pe = pe->next;
 	while (pe != NULL) {
 		struct port_c_entry *cur = pe;

====================

>> +	pe = pe->next;
>> +	while (pe != NULL) {
>> +		struct port_c_entry *cur = pe;
>> +		pe = pe->next;
>> +		port_c_destroy_entry(cur);
>> +		mempool_free(&port_entry_pool, cur);
>> +	}
>> +}
>> +
> 
> <snipped>
> 
>> +
>> +int
>> +port_c_add_mp(struct port *base, const char *mp, const char *mp_end)
>> +{
>> +	struct port_c *port = (struct port_c *)base;
>> +	struct port_c_entry *pe;
>> +	uint32_t size = mp_end - mp;
> 
> Minor: since there are no guarantees except the existing contract and
> common sense making this value be non-negative, I propose to add a
> corresponding assert. Feel free to ignore.

I don't mind.

====================
diff --git a/src/box/port.c b/src/box/port.c
index 925a4b2d5..3498f7af0 100644
--- a/src/box/port.c
+++ b/src/box/port.c
@@ -202,6 +207,7 @@ port_c_add_mp(struct port *base, const char *mp, const char *mp_end)
 {
 	struct port_c *port = (struct port_c *)base;
 	struct port_c_entry *pe;
+	assert(mp_end > mp);
 	uint32_t size = mp_end - mp;
 	char *dst;
 	if (size <= PORT_ENTRY_SIZE) {

====================