From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 4F732469719 for ; Fri, 16 Oct 2020 01:07:27 +0300 (MSK) References: <23f2b9468c529a6276ac75c1cfe81e60e1fabfb2.1602629628.git.tsafin@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 16 Oct 2020 00:07:25 +0200 MIME-Version: 1.0 In-Reply-To: <23f2b9468c529a6276ac75c1cfe81e60e1fabfb2.1602629628.git.tsafin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2.X v4 4/4] module api: box_ibuf_* wrappers List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Timur Safin , alexander.turenko@tarantool.org Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! See 4 comments below. On 14.10.2020 01:01, Timur Safin wrote: > Introduced the bare minimum of ibuf accessors, which make > external merger possible: > - box_ibuf_reserve > - box_ibuf_read_range > - box_ibuf_write_range > > Part of #5384 > --- > src/CMakeLists.txt | 1 + > src/box/CMakeLists.txt | 1 + > src/box/ibuf.c | 65 +++++++++++++++++++++++++++++++ > src/box/ibuf.h | 67 ++++++++++++++++++++++++++++++++ > src/exports.h | 3 ++ > test/app-tap/module_api.c | 44 +++++++++++++++++++++ > test/app-tap/module_api.test.lua | 2 +- > 7 files changed, 182 insertions(+), 1 deletion(-) > create mode 100644 src/box/ibuf.c > create mode 100644 src/box/ibuf.h > > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > index 699536652..3817f5c62 100644 > --- a/src/CMakeLists.txt > +++ b/src/CMakeLists.txt > @@ -152,6 +152,7 @@ set(api_headers > ${CMAKE_SOURCE_DIR}/src/box/tuple_extract_key.h > ${CMAKE_SOURCE_DIR}/src/box/schema_def.h > ${CMAKE_SOURCE_DIR}/src/box/box.h > + ${CMAKE_SOURCE_DIR}/src/box/ibuf.h > ${CMAKE_SOURCE_DIR}/src/box/index.h > ${CMAKE_SOURCE_DIR}/src/box/iterator_type.h > ${CMAKE_SOURCE_DIR}/src/box/error.h > diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt > index 8b2e704cf..690de5e83 100644 > --- a/src/box/CMakeLists.txt > +++ b/src/box/CMakeLists.txt > @@ -191,6 +191,7 @@ add_library(box STATIC > wal.c > call.c > merger.c > + ibuf.c > ${sql_sources} > ${lua_sources} > lua/init.c > diff --git a/src/box/ibuf.c b/src/box/ibuf.c > new file mode 100644 > index 000000000..12b9f2781 > --- /dev/null > +++ b/src/box/ibuf.c > @@ -0,0 +1,65 @@ > +/* > + * Copyright 2020, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include > + > +#include "ibuf.h" > +#include 1. Please, don't include non-system headers using <>. Use "". The same for box/ibuf.h. > + > +void * > +box_ibuf_reserve(box_ibuf_t *ibuf, size_t size) > +{ > + return ibuf_reserve(ibuf, size); 2. It should set a diag error in case of fail. > +} > + > +void > +box_ibuf_read_range(box_ibuf_t *ibuf, char ***rpos, char ***wpos) > +{ > + assert(ibuf != NULL); > + if (ibuf == NULL) > + return; > + if (rpos != NULL) > + *rpos = &ibuf->rpos; > + if (wpos != NULL) > + *wpos = &ibuf->wpos; 3. I would better assume all the arguments are not NULL, and document it. Especially ibuf itself. We need some border where to stop the sanity checks, and this looks like an overkill already. For example, box_tuple_ref() also works with a pointer, but it does not check for it being not NULL. It is just stupid to call method of an object with that object passed as NULL. The same below. > +} > + > +void > +box_ibuf_write_range(box_ibuf_t *ibuf, char ***wpos, char ***end) > +{ > + if (ibuf == NULL) > + return; > + if (wpos != NULL) > + *wpos = &ibuf->wpos; > + if (end != NULL) > + *end = &ibuf->end; > + > +} > diff --git a/src/box/ibuf.h b/src/box/ibuf.h > new file mode 100644 > index 000000000..d17f8f4b4 > --- /dev/null > +++ b/src/box/ibuf.h > @@ -0,0 +1,67 @@ > +#pragma once > +/* > + * Copyright 2020, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include > + > +#include > + > +#if defined(__cplusplus) > +extern "C" { > +#endif /* defined(__cplusplus) */ > + > +/** \cond public */ > + > +typedef struct ibuf box_ibuf_t; > + > +/** > + * Reserve requested amount of bytes in ibuf buffer 4. Please, finish sentences with a dot. I don't understand. Is it so hard for people to put a dot in the end? Does it cost money or something? Do people write the same way in other places, like emails, documents? Why is the code allowed to be treated worse? Talking of the comments content - they are mostly useless. What is read range, what is write range? How do they relate? Is a user supposed to update them manually, when work with the buffer? Current description makes it impossible to use box_ibuf_t without reading the source code. What users are not supposed to do usually to be able to use the API.