From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tsafin@tarantool.org>
Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111])
 (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 CFEE0469719
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 16 Oct 2020 00:35:06 +0300 (MSK)
From: "Timur Safin" <tsafin@tarantool.org>
References: <cover.1602629628.git.tsafin@tarantool.org>
 <23f2b9468c529a6276ac75c1cfe81e60e1fabfb2.1602629628.git.tsafin@tarantool.org>
 <20201014033117.ipfnfm4bxfklhp4c@tkn_work_nb>
In-Reply-To: <20201014033117.ipfnfm4bxfklhp4c@tkn_work_nb>
Date: Fri, 16 Oct 2020 00:35:05 +0300
Message-ID: <053201d6a33b$0c1c2f60$24548e20$@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Language: ru
Subject: Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_*
	wrappers
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: 'Alexander Turenko' <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org

After our discussions within messenger, I've updated ibuf.c to
raise error if there is invalid argument passed, to follow
current box_* functions convention. E.g.

+void
+box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
+{
+       if (ibuf == NULL) {
+               diag_set(ClientError, ER_ILLEGAL_PARAMS,
+                        "Invalid ibuf argument");
+               return;
+       }
+       if (rpos != NULL)
+               *rpos = &ibuf->rpos;
+       if (wpos != NULL)
+               *wpos = &ibuf->wpos;
+}
+
+void
+box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end)
+{
+       if (ibuf == NULL) {
+               diag_set(ClientError, ER_ILLEGAL_PARAMS,
+                        "Invalid ibuf argument");
+               return;
+       }
+       if (wpos != NULL)
+               *wpos = &ibuf->wpos;
+       if (end != NULL)
+               *end = &ibuf->end;
+
+}

Code has been updated in the branch tsafin/gh-5273-expand-module-api-v4

Regards,
Timur

: -----Original Message-----
: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Sent: Wednesday, October 14, 2020 6:31 AM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: v.shpilevoy@tarantool.org; tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 2.X v4 4/4] module api: box_ibuf_* wrappers
: 
: > +void
: > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos)
: > +{
: > +	assert(ibuf != NULL);
: > +	if (ibuf == NULL)
: > +		return;
: 
: So it'll not (at least stochastically) fail on ibuf pointer
: dereferencing on RelWithDebInfo, but will just silently ignore the usage
: error. I guess it is the bad pattern. At least I don't see anything
: similar in our code: we either assert() or fail with setting an error to
: the diagnostics area and returning of -1 / NULL or kinda.
: 
: So I propose to jeave assert(), but remove the if (ibuf == NULL) {<...>}.
: 
: Don't forget to update box_ibuf_write_range() in sync (not it has no
: assert()).