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

[Vladislav Shpilevoy <v.shpilevoy@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 <COPYRIGHT HOLDER> ``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
> + * <COPYRIGHT HOLDER> 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 <stdlib.h>
> +
> +#include "ibuf.h"
> +#include <small/ibuf.h>

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 <COPYRIGHT HOLDER> ``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
> + * <COPYRIGHT HOLDER> 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 <stdlib.h>
> +
> +#include <trivia/util.h>
> +
> +#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.