Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_* wrappers

["Timur Safin" <tsafin@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()).