From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38])
 (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 55BA4469719
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 16 Oct 2020 00:52:35 +0300 (MSK)
Date: Fri, 16 Oct 2020 00:52:55 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20201015215255.bykuuldtpygharst@tkn_work_nb>
References: <cover.1602629628.git.tsafin@tarantool.org>
 <23f2b9468c529a6276ac75c1cfe81e60e1fabfb2.1602629628.git.tsafin@tarantool.org>
 <20201014033117.ipfnfm4bxfklhp4c@tkn_work_nb>
 <053201d6a33b$0c1c2f60$24548e20$@tarantool.org>
 <20201015214225.dwgcpi7fvizrkcem@tkn_work_nb>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20201015214225.dwgcpi7fvizrkcem@tkn_work_nb>
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: Timur Safin <tsafin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org

On Fri, Oct 16, 2020 at 12:42:26AM +0300, Alexander Turenko wrote:
> On Fri, Oct 16, 2020 at 12:35:05AM +0300, Timur Safin wrote:
> > 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.
> 
> Nit: it *sets* an error, not raise.
> 
> > 
> > +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;
> > +}
> 
> You should return a code (0 / -1) so. The diagnostics area is like
> errno: a caller should lean on the code, not the diagnostics area to
> determine whether an error took place. If it occurs, then the
> diagnostics is guaranteed to exist and to be actual.

Hey, stop. Is there any other function in the module API that allows to
pass NULL instead of an object? Is there a valid use case for this?
Should a user check box_ibuf_read_range() return value and handle a
possible error?

I don't see any sense in handling NULL here. Let's leave assert(ibuf !=
NULL) or nothing.

The practice about silengly ignore incorrect input parameters that you
mentioned before is certainly bad patters. I suggest to avoid it.