Re: [Tarantool-patches] [PATCH v8 2/4] module_cache: move module handling into own subsystem

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

>>> +
>>> +/**
>>> + * Find a path to a module using Lua's package.cpath.
>>> + *
>>> + * @param ctx module find context
>>> + * @param[out] ctx->path path to shared library.
>>> + * @param[out] ctx->path_len size of @a ctx->path buffer.
>>
>> 2. The last two lines are not params. The only parameter is ctx. If
>> you want to describe it, it should be done under the same @param.
>> Otherwise the comment looks mismatching the signature. I gonna say
>> that again. I don't force to use Doxygen, but if you decide to use it,
>> it should be done correctly.
> 
> Well, I didnt find any reference for the correct way to describe such
> updates of internal structures.

Doxygen syntax can be found on the official site, available on a first
link from google when you type 'doxygen'. If it does not provide a special
syntax for describing changes of individual fields of a single parameter,
then you need to describe them in the comment main part, or the @param
describing the parameter, like I proposed above.

> I think I better drop this doxy-style
> comment.

Or drop it, yes.

>>> + *
>>> + * @return 0 on succes, -1 otherwise, diag is set.
>>> + */
>>> +static int
>>> +module_find(struct module_find_ctx *ctx)
>>> diff --git a/src/box/module_cache.h b/src/box/module_cache.h
>>> new file mode 100644
>>> index 000000000..3f275024a
>>> --- /dev/null
>>> +++ b/src/box/module_cache.h
>>> @@ -0,0 +1,156 @@
>>> +/*
>>> + * SPDX-License-Identifier: BSD-2-Clause
>>> + *
>>> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
>>> + */
>>> +
>>> +#pragma once
>>> +
>>> +#include <small/rlist.h>
>>> +
>>> +#include "func_def.h"
>>
>> 3. Func_def heavily depends on schema. Also it is a part of
>> all the _func handling, meaning this is a cyclic dependency now -
>> module_cache depends on func and vice versa.
> 
> Nope. Where you get it? func_def.h is not touched in this series
> at all. What we have now

I didn't say you touched it. I said it depends on schema, and is a part of
stored functions code. Including func.h, func.c, call.h, call.c, and the
others.

> module_cache.h: func_def.h
> func.h: module_cache.h func_def.h
> func_def.h: trivia/util.h field_def.h opt_def.h
> 
> I don't understand which cyclic dependency you mean.

func.h/func.c/func_def.h/func_def.c are a part of the subsystem for
calling stored functions. Inside of the schema. It also depends on
module_cache.

module_cache is not related to schema, but depends on func_def.h
somewhy. This is the cycle. Module_cache and stored functions depend
on each other.

Talking of the schema details: func_def.h operates on user ID, on
function ID, on Lua specifics like sandboxing, knows about multikey,
and so on. It heavily depends on schema, and module_cache should not
depend on schema.

AFAIS, you included it simply because it defined box_function_ctx and
box_function_f. But it should be vice-versa.

I did this and all was built fine:

====================
diff --git a/src/box/func_def.h b/src/box/func_def.h
index d99d89190..75cd6a0d3 100644
--- a/src/box/func_def.h
+++ b/src/box/func_def.h
@@ -168,20 +168,6 @@ func_def_dup(struct func_def *def);
 int
 func_def_check(struct func_def *def);
 
-/**
- * API of C stored function.
- */
-
-struct port;
-
-struct box_function_ctx {
-	struct port *port;
-};
-
-typedef struct box_function_ctx box_function_ctx_t;
-typedef int (*box_function_f)(box_function_ctx_t *ctx,
-	     const char *args, const char *args_end);
-
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/box/module_cache.h b/src/box/module_cache.h
index 3f275024a..1ec7f0042 100644
--- a/src/box/module_cache.h
+++ b/src/box/module_cache.h
@@ -8,12 +8,20 @@
 
 #include <small/rlist.h>
 
-#include "func_def.h"
-
 #if defined(__cplusplus)
 extern "C" {
 #endif /* defined(__cplusplus) */
 
+struct port;
+
+struct box_function_ctx {
+	struct port *port;
+};
+
+typedef struct box_function_ctx box_function_ctx_t;
+typedef int (*box_function_f)(box_function_ctx_t *ctx,
+	     const char *args, const char *args_end);
+
 /**
  * Function name descriptor: a symbol and a package.
  */