* [PATCH 01/11] box: move info_handler interface into src/info
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-12-03 11:05 ` Vladimir Davydov
2018-12-03 20:41 ` [tarantool-patches] " Konstantin Osipov
2018-11-30 15:39 ` [PATCH 10/11] evio: remove exceptions Vladislav Shpilevoy
` (10 subsequent siblings)
11 siblings, 2 replies; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Box/info.h defines info_handler interface with a set
of virtual functions. It allows to hide Lua from code
not depending on this language, and is used in things
like index:info(), box.info() to build Lua table with
some info. But it does not depend on box/ so move it
to src/. And alongside apply a bit refactoring: remove
useless comments, comply with line width etc.
Also, this API is needed for the forthcoming SWIM
module which is going to be placed into src/.
Needed for #3234
---
src/CMakeLists.txt | 1 +
src/box/lua/index.c | 4 +-
src/box/lua/info.c | 78 +---------------------------
src/box/lua/sql.c | 4 +-
src/box/lua/stat.c | 4 +-
src/box/sql.c | 2 +-
src/{box => }/info.h | 84 +++++++++++++++---------------
src/lua/info.c | 118 +++++++++++++++++++++++++++++++++++++++++++
src/lua/info.h | 49 ++++++++++++++++++
9 files changed, 218 insertions(+), 126 deletions(-)
rename src/{box => }/info.h (72%)
create mode 100644 src/lua/info.c
create mode 100644 src/lua/info.h
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index e004f2154..7d0734f55 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -179,6 +179,7 @@ set (server_sources
lua/crypto.c
lua/httpc.c
lua/utf8.c
+ lua/info.c
${lua_sources}
${PROJECT_SOURCE_DIR}/third_party/lua-yaml/lyaml.cc
${PROJECT_SOURCE_DIR}/third_party/lua-yaml/b64.c
diff --git a/src/box/lua/index.c b/src/box/lua/index.c
index ef89c397d..6265c044a 100644
--- a/src/box/lua/index.c
+++ b/src/box/lua/index.c
@@ -30,10 +30,10 @@
*/
#include "box/lua/index.h"
#include "lua/utils.h"
+#include "lua/info.h"
+#include <info.h>
#include "box/box.h"
#include "box/index.h"
-#include "box/info.h"
-#include "box/lua/info.h"
#include "box/lua/tuple.h"
#include "box/lua/misc.h" /* lbox_encode_tuple_on_gc() */
diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index 03b953288..f94121e61 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -32,7 +32,7 @@
#define _GNU_SOURCE
#endif
-#include "box/lua/info.h"
+#include "lua/info.h"
#include <ctype.h> /* tolower() */
@@ -45,7 +45,7 @@
#include "box/iproto.h"
#include "box/wal.h"
#include "box/replication.h"
-#include "box/info.h"
+#include <info.h>
#include "box/gc.h"
#include "box/engine.h"
#include "box/vinyl.h"
@@ -448,80 +448,6 @@ lbox_info_gc(struct lua_State *L)
return 1;
}
-static void
-luaT_info_begin(struct info_handler *info)
-{
- lua_State *L = (lua_State *) info->ctx;
- lua_newtable(L);
-}
-
-static void
-luaT_info_end(struct info_handler *info)
-{
- (void) info;
-}
-
-static void
-luaT_info_begin_table(struct info_handler *info, const char *key)
-{
- lua_State *L = (lua_State *) info->ctx;
- lua_pushstring(L, key);
- lua_newtable(L);
-}
-
-static void
-luaT_info_end_table(struct info_handler *info)
-{
- lua_State *L = (lua_State *) info->ctx;
- lua_settable(L, -3);
-}
-
-static void
-luaT_info_append_double(struct info_handler *info,
- const char *key, double value)
-{
- lua_State *L = (lua_State *) info->ctx;
- lua_pushstring(L, key);
- lua_pushnumber(L, value);
- lua_settable(L, -3);
-}
-
-static void
-luaT_info_append_int(struct info_handler *info, const char *key,
- int64_t value)
-{
- lua_State *L = (lua_State *) info->ctx;
- lua_pushstring(L, key);
- luaL_pushint64(L, value);
- lua_settable(L, -3);
-}
-
-static void
-luaT_info_append_str(struct info_handler *info, const char *key,
- const char *value)
-{
- lua_State *L = (lua_State *) info->ctx;
- lua_pushstring(L, key);
- lua_pushstring(L, value);
- lua_settable(L, -3);
-}
-
-void
-luaT_info_handler_create(struct info_handler *h, struct lua_State *L)
-{
- static struct info_handler_vtab lua_vtab = {
- .begin = luaT_info_begin,
- .end = luaT_info_end,
- .begin_table = luaT_info_begin_table,
- .end_table = luaT_info_end_table,
- .append_int = luaT_info_append_int,
- .append_str = luaT_info_append_str,
- .append_double = luaT_info_append_double
- };
- h->vtab = &lua_vtab;
- h->ctx = L;
-}
-
static int
lbox_info_vinyl_call(struct lua_State *L)
{
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 17e269423..692366607 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -3,9 +3,9 @@
#include "lua/msgpack.h"
#include "box/sql/sqliteInt.h"
-#include "box/info.h"
+#include <info.h>
+#include "lua/info.h"
#include "lua/utils.h"
-#include "info.h"
static void
lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
diff --git a/src/box/lua/stat.c b/src/box/lua/stat.c
index 83a5b64e6..3fce81f61 100644
--- a/src/box/lua/stat.c
+++ b/src/box/lua/stat.c
@@ -42,8 +42,8 @@
#include "box/iproto.h"
#include "box/engine.h"
#include "box/vinyl.h"
-#include "box/info.h"
-#include "box/lua/info.h"
+#include <info.h>
+#include "lua/info.h"
#include "lua/utils.h"
extern struct rmean *rmean_box;
diff --git a/src/box/sql.c b/src/box/sql.c
index c3418008c..1fcd3e809 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -36,7 +36,7 @@
#include "sql/vdbeInt.h"
#include "index.h"
-#include "info.h"
+#include <info.h>
#include "schema.h"
#include "box.h"
#include "txn.h"
diff --git a/src/box/info.h b/src/info.h
similarity index 72%
rename from src/box/info.h
rename to src/info.h
index df82becd5..bf1809ca9 100644
--- a/src/box/info.h
+++ b/src/info.h
@@ -1,7 +1,7 @@
-#ifndef INCLUDES_TARANTOOL_BOX_INFO_H
-#define INCLUDES_TARANTOOL_BOX_INFO_H
+#ifndef INCLUDES_TARANTOOL_INFO_H
+#define INCLUDES_TARANTOOL_INFO_H
/*
- * Copyright 2010-2017, Tarantool AUTHORS, please see AUTHORS file.
+ * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
*
* Redistribution and use in source and binary forms, with or
* without modification, are permitted provided that the following
@@ -35,9 +35,10 @@
/**
* @file
- * This module provides an adapter for Lua/C API to generate box.info()
- * and index:info() introspection trees. The primary purpose of this
- * adapter is to eliminate Engine <-> Lua interdependency.
+ * This module provides an adapter for Lua/C API to generate
+ * box.info() and index:info() introspection trees. The primary
+ * purpose of this adapter is to eliminate Engine <-> Lua
+ * interdependency.
*
* TREE STRUCTURE
*
@@ -57,20 +58,18 @@
*
* IMPLEMENTATION DETAILS
*
- * Current implementation calls Lua/C API under the hood without any
- * pcall() wrapping. As you may now, idiosyncratic Lua/C API unwinds
- * C stacks on errors in a way you can't handle in C. Please ensure that
- * all blocks of code which call info_append_XXX() functions are
- * exception/longjmp safe.
+ * Current implementation calls Lua/C API under the hood without
+ * any pcall() wrapping. As you may now, idiosyncratic Lua/C API
+ * unwinds C stacks on errors in a way you can't handle in C.
+ * Please ensure that all blocks of code which call
+ * info_append_XXX() functions are exception/longjmp safe.
*/
#if defined(__cplusplus)
extern "C" {
#endif /* defined(__cplusplus) */
-/**
- * Virtual method table for struct info_handler.
- */
+/** Virtual method table for struct info_handler. */
struct info_handler_vtab {
/** The begin of document. */
void (*begin)(struct info_handler *);
@@ -86,13 +85,17 @@ struct info_handler_vtab {
/** Set int64_t value. */
void (*append_int)(struct info_handler *, const char *key,
int64_t value);
+ /** Set uint64_t value. */
+ void (*append_uint)(struct info_handler *, const char *key,
+ uint64_t value);
/** Set double value. */
void (*append_double)(struct info_handler *,
const char *key, double value);
};
/**
- * Adapter for Lua/C API to generate box.info() sections from engines.
+ * Adapter for Lua/C API to generate box.info() sections from
+ * engines.
*/
struct info_handler {
struct info_handler_vtab *vtab;
@@ -102,8 +105,6 @@ struct info_handler {
/**
* Starts a new document and creates root-level associative array.
- * @param info box.info() adapter.
- * @throws C++ exception on OOM, see info.h comments.
* @pre must be called once before any other functions.
*/
static inline void
@@ -114,8 +115,6 @@ info_begin(struct info_handler *info)
/**
* Finishes the document and closes root-level associative array.
- * @param info box.info() adapter.
- * @throws C++ exception on OOM, see info.h comments.
* @pre must be called at the end.
*/
static inline void
@@ -125,11 +124,8 @@ info_end(struct info_handler *info)
}
/**
- * Associates int64_t value with @a key in the current associative array.
- * @param info box.info() adapter.
- * @param key key.
- * @param value value.
- * @throws C++ exception on OOM, see info.h comments.
+ * Associates int64_t value with @a key in the current associative
+ * array.
* @pre associative array is started.
*/
static inline void
@@ -139,16 +135,24 @@ info_append_int(struct info_handler *info, const char *key, int64_t value)
}
/**
- * Associates zero-terminated string with @a key in the current associative
- * array.
- * @param info box.info() adapter.
- * @param key key.
- * @param value value.
- * @throws C++ exception on OOM, see info.h comments.
+ * Associates uint64_t value with @a key in the current
+ * associative array.
+ * @pre associative array is started.
+ */
+static inline void
+info_append_uint(struct info_handler *info, const char *key, uint64_t value)
+{
+ return info->vtab->append_uint(info, key, value);
+}
+
+/**
+ * Associates zero-terminated string with @a key in the current
+ * associative array.
+ * @pre associative array is started.
*/
static inline void
info_append_str(struct info_handler *info, const char *key,
- const char *value)
+ const char *value)
{
return info->vtab->append_str(info, key, value);
}
@@ -156,10 +160,7 @@ info_append_str(struct info_handler *info, const char *key,
/**
* Associates double value with @a key in the current associative
* array.
- * @param info box.info() adapter.
- * @param key key.
- * @param value value.
- * @throws C++ exception on OOM, see info.h comments.
+ * @pre associative array is started.
*/
static inline void
info_append_double(struct info_handler *info, const char *key,
@@ -168,11 +169,9 @@ info_append_double(struct info_handler *info, const char *key,
return info->vtab->append_double(info, key, value);
}
-/*
+/**
* Associates a new associative array with @a key.
- * @param info box.info() adapter.
- * @param key key.
- * @throws C++ exception on OOM, see info.h comments.
+ * @pre associative array is started.
*/
static inline void
info_table_begin(struct info_handler *info, const char *key)
@@ -180,10 +179,9 @@ info_table_begin(struct info_handler *info, const char *key)
return info->vtab->begin_table(info, key);
}
-/*
+/**
* Finishes the current active associative array.
- * @param info box.info() adapter
- * @throws C++ exception on OOM, see info.h comments.
+ * @pre associative array is started.
*/
static inline void
info_table_end(struct info_handler *info)
@@ -195,4 +193,4 @@ info_table_end(struct info_handler *info)
} /* extern "C" */
#endif /* defined(__cplusplus) */
-#endif /* INCLUDES_TARANTOOL_BOX_INFO_H */
+#endif /* INCLUDES_TARANTOOL_INFO_H */
diff --git a/src/lua/info.c b/src/lua/info.c
new file mode 100644
index 000000000..f28cc48f7
--- /dev/null
+++ b/src/lua/info.c
@@ -0,0 +1,118 @@
+/*
+ * Copyright 2010-2018, 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 "lua/info.h"
+#include <info.h>
+#include "lua/utils.h"
+
+static void
+luaT_info_begin(struct info_handler *info)
+{
+ lua_State *L = (lua_State *) info->ctx;
+ lua_newtable(L);
+}
+
+static void
+luaT_info_end(struct info_handler *info)
+{
+ (void) info;
+}
+
+static void
+luaT_info_begin_table(struct info_handler *info, const char *key)
+{
+ lua_State *L = (lua_State *) info->ctx;
+ lua_pushstring(L, key);
+ lua_newtable(L);
+}
+
+static void
+luaT_info_end_table(struct info_handler *info)
+{
+ lua_State *L = (lua_State *) info->ctx;
+ lua_settable(L, -3);
+}
+
+static void
+luaT_info_append_double(struct info_handler *info,
+ const char *key, double value)
+{
+ lua_State *L = (lua_State *) info->ctx;
+ lua_pushstring(L, key);
+ lua_pushnumber(L, value);
+ lua_settable(L, -3);
+}
+
+static void
+luaT_info_append_int(struct info_handler *info, const char *key,
+ int64_t value)
+{
+ lua_State *L = (lua_State *) info->ctx;
+ lua_pushstring(L, key);
+ luaL_pushint64(L, value);
+ lua_settable(L, -3);
+}
+
+static void
+luaT_info_append_uint(struct info_handler *info, const char *key,
+ uint64_t value)
+{
+ lua_State *L = (lua_State *) info->ctx;
+ lua_pushstring(L, key);
+ luaL_pushuint64(L, value);
+ lua_settable(L, -3);
+}
+
+static void
+luaT_info_append_str(struct info_handler *info, const char *key,
+ const char *value)
+{
+ lua_State *L = (lua_State *) info->ctx;
+ lua_pushstring(L, key);
+ lua_pushstring(L, value);
+ lua_settable(L, -3);
+}
+
+void
+luaT_info_handler_create(struct info_handler *h, struct lua_State *L)
+{
+ static struct info_handler_vtab lua_vtab = {
+ .begin = luaT_info_begin,
+ .end = luaT_info_end,
+ .begin_table = luaT_info_begin_table,
+ .end_table = luaT_info_end_table,
+ .append_int = luaT_info_append_int,
+ .append_uint = luaT_info_append_uint,
+ .append_str = luaT_info_append_str,
+ .append_double = luaT_info_append_double
+ };
+ h->vtab = &lua_vtab;
+ h->ctx = L;
+}
diff --git a/src/lua/info.h b/src/lua/info.h
new file mode 100644
index 000000000..712d0f78d
--- /dev/null
+++ b/src/lua/info.h
@@ -0,0 +1,49 @@
+#ifndef INCLUDES_TARANTOOL_LUA_INFO_H
+#define INCLUDES_TARANTOOL_LUA_INFO_H
+
+/*
+ * Copyright 2010-2016, 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.
+ */
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct lua_State;
+struct info_handler;
+
+void
+luaT_info_handler_create(struct info_handler *h, struct lua_State *L);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* INCLUDES_TARANTOOL_LUA_INFO_H */
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 01/11] box: move info_handler interface into src/info
2018-11-30 15:39 ` [PATCH 01/11] box: move info_handler interface into src/info Vladislav Shpilevoy
@ 2018-12-03 11:05 ` Vladimir Davydov
2018-12-03 21:48 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-03 20:41 ` [tarantool-patches] " Konstantin Osipov
1 sibling, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 11:05 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:31PM +0300, Vladislav Shpilevoy wrote:
> Box/info.h defines info_handler interface with a set
> of virtual functions. It allows to hide Lua from code
> not depending on this language, and is used in things
> like index:info(), box.info() to build Lua table with
> some info. But it does not depend on box/ so move it
> to src/. And alongside apply a bit refactoring: remove
> useless comments, comply with line width etc.
>
> Also, this API is needed for the forthcoming SWIM
> module which is going to be placed into src/.
>
> Needed for #3234
> ---
> src/CMakeLists.txt | 1 +
> src/box/lua/index.c | 4 +-
> src/box/lua/info.c | 78 +---------------------------
> src/box/lua/sql.c | 4 +-
> src/box/lua/stat.c | 4 +-
> src/box/sql.c | 2 +-
> src/{box => }/info.h | 84 +++++++++++++++---------------
> src/lua/info.c | 118 +++++++++++++++++++++++++++++++++++++++++++
> src/lua/info.h | 49 ++++++++++++++++++
> 9 files changed, 218 insertions(+), 126 deletions(-)
> rename src/{box => }/info.h (72%)
> create mode 100644 src/lua/info.c
> create mode 100644 src/lua/info.h
This patch has nothing to do with the rest of the series, which is
devoted to removing exceptions from evio, so it can be reviewed and
committed independently. Please submit the next version of this patch
separately.
> diff --git a/src/box/lua/index.c b/src/box/lua/index.c
> index ef89c397d..6265c044a 100644
> --- a/src/box/lua/index.c
> +++ b/src/box/lua/index.c
> @@ -30,10 +30,10 @@
> */
> #include "box/lua/index.h"
> #include "lua/utils.h"
> +#include "lua/info.h"
> +#include <info.h>
We typically use angular brackets <> only for system headers and
external libraries. For the rest of the source code, including src/,
we prefer quotes "".
> diff --git a/src/box/info.h b/src/info.h
> similarity index 72%
> rename from src/box/info.h
> rename to src/info.h
> index df82becd5..bf1809ca9 100644
> --- a/src/box/info.h
> +++ b/src/info.h
> @@ -1,7 +1,7 @@
> -#ifndef INCLUDES_TARANTOOL_BOX_INFO_H
> -#define INCLUDES_TARANTOOL_BOX_INFO_H
> +#ifndef INCLUDES_TARANTOOL_INFO_H
> +#define INCLUDES_TARANTOOL_INFO_H
> /*
> - * Copyright 2010-2017, Tarantool AUTHORS, please see AUTHORS file.
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
> *
> * Redistribution and use in source and binary forms, with or
> * without modification, are permitted provided that the following
> @@ -35,9 +35,10 @@
>
> /**
> * @file
> - * This module provides an adapter for Lua/C API to generate box.info()
> - * and index:info() introspection trees. The primary purpose of this
> - * adapter is to eliminate Engine <-> Lua interdependency.
> + * This module provides an adapter for Lua/C API to generate
> + * box.info() and index:info() introspection trees. The primary
> + * purpose of this adapter is to eliminate Engine <-> Lua
> + * interdependency.
> *
> * TREE STRUCTURE
> *
> @@ -57,20 +58,18 @@
> *
> * IMPLEMENTATION DETAILS
> *
> - * Current implementation calls Lua/C API under the hood without any
> - * pcall() wrapping. As you may now, idiosyncratic Lua/C API unwinds
> - * C stacks on errors in a way you can't handle in C. Please ensure that
> - * all blocks of code which call info_append_XXX() functions are
> - * exception/longjmp safe.
> + * Current implementation calls Lua/C API under the hood without
> + * any pcall() wrapping. As you may now, idiosyncratic Lua/C API
> + * unwinds C stacks on errors in a way you can't handle in C.
> + * Please ensure that all blocks of code which call
> + * info_append_XXX() functions are exception/longjmp safe.
> */
Please refrain from gratuitous changes. The comment looks just fine as
it is. And even if there is a typo or the text width is a little longer
than recommended, there is no need to pollute the history to fix that -
every such change makes git-blame painful.
Please remove all uncalled for changes from this patch as well as other
patches of the series. Do only as much as you need to proceed.
>
> #if defined(__cplusplus)
> extern "C" {
> #endif /* defined(__cplusplus) */
>
> -/**
> - * Virtual method table for struct info_handler.
> - */
> +/** Virtual method table for struct info_handler. */
> struct info_handler_vtab {
> /** The begin of document. */
> void (*begin)(struct info_handler *);
> @@ -86,13 +85,17 @@ struct info_handler_vtab {
> /** Set int64_t value. */
> void (*append_int)(struct info_handler *, const char *key,
> int64_t value);
> + /** Set uint64_t value. */
> + void (*append_uint)(struct info_handler *, const char *key,
> + uint64_t value);
Why would one need append_uint() when one can pass uint64_t to
append_int(). I deliberately removed append_uint() a while back.
I don't think there's any need to reintroduce this function.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 01/11] box: move info_handler interface into src/info
2018-12-03 11:05 ` Vladimir Davydov
@ 2018-12-03 21:48 ` Vladislav Shpilevoy
0 siblings, 0 replies; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-03 21:48 UTC (permalink / raw)
To: tarantool-patches, Vladimir Davydov
Hi! Thanks for the review!
On 03/12/2018 14:05, Vladimir Davydov wrote:
> On Fri, Nov 30, 2018 at 06:39:31PM +0300, Vladislav Shpilevoy wrote:
>> Box/info.h defines info_handler interface with a set
>> of virtual functions. It allows to hide Lua from code
>> not depending on this language, and is used in things
>> like index:info(), box.info() to build Lua table with
>> some info. But it does not depend on box/ so move it
>> to src/. And alongside apply a bit refactoring: remove
>> useless comments, comply with line width etc.
>>
>> Also, this API is needed for the forthcoming SWIM
>> module which is going to be placed into src/.
>>
>> Needed for #3234
>> ---
>> src/CMakeLists.txt | 1 +
>> src/box/lua/index.c | 4 +-
>> src/box/lua/info.c | 78 +---------------------------
>> src/box/lua/sql.c | 4 +-
>> src/box/lua/stat.c | 4 +-
>> src/box/sql.c | 2 +-
>> src/{box => }/info.h | 84 +++++++++++++++---------------
>> src/lua/info.c | 118 +++++++++++++++++++++++++++++++++++++++++++
>> src/lua/info.h | 49 ++++++++++++++++++
>> 9 files changed, 218 insertions(+), 126 deletions(-)
>> rename src/{box => }/info.h (72%)
>> create mode 100644 src/lua/info.c
>> create mode 100644 src/lua/info.h
>
> This patch has nothing to do with the rest of the series, which is
> devoted to removing exceptions from evio, so it can be reviewed and
> committed independently. Please submit the next version of this patch
> separately.
>
>> diff --git a/src/box/lua/index.c b/src/box/lua/index.c
>> index ef89c397d..6265c044a 100644
>> --- a/src/box/lua/index.c
>> +++ b/src/box/lua/index.c
>> @@ -30,10 +30,10 @@
>> */
>> #include "box/lua/index.h"
>> #include "lua/utils.h"
>> +#include "lua/info.h"
>> +#include <info.h>
>
> We typically use angular brackets <> only for system headers and
> external libraries. For the rest of the source code, including src/,
> we prefer quotes "".
Unfortunately, now it is the only way to resolve duplicate headers
like this:
src/info.h
src/lua/info.h
and
src/fiber.h
src/lua/fiber.h
and
src/fiber_channel.h
src/lua/fiber_channel.h
and
src/fiber_cond.h
src/lua/fiber_cond.h
and
src/fio.h
src/lua/fio.h
and
src/trigger.h
src/lua/trigger.h
etc
Also we use <> for many small/ header appearances.
So yes, you are right, we prefer, but "" is not mandatory. We use
<> when we are in src/lua/ and want to include header src/#some_file#
instead of src/lua/#some_file#. "" includes local header, but we
need global.
Many ways exist how to solve this problem but none of them shall be
part of this patch.
I remind that swim is going to be a separate module is src/lib and it
needs info to dump swim.info() from C to Lua without strict dependency
on Lua.
>> @@ -86,13 +85,17 @@ struct info_handler_vtab {
>> /** Set int64_t value. */
>> void (*append_int)(struct info_handler *, const char *key,
>> int64_t value);
>> + /** Set uint64_t value. */
>> + void (*append_uint)(struct info_handler *, const char *key,
>> + uint64_t value);
>
> Why would one need append_uint() when one can pass uint64_t to
> append_int(). I deliberately removed append_uint() a while back.
> I don't think there's any need to reintroduce this function.
>
I need it since SWIM has unsigned incarnation number adhered to
each cluster member. I want to show it in swim.info() at least
for tests so I returned append_uint back.
Talking about passing uint to append_int - I do not want to
cast. Also, API will not be symmetric as you usually require in
your own reviews, but as you wish.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [tarantool-patches] [PATCH 01/11] box: move info_handler interface into src/info
2018-11-30 15:39 ` [PATCH 01/11] box: move info_handler interface into src/info Vladislav Shpilevoy
2018-12-03 11:05 ` Vladimir Davydov
@ 2018-12-03 20:41 ` Konstantin Osipov
2018-12-03 21:48 ` [tarantool-patches] " Vladislav Shpilevoy
1 sibling, 1 reply; 37+ messages in thread
From: Konstantin Osipov @ 2018-12-03 20:41 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/11/30 18:41]:
> @@ -30,10 +30,10 @@
> */
> #include "box/lua/index.h"
> #include "lua/utils.h"
> +#include "lua/info.h"
> +#include <info.h>
Please prefer local headers whenever you can ("" rather than <>).
Your host platform can have a header file info.h at a system-wide
location. It's easy if it simply breaks your compile in a funny
way. Worse if it doesn't break.
> +++ b/src/box/lua/info.c
> @@ -32,7 +32,7 @@
> #define _GNU_SOURCE
> #endif
Since you are up to cleaning things up a bit, please get rid of
file-local _GNU_SOURCE. It should be part of cmake compile flags
for this file.
Otherwise the patch is OK to push.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 01/11] box: move info_handler interface into src/info
2018-12-03 20:41 ` [tarantool-patches] " Konstantin Osipov
@ 2018-12-03 21:48 ` Vladislav Shpilevoy
2018-12-04 8:52 ` Vladimir Davydov
0 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-03 21:48 UTC (permalink / raw)
To: tarantool-patches, Konstantin Osipov; +Cc: vdavydov.dev
Hi! Thanks for the review!
On 03/12/2018 23:41, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/11/30 18:41]:
>> @@ -30,10 +30,10 @@
>> */
>> #include "box/lua/index.h"
>> #include "lua/utils.h"
>> +#include "lua/info.h"
>> +#include <info.h>
>
> Please prefer local headers whenever you can ("" rather than <>).
> Your host platform can have a header file info.h at a system-wide
> location. It's easy if it simply breaks your compile in a funny
> way. Worse if it doesn't break.
Look at my answer to Vova.
>
>> +++ b/src/box/lua/info.c
>> @@ -32,7 +32,7 @@
>> #define _GNU_SOURCE
>> #endif
>
> Since you are up to cleaning things up a bit, please get rid of
> file-local _GNU_SOURCE. It should be part of cmake compile flags
> for this file.
Unfortunately, Vova required to remove all my cleanup so
I can not remove _GNU_SOURCE in scope of this patch, sorry.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 01/11] box: move info_handler interface into src/info
2018-12-03 21:48 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-04 8:52 ` Vladimir Davydov
0 siblings, 0 replies; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-04 8:52 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Konstantin Osipov
On Tue, Dec 04, 2018 at 12:48:25AM +0300, Vladislav Shpilevoy wrote:
> > > +++ b/src/box/lua/info.c
> > > @@ -32,7 +32,7 @@
> > > #define _GNU_SOURCE
> > > #endif
> >
> > Since you are up to cleaning things up a bit, please get rid of
> > file-local _GNU_SOURCE. It should be part of cmake compile flags
> > for this file.
>
> Unfortunately, Vova required to remove all my cleanup so
> I can not remove _GNU_SOURCE in scope of this patch, sorry.
Don't be ridiculous. Removing a stale definition along the way would be
fine with me. I detested the way you reformatted comments and refactored
the code for no apparent reason. I squashed removal of this pointless
_GNU_SOURCE into your patch before merging it.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 10/11] evio: remove exceptions
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
2018-11-30 15:39 ` [PATCH 01/11] box: move info_handler interface into src/info Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-11-30 15:39 ` [PATCH 11/11] evio: turn into C Vladislav Shpilevoy
` (9 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Remove them to be able to convert evio to C - final
step to make it usable in swim.
Needed for #3234
---
src/box/iproto.cc | 41 ++++++++++------------
src/coio.cc | 8 +++--
src/evio.cc | 87 ++++++++++++++++++++++++-----------------------
src/evio.h | 13 ++++---
4 files changed, 73 insertions(+), 76 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index dd76e28bd..7e5110848 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -2014,29 +2014,24 @@ iproto_do_cfg_f(struct cbus_call_msg *m)
{
struct iproto_cfg_msg *cfg_msg = (struct iproto_cfg_msg *) m;
int old;
- try {
- switch (cfg_msg->op) {
- case IPROTO_CFG_MSG_MAX:
- cpipe_set_max_input(&tx_pipe,
- cfg_msg->iproto_msg_max / 2);
- old = iproto_msg_max;
- iproto_msg_max = cfg_msg->iproto_msg_max;
- if (old < iproto_msg_max)
- iproto_resume();
- break;
- case IPROTO_CFG_LISTEN:
- if (evio_service_is_active(&binary))
- evio_service_stop(&binary);
- if (cfg_msg->uri != NULL) {
- evio_service_bind(&binary, cfg_msg->uri);
- evio_service_listen(&binary);
- }
- break;
- default:
- unreachable();
- }
- } catch (Exception *e) {
- return -1;
+ switch (cfg_msg->op) {
+ case IPROTO_CFG_MSG_MAX:
+ cpipe_set_max_input(&tx_pipe, cfg_msg->iproto_msg_max / 2);
+ old = iproto_msg_max;
+ iproto_msg_max = cfg_msg->iproto_msg_max;
+ if (old < iproto_msg_max)
+ iproto_resume();
+ break;
+ case IPROTO_CFG_LISTEN:
+ if (evio_service_is_active(&binary))
+ evio_service_stop(&binary);
+ if (cfg_msg->uri != NULL &&
+ (evio_service_bind(&binary, cfg_msg->uri) != 0 ||
+ evio_service_listen(&binary) != 0))
+ return -1;
+ break;
+ default:
+ unreachable();
}
return 0;
}
diff --git a/src/coio.cc b/src/coio.cc
index a888a54dd..1615d075a 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -77,7 +77,8 @@ coio_connect_addr(struct ev_io *coio, struct sockaddr *addr,
socklen_t len, ev_tstamp timeout)
{
ev_loop *loop = loop();
- evio_socket(coio, addr->sa_family, SOCK_STREAM, 0);
+ if (evio_socket(coio, addr->sa_family, SOCK_STREAM, 0) != 0)
+ diag_raise();
if (sio_connect(coio->fd, addr, len) == 0)
return 0;
auto coio_guard = make_scoped_guard([=]{ evio_close(loop, coio); });
@@ -646,8 +647,9 @@ coio_service_init(struct coio_service *service, const char *name,
void
coio_service_start(struct evio_service *service, const char *uri)
{
- evio_service_bind(service, uri);
- evio_service_listen(service);
+ if (evio_service_bind(service, uri) != 0 ||
+ evio_service_listen(service) != 0)
+ diag_raise();
}
void
diff --git a/src/evio.cc b/src/evio.cc
index 166ba7f95..2bfd0a2e5 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -29,14 +29,12 @@
* SUCH DAMAGE.
*/
#include "evio.h"
-#include "scoped_guard.h"
#include <stdio.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <arpa/inet.h>
-
#include <trivia/util.h>
#include "exception.h"
@@ -51,18 +49,19 @@ evio_close(ev_loop *loop, struct ev_io *evio)
}
}
-void
+int
evio_socket(struct ev_io *evio, int domain, int type, int protocol)
{
assert(! evio_has_fd(evio));
evio->fd = sio_socket(domain, type, protocol);
if (evio->fd < 0)
- diag_raise();
+ return -1;
if (evio_setsockopt_client(evio->fd, domain, type) != 0) {
close(evio->fd);
ev_io_set(evio, -1, 0);
- diag_raise();
+ return -1;
}
+ return 0;
}
static int
@@ -121,14 +120,14 @@ evio_setsockopt_client(int fd, int family, int type)
}
/** Set options for server sockets. */
-static void
+static int
evio_setsockopt_server(int fd, int family, int type)
{
int on = 1;
if (sio_setfl(fd, O_NONBLOCK) != 0)
- diag_raise();
+ return -1;
if (sio_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)) != 0)
- diag_raise();
+ return -1;
/*
* Send all buffered messages on socket before take
@@ -137,10 +136,11 @@ evio_setsockopt_server(int fd, int family, int type)
struct linger linger = { 0, 0 };
if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER, &linger,
sizeof(linger)) != 0)
- diag_raise();
+ return -1;
if (type == SOCK_STREAM && family != AF_UNIX &&
evio_setsockopt_keeping(fd) != 0)
- diag_raise();
+ return -1;
+ return 0;
}
/**
@@ -220,12 +220,8 @@ err:
return -1;
}
-/**
- * Try to bind on the configured port.
- *
- * Throws an exception if error.
- */
-static void
+/** Try to bind on the configured port. */
+static int
evio_service_bind_addr(struct evio_service *service)
{
say_debug("%s: binding to %s...", service->name,
@@ -233,23 +229,22 @@ evio_service_bind_addr(struct evio_service *service)
/* Create a socket. */
int fd = sio_socket(service->addr.sa_family, SOCK_STREAM, IPPROTO_TCP);
if (fd < 0)
- diag_raise();
-
- auto fd_guard = make_scoped_guard([=]{ close(fd); });
-
- evio_setsockopt_server(fd, service->addr.sa_family, SOCK_STREAM);
+ return -1;
+ if (evio_setsockopt_server(fd, service->addr.sa_family,
+ SOCK_STREAM) != 0)
+ goto error;
if (sio_bind(fd, &service->addr, service->addr_len)) {
if (errno != EADDRINUSE)
- diag_raise();
+ goto error;
if (evio_service_reuse_addr(service, fd))
- diag_raise();
+ goto error;
if (sio_bind(fd, &service->addr, service->addr_len)) {
if (errno == EADDRINUSE) {
diag_set(SocketError, sio_socketname(fd),
"bind");
}
- diag_raise();
+ goto error;
}
}
@@ -258,11 +253,13 @@ evio_service_bind_addr(struct evio_service *service)
/* Register the socket in the event loop. */
ev_io_set(&service->ev, fd, EV_READ);
-
- fd_guard.is_active = false;
+ return 0;
+error:
+ close(fd);
+ return -1;
}
-void
+int
evio_service_listen(struct evio_service *service)
{
say_debug("%s: listening on %s...", service->name,
@@ -270,8 +267,9 @@ evio_service_listen(struct evio_service *service)
int fd = service->ev.fd;
if (sio_listen(fd) < 0)
- diag_raise();
+ return -1;
ev_io_start(service->loop, &service->ev);
+ return 0;
}
void
@@ -294,13 +292,14 @@ evio_service_init(ev_loop *loop, struct evio_service *service, const char *name,
service->ev.data = service;
}
-void
+int
evio_service_bind(struct evio_service *service, const char *uri)
{
struct uri u;
if (uri_parse(&u, uri) || u.service == NULL) {
- tnt_raise(SocketError, sio_socketname(-1),
- "invalid uri for bind: %s", uri);
+ diag_set(SocketError, sio_socketname(-1),
+ "invalid uri for bind: %s", uri);
+ return -1;
}
snprintf(service->serv, sizeof(service->serv), "%.*s",
@@ -334,25 +333,27 @@ evio_service_bind(struct evio_service *service, const char *uri)
*/
if (getaddrinfo(*service->host ? service->host : NULL, service->serv,
&hints, &res) != 0 || res == NULL) {
- tnt_raise(SocketError, sio_socketname(-1),
- "can't resolve uri for bind");
+ diag_set(SocketError, sio_socketname(-1),
+ "can't resolve uri for bind");
+ return -1;
}
- auto addrinfo_guard = make_scoped_guard([=]{ freeaddrinfo(res); });
for (struct addrinfo *ai = res; ai != NULL; ai = ai->ai_next) {
memcpy(&service->addr, ai->ai_addr, ai->ai_addrlen);
service->addr_len = ai->ai_addrlen;
- try {
- return evio_service_bind_addr(service);
- } catch (SocketError *e) {
- say_error("%s: failed to bind on %s: %s", service->name,
- sio_strfaddr(ai->ai_addr, ai->ai_addrlen),
- e->get_errmsg());
- /* ignore */
+ if (evio_service_bind_addr(service) == 0) {
+ freeaddrinfo(res);
+ return 0;
}
+ struct error *e = diag_last_error(diag_get());
+ say_error("%s: failed to bind on %s: %s", service->name,
+ sio_strfaddr(ai->ai_addr, ai->ai_addrlen), e->errmsg);
+ /* ignore */
}
- tnt_raise(SocketError, sio_socketname(-1), "%s: failed to bind",
- service->name);
+ freeaddrinfo(res);
+ diag_set(SocketError, sio_socketname(-1), "%s: failed to bind",
+ service->name);
+ return -1;
}
void
diff --git a/src/evio.h b/src/evio.h
index ae2b9b9a8..1446a5617 100644
--- a/src/evio.h
+++ b/src/evio.h
@@ -39,7 +39,7 @@
#include "sio.h"
#include "uri.h"
/**
- * Exception-aware way to add a socket to the event loop.
+ * A way to add a socket to the event loop.
*
* Coroutines/fibers are not used for port listeners since
* listener's job is usually simple and only involves creating a
@@ -80,9 +80,8 @@ struct evio_service
/**
* A callback invoked on every accepted client socket.
- * It's OK to throw an exception in the callback:
- * when it happens, the exception is logged, and the
- * accepted socket is closed.
+ * If returns != 0, the error is logged, and the accepted
+ * socket is closed.
*/
evio_accept_f on_accept;
void *on_accept_param;
@@ -98,11 +97,11 @@ evio_service_init(ev_loop *loop, struct evio_service *service, const char *name,
evio_accept_f on_accept, void *on_accept_param);
/** Bind service to specified uri. */
-void
+int
evio_service_bind(struct evio_service *service, const char *uri);
/** Listen on bounded socket. */
-void
+int
evio_service_listen(struct evio_service *service);
/** If started, stop event flow and close the acceptor socket. */
@@ -113,7 +112,7 @@ evio_service_stop(struct evio_service *service);
* Create a client socket. Sets keepalive, nonblock and nodelay
* options.
*/
-void
+int
evio_socket(struct ev_io *coio, int domain, int type, int protocol);
/** Close evio service socket and detach from event loop. */
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 11/11] evio: turn into C
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
2018-11-30 15:39 ` [PATCH 01/11] box: move info_handler interface into src/info Vladislav Shpilevoy
2018-11-30 15:39 ` [PATCH 10/11] evio: remove exceptions Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-11-30 15:39 ` [PATCH 02/11] sio: remove unused functions, restyle code Vladislav Shpilevoy
` (8 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Evio now officially can be used from C code.
Needed for #3234
---
src/CMakeLists.txt | 2 +-
src/{evio.cc => evio.c} | 0
src/evio.h | 9 +++++++++
3 files changed, 10 insertions(+), 1 deletion(-)
rename src/{evio.cc => evio.c} (100%)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 77353b244..58031d671 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -89,7 +89,7 @@ set (core_sources
fiber_channel.c
latch.c
sio.c
- evio.cc
+ evio.c
coio.cc
coio_task.c
coio_file.c
diff --git a/src/evio.cc b/src/evio.c
similarity index 100%
rename from src/evio.cc
rename to src/evio.c
diff --git a/src/evio.h b/src/evio.h
index 1446a5617..574c1f807 100644
--- a/src/evio.h
+++ b/src/evio.h
@@ -38,6 +38,11 @@
#include "tarantool_ev.h"
#include "sio.h"
#include "uri.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
/**
* A way to add a socket to the event loop.
*
@@ -150,4 +155,8 @@ evio_timeout_update(ev_loop *loop, ev_tstamp start, ev_tstamp *delay)
int
evio_setsockopt_client(int fd, int family, int type);
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
#endif /* TARANTOOL_EVIO_H_INCLUDED */
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 02/11] sio: remove unused functions, restyle code
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (2 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 11/11] evio: turn into C Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-12-03 12:28 ` Vladimir Davydov
2018-11-30 15:39 ` [PATCH 03/11] sio: remove exceptions Vladislav Shpilevoy
` (7 subsequent siblings)
11 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Sio code is dirty and full of unused functions. Lets
clean it a bit: move comments to declarations, add
explicit 'struct' to struct types, remove unused code,
put function return values onto separate lines, remove
unused headers, reduce indentation level.
This is a preparation patch to make sio C.
---
src/evio.cc | 9 +-
src/sio.cc | 335 +++++++++++-----------------------------------------
src/sio.h | 142 +++++++++++++---------
3 files changed, 155 insertions(+), 331 deletions(-)
diff --git a/src/evio.cc b/src/evio.cc
index a6ac65daf..a8475e0d1 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -111,7 +111,7 @@ evio_setsockopt_client(int fd, int family, int type)
{
int on = 1;
/* In case this throws, the socket is not leaked. */
- if (sio_setfl(fd, O_NONBLOCK, on))
+ if (sio_setfl(fd, O_NONBLOCK) != 0)
diag_raise();
if (type == SOCK_STREAM && family != AF_UNIX) {
/*
@@ -136,7 +136,7 @@ evio_setsockopt_server(int fd, int family, int type)
{
int on = 1;
/* In case this throws, the socket is not leaked. */
- if (sio_setfl(fd, O_NONBLOCK, on))
+ if (sio_setfl(fd, O_NONBLOCK) != 0)
diag_raise();
/* Allow reuse local adresses. */
if (sio_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
@@ -294,10 +294,7 @@ evio_service_listen(struct evio_service *service)
sio_strfaddr(&service->addr, service->addr_len));
int fd = service->ev.fd;
- if (sio_listen(fd)) {
- /* raise for addr in use to */
- tnt_raise(SocketError, sio_socketname(fd), "listen");
- }
+ sio_listen(fd);
ev_io_start(service->loop, &service->ev);
}
diff --git a/src/sio.cc b/src/sio.cc
index 0475b9e83..b2f05e5c8 100644
--- a/src/sio.cc
+++ b/src/sio.cc
@@ -33,23 +33,13 @@
#include <sys/un.h>
#include <sys/uio.h>
#include <errno.h>
-#include <stdio.h>
#include <limits.h>
#include <netinet/in.h> /* TCP_NODELAY */
#include <netinet/tcp.h> /* TCP_NODELAY */
-#include <arpa/inet.h> /* inet_ntoa */
-#include <poll.h>
-#include <unistd.h> /* lseek for sending file */
-#include <sys/stat.h> /* fstat for sending file */
-#ifdef TARGET_OS_LINUX
-#include <sys/sendfile.h> /* sendfile system call */
-#endif /* #ifdef TARGET_OS_LINUX */
-
#include "say.h"
#include "trivia/util.h"
#include "exception.h"
-/** Pretty print socket name and peer (for exceptions) */
const char *
sio_socketname(int fd)
{
@@ -62,10 +52,9 @@ sio_socketname(int fd)
socklen_t addrlen = sizeof(addr);
int rc = getsockname(fd, (struct sockaddr *) &addr, &addrlen);
if (rc == 0) {
- n += snprintf(name + n,
- sizeof(name) - n, ", aka %s",
- sio_strfaddr((struct sockaddr *)&addr,
- addrlen));
+ n += snprintf(name + n, sizeof(name) - n, ", aka %s",
+ sio_strfaddr((struct sockaddr *)&addr,
+ addrlen));
}
addrlen = sizeof(addr);
rc = getpeername(fd, (struct sockaddr *) &addr, &addrlen);
@@ -73,7 +62,7 @@ sio_socketname(int fd)
n += snprintf(name + n, sizeof(name) - n,
", peer of %s",
sio_strfaddr((struct sockaddr *)&addr,
- addrlen));
+ addrlen));
}
}
/*
@@ -84,7 +73,8 @@ sio_socketname(int fd)
return name;
}
-/** Get a string representation of a socket option name,
+/**
+ * Get a string representation of a socket option name,
* for logging.
*/
static const char *
@@ -107,27 +97,18 @@ sio_option_name(int option)
#undef CASE_OPTION
}
-/** shut down part of a full-duplex connection */
-int
-sio_shutdown(int fd, int how)
-{
- int rc = shutdown(fd, how);
- if (rc < 0)
- diag_set(SocketError, sio_socketname(fd), "shutdown");
- return rc;
-}
-
-/** Try to automatically configure a listen backlog.
+/**
+ * Try to automatically configure a listen backlog.
* On Linux, use the system setting, which defaults
* to 128. This way a system administrator can tune
* the backlog as needed. On other systems, use SOMAXCONN.
*/
-int
+static int
sio_listen_backlog()
{
#ifdef TARGET_OS_LINUX
FILE *proc = fopen("/proc/sys/net/core/somaxconn", "r");
- if (proc) {
+ if (proc != NULL) {
int backlog;
int rc = fscanf(proc, "%d", &backlog);
fclose(proc);
@@ -138,11 +119,10 @@ sio_listen_backlog()
return SOMAXCONN;
}
-/** Create a TCP socket. */
int
sio_socket(int domain, int type, int protocol)
{
- /* AF_UNIX can't use tcp protocol */
+ /* AF_UNIX can't use a protocol. */
if (domain == AF_UNIX)
protocol = 0;
int fd = socket(domain, type, protocol);
@@ -151,60 +131,55 @@ sio_socket(int domain, int type, int protocol)
return fd;
}
-/** Get socket flags, raise an exception if error. */
int
sio_getfl(int fd)
{
int flags = fcntl(fd, F_GETFL, 0);
- if (flags < 0)
+ if (flags < 0) {
diag_set(SocketError, sio_socketname(fd),
"fcntl(..., F_GETFL, ...)");
+ }
return flags;
}
-/** Set socket flags, raise an exception if error. */
int
-sio_setfl(int fd, int flag, int on)
+sio_setfl(int fd, int flag)
{
int flags = sio_getfl(fd);
if (flags < 0)
return flags;
- flags = fcntl(fd, F_SETFL, on ? flags | flag : flags & ~flag);
- if (flags < 0)
+ flags = fcntl(fd, F_SETFL, flags | flag);
+ if (flags < 0) {
diag_set(SocketError, sio_socketname(fd),
"fcntl(..., F_SETFL, ...)");
+ }
return flags;
}
-/** Set an option on a socket.
- * @retval -1 on error
- * */
int
sio_setsockopt(int fd, int level, int optname,
const void *optval, socklen_t optlen)
{
int rc = setsockopt(fd, level, optname, optval, optlen);
- if (rc) {
+ if (rc != 0) {
diag_set(SocketError, sio_socketname(fd),
"setsockopt(%s)", sio_option_name(optname));
}
return rc;
}
-/** Get a socket option value. */
int
sio_getsockopt(int fd, int level, int optname,
void *optval, socklen_t *optlen)
{
int rc = getsockopt(fd, level, optname, optval, optlen);
- if (rc) {
+ if (rc != 0) {
diag_set(SocketError, sio_socketname(fd), "getsockopt(%s)",
sio_option_name(optname));
}
return rc;
}
-/** Connect a client socket to a server. */
int
sio_connect(int fd, struct sockaddr *addr, socklen_t addrlen)
{
@@ -217,7 +192,6 @@ sio_connect(int fd, struct sockaddr *addr, socklen_t addrlen)
return rc;
}
-/** Bind a socket to the given address. */
int
sio_bind(int fd, struct sockaddr *addr, socklen_t addrlen)
{
@@ -227,71 +201,62 @@ sio_bind(int fd, struct sockaddr *addr, socklen_t addrlen)
return rc;
}
-/** Mark a socket as accepting connections. */
int
sio_listen(int fd)
{
int rc = listen(fd, sio_listen_backlog());
- if (rc < 0 && errno != EADDRINUSE)
+ if (rc < 0)
tnt_raise(SocketError, sio_socketname(fd), "listen");
return rc;
}
-/** Accept a client connection on a server socket. */
int
sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen)
{
/* Accept a connection. */
int newfd = accept(fd, addr, addrlen);
- if (newfd < 0 &&
- (errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR))
+ if (newfd < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
+ errno != EINTR)
tnt_raise(SocketError, sio_socketname(fd), "accept");
return newfd;
}
-/** Read up to 'count' bytes from a socket. */
ssize_t
sio_read(int fd, void *buf, size_t count)
{
ssize_t n = read(fd, buf, count);
- if (n < 0) {
- if (errno == EWOULDBLOCK)
- errno = EINTR;
- switch (errno) {
- case EAGAIN:
- case EINTR:
- break;
- /*
- * Happens typically when the client closes
- * socket on timeout without reading the previous
- * query's response completely. Treat the same as
- * EOF.
- */
- case ECONNRESET:
- errno = 0;
- n = 0;
- break;
- default:
- tnt_raise(SocketError, sio_socketname(fd),
- "read(%zd)", count);
- }
+ if (n >= 0)
+ return n;
+ if (errno == EWOULDBLOCK)
+ errno = EINTR;
+ switch (errno) {
+ case EAGAIN:
+ case EINTR:
+ break;
+ /*
+ * Happens typically when the client closes socket on
+ * timeout without reading the previous query's response
+ * completely. Treat the same as EOF.
+ */
+ case ECONNRESET:
+ errno = 0;
+ n = 0;
+ break;
+ default:
+ tnt_raise(SocketError, sio_socketname(fd), "read(%zd)", count);
}
return n;
}
-/** Write up to 'count' bytes to a socket. */
ssize_t
sio_write(int fd, const void *buf, size_t count)
{
ssize_t n = write(fd, buf, count);
- if (n < 0 && errno != EAGAIN &&
- errno != EWOULDBLOCK && errno != EINTR)
- tnt_raise(SocketError, sio_socketname(fd),
- "write(%zd)", count);
+ if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR)
+ tnt_raise(SocketError, sio_socketname(fd), "write(%zd)", count);
return n;
}
-/** Write to a socket with iovec. */
ssize_t
sio_writev(int fd, const struct iovec *iov, int iovcnt)
{
@@ -305,225 +270,59 @@ sio_writev(int fd, const struct iovec *iov, int iovcnt)
return n;
}
-/** Blocking I/O writev */
-ssize_t
-sio_writev_all(int fd, struct iovec *iov, int iovcnt)
-{
- ssize_t bytes_total = 0;
- struct iovec *iovend = iov + iovcnt;
- while (1) {
- int cnt = iovend - iov;
- if (cnt > IOV_MAX)
- cnt = IOV_MAX;
- ssize_t write_res = writev(fd, iov, cnt);
- if (write_res < 0) {
- if (errno == EINTR)
- continue;
- tnt_raise(SocketError, sio_socketname(fd),
- "writev(%d)", cnt);
- }
- size_t bytes_written = (size_t)write_res;
- bytes_total += bytes_written;
- /*
- * Check for iov < iovend, since otherwise
- * if iovend->iov_len is 0, iov may go beyond
- * iovend
- */
- while (bytes_written >= iov->iov_len) {
- bytes_written -= (iov++)->iov_len;
- if (iov == iovend)
- break;
- }
- if (iov == iovend)
- break;
- iov->iov_base = (char *) iov->iov_base + bytes_written;
- iov->iov_len -= bytes_written;
- }
- return bytes_total;
-}
-
-ssize_t
-sio_readn_ahead(int fd, void *buf, size_t count, size_t buf_size)
-{
- size_t read_count = 0;
- while (read_count < count) {
- ssize_t read_res = read(fd, (char *) buf + read_count,
- buf_size - read_count);
- if (read_res < 0 && (errno == EWOULDBLOCK ||
- errno == EINTR || errno == EAGAIN))
- continue;
-
- if (read_res <= 0)
- tnt_raise(SocketError, sio_socketname(fd),
- "read (%zd)", count);
-
- read_count += read_res;
- }
- return read_count;
-}
-
-ssize_t
-sio_writen(int fd, const void *buf, size_t count)
-{
- size_t write_count = 0;
- while (write_count < count) {
- ssize_t write_res = write(fd, (char *) buf + write_count,
- count - write_count);
- if (write_res < 0 && (errno == EWOULDBLOCK ||
- errno == EINTR || errno == EAGAIN))
- continue;
-
- if (write_res <= 0)
- tnt_raise(SocketError, sio_socketname(fd),
- "write (%zd)", count);
-
- write_count += write_res;
- }
- return write_count;
-}
-
-static inline off_t
-sio_lseek(int fd, off_t offset, int whence)
-{
- off_t res = lseek(fd, offset, whence);
- if (res == -1)
- tnt_raise(SocketError, sio_socketname(fd),
- "lseek");
- return res;
-}
-
-#if defined(HAVE_SENDFILE_LINUX)
-ssize_t
-sio_sendfile(int sock_fd, int file_fd, off_t *offset, size_t size)
-{
- ssize_t send_res = sendfile(sock_fd, file_fd, offset, size);
- if (send_res < 0 || (size_t)send_res < size)
- tnt_raise(SocketError, sio_socketname(sock_fd),
- "sendfile");
- return send_res;
-}
-#else
-ssize_t
-sio_sendfile(int sock_fd, int file_fd, off_t *offset, size_t size)
-{
- if (offset)
- sio_lseek(file_fd, *offset, SEEK_SET);
-
- const size_t buffer_size = 8192;
- char buffer[buffer_size];
- size_t bytes_sent = 0;
- while (bytes_sent < size) {
- size_t to_send_now = MIN(size - bytes_sent, buffer_size);
- ssize_t n = sio_read(file_fd, buffer, to_send_now);
- sio_writen(sock_fd, buffer, n);
- bytes_sent += n;
- }
-
- if (offset)
- lseek(file_fd, *offset, SEEK_SET);
-
- return bytes_sent;
-}
-#endif
-
-ssize_t
-sio_recvfile(int sock_fd, int file_fd, off_t *offset, size_t size)
-{
- if (offset)
- sio_lseek(file_fd, *offset, SEEK_SET);
-
- const size_t buffer_size = 8192;
- char buffer[buffer_size];
- size_t bytes_read = 0;
- while (bytes_read < size) {
- size_t to_read_now = MIN(size - bytes_read, buffer_size);
- ssize_t n = sio_read(sock_fd, buffer, to_read_now);
- if (n < 0)
- return -1;
- sio_writen(file_fd, buffer, n);
- bytes_read += n;
- }
-
- if (offset)
- sio_lseek(file_fd, *offset, SEEK_SET);
-
- return bytes_read;
-}
-
-/** Send a message on a socket. */
ssize_t
sio_sendto(int fd, const void *buf, size_t len, int flags,
const struct sockaddr *dest_addr, socklen_t addrlen)
{
ssize_t n = sendto(fd, buf, len, flags, (struct sockaddr*)dest_addr,
addrlen);
- if (n < 0 && errno != EAGAIN &&
- errno != EWOULDBLOCK && errno != EINTR)
- tnt_raise(SocketError, sio_socketname(fd),
- "sendto(%zd)", len);
+ if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR)
+ tnt_raise(SocketError, sio_socketname(fd), "sendto(%zd)", len);
return n;
}
-/** Receive a message on a socket. */
ssize_t
sio_recvfrom(int fd, void *buf, size_t len, int flags,
struct sockaddr *src_addr, socklen_t *addrlen)
{
ssize_t n = recvfrom(fd, buf, len, flags, (struct sockaddr*)src_addr,
addrlen);
- if (n < 0 && errno != EAGAIN &&
- errno != EWOULDBLOCK && errno != EINTR)
- tnt_raise(SocketError, sio_socketname(fd),
- "recvfrom(%zd)", len);
+ if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR) {
+ tnt_raise(SocketError, sio_socketname(fd), "recvfrom(%zd)",
+ len);
+ }
return n;
}
-/** Get socket peer name. */
int
sio_getpeername(int fd, struct sockaddr *addr, socklen_t *addrlen)
{
- if (getpeername(fd, addr, addrlen) < 0) {
+ int rc = getpeername(fd, addr, addrlen);
+ if (rc < 0)
say_syserror("getpeername");
- return -1;
- }
- /* XXX: I've no idea where this is copy-pasted from. */
- /*
- if (addr->sin_addr.s_addr == 0) {
- say_syserror("getpeername: empty peer");
- return -1;
- }
- */
- return 0;
+ return rc;
}
-/** Pretty print a peer address. */
const char *
sio_strfaddr(struct sockaddr *addr, socklen_t addrlen)
{
static __thread char name[NI_MAXHOST + _POSIX_PATH_MAX + 2];
- switch(addr->sa_family) {
- case AF_UNIX:
- if (addrlen >= sizeof(sockaddr_un)) {
- snprintf(name, sizeof(name), "unix/:%s",
- ((struct sockaddr_un *)addr)->sun_path);
- } else {
- snprintf(name, sizeof(name),
- "unix/:(socket)");
- }
- break;
- default: {
- char host[NI_MAXHOST], serv[NI_MAXSERV];
- if (getnameinfo(addr, addrlen, host, sizeof(host),
- serv, sizeof(serv),
- NI_NUMERICHOST | NI_NUMERICSERV) == 0) {
- snprintf(name, sizeof(name),
- addr->sa_family == AF_INET
- ? "%s:%s" : "[%s]:%s", host, serv);
- } else {
- snprintf(name, sizeof(name), "(host):(port)");
- }
- break;
+ if (addr->sa_family == AF_UNIX) {
+ if (addrlen >= sizeof(struct sockaddr_un)) {
+ snprintf(name, sizeof(name), "unix/:%s",
+ ((struct sockaddr_un *)addr)->sun_path);
+ } else {
+ snprintf(name, sizeof(name), "unix/:(socket)");
}
+ return name;
+ }
+ char host[NI_MAXHOST], serv[NI_MAXSERV];
+ if (getnameinfo(addr, addrlen, host, sizeof(host), serv, sizeof(serv),
+ NI_NUMERICHOST | NI_NUMERICSERV) == 0) {
+ snprintf(name, sizeof(name), addr->sa_family == AF_INET ?
+ "%s:%s" : "[%s]:%s", host, serv);
+ } else {
+ snprintf(name, sizeof(name), "(host):(port)");
}
return name;
}
diff --git a/src/sio.h b/src/sio.h
index f728af547..d937cfd3d 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -48,22 +48,23 @@ extern "C" {
enum { SERVICE_NAME_MAXLEN = 32 };
+/** Pretty print a peer address. */
const char *
sio_strfaddr(struct sockaddr *addr, socklen_t addrlen);
+/** Get socket peer name. */
int
sio_getpeername(int fd, struct sockaddr *addr, socklen_t *addrlen);
/**
- * Advance write position in the iovec array
- * based on its current value and the number of
- * bytes written.
+ * Advance write position in the iovec array based on its current
+ * value and the number of bytes written.
*
- * @param[in] iov the vector being written with writev().
- * @param[in] nwr number of bytes written, @pre >= 0
- * @param[in,out] iov_len offset in iov[0];
+ * @param iov The vector written with writev().
+ * @param nwr Number of bytes written.
+ * @param[in, out] iov_len Offset in iov[0];
*
- * @return offset of iov[0] for the next write
+ * @retval Offset of iov[0] for the next write
*/
static inline int
sio_move_iov(struct iovec *iov, size_t nwr, size_t *iov_len)
@@ -92,88 +93,115 @@ sio_add_to_iov(struct iovec *iov, size_t size)
#if defined(__cplusplus)
} /* extern "C" */
-const char *sio_socketname(int fd);
-int sio_socket(int domain, int type, int protocol);
+/** Pretty format socket name and peer. */
+const char *
+sio_socketname(int fd);
+
+/**
+ * Create a socket. A wrapper for socket() function, but sets
+ * diagnostics on error.
+ */
+int
+sio_socket(int domain, int type, int protocol);
-int sio_shutdown(int fd, int how);
+/**
+ * Get file descriptor flags. A wrapper for fcntl(F_GETFL), but
+ * sets diagnostics on error.
+ */
+int
+sio_getfl(int fd);
-int sio_getfl(int fd);
-int sio_setfl(int fd, int flag, int on);
+/**
+ * Set a file descriptor flag. A wrapper for
+ * fcntl(F_SETFL, flag | fcntl(F_GETFL)), but sets diagnostics on
+ * error.
+ */
+int
+sio_setfl(int fd, int flag);
+/**
+ * Set an option on a socket. A wrapper for setsockopt(), but sets
+ * diagnostics on error.
+ */
int
sio_setsockopt(int fd, int level, int optname,
const void *optval, socklen_t optlen);
+
+/**
+ * Get a socket option value. A wrapper for setsockopt(), but sets
+ * diagnostics on error.
+ */
int
sio_getsockopt(int fd, int level, int optname,
void *optval, socklen_t *optlen);
-int sio_connect(int fd, struct sockaddr *addr, socklen_t addrlen);
-int sio_bind(int fd, struct sockaddr *addr, socklen_t addrlen);
-int sio_listen(int fd);
-int sio_listen_backlog();
-int sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen);
-
-ssize_t sio_read(int fd, void *buf, size_t count);
-
-ssize_t sio_write(int fd, const void *buf, size_t count);
-ssize_t sio_writev(int fd, const struct iovec *iov, int iovcnt);
+/**
+ * Connect a client socket to a server. A wrapper for connect(),
+ * but sets diagnostics on error except EINPROGRESS.
+ */
+int
+sio_connect(int fd, struct sockaddr *addr, socklen_t addrlen);
-ssize_t sio_write_total(int fd, const void *buf, size_t count, size_t total);
+/**
+ * Bind a socket to the given address. A wrapper for bind(), but
+ * sets diagnostics on error except EADDRINUSE.
+ */
+int
+sio_bind(int fd, struct sockaddr *addr, socklen_t addrlen);
/**
- * Read at least count and up to buf_size bytes from fd.
- * Throw exception on error or disconnect.
- *
- * @return the number of of bytes actually read.
+ * Mark a socket as accepting connections. A wrapper for listen(),
+ * but throws exception on error.
*/
-ssize_t
-sio_readn_ahead(int fd, void *buf, size_t count, size_t buf_size);
+int
+sio_listen(int fd);
/**
- * Read count bytes from fd.
- * Throw an exception on error or disconnect.
- *
- * @return count of bytes actually read.
+ * Accept a client connection on a server socket. A wrapper for
+ * accept(), but throws exception on error except EAGAIN, EINTR,
+ * EWOULDBLOCK.
*/
-static inline ssize_t
-sio_readn(int fd, void *buf, size_t count)
-{
- return sio_readn_ahead(fd, buf, count, count);
-}
+int
+sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen);
/**
- * Write count bytes to fd.
- * Throw an exception on error or disconnect.
- *
- * @return count of bytes actually written.
+ * Read up to @a count bytes from a socket. A wrapper for read(),
+ * but throws exception on error except EWOULDBLOCK, EINTR,
+ * EAGAIN, ECONNRESET.
*/
ssize_t
-sio_writen(int fd, const void *buf, size_t count);
+sio_read(int fd, void *buf, size_t count);
-/* Only for blocked I/O */
+/**
+ * Write up to @a count bytes to a socket. A wrapper for write(),
+ * but throws exception on error except EAGAIN, EINTR,
+ * EWOULDBLOCK.
+ */
ssize_t
-sio_writev_all(int fd, struct iovec *iov, int iovcnt);
+sio_write(int fd, const void *buf, size_t count);
/**
- * A wrapper over sendfile.
- * Throw if send file failed.
+ * Write @a iov vector to a socket. A wrapper for writev(), but
+ * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
*/
ssize_t
-sio_sendfile(int sock_fd, int file_fd, off_t *offset, size_t size);
+sio_writev(int fd, const struct iovec *iov, int iovcnt);
/**
- * Receive a file sent by sendfile
- * Throw if receiving failed
+ * Send a message on a socket. A wrapper for sendto(), but throws
+ * exception on error except EAGAIN, EINTR, EWOULDBLOCK.
*/
ssize_t
-sio_recvfile(int sock_fd, int file_fd, off_t *offset, size_t size);
-
+sio_sendto(int fd, const void *buf, size_t len, int flags,
+ const struct sockaddr *dest_addr, socklen_t addrlen);
-ssize_t sio_sendto(int fd, const void *buf, size_t len, int flags,
- const struct sockaddr *dest_addr, socklen_t addrlen);
-
-ssize_t sio_recvfrom(int fd, void *buf, size_t len, int flags,
- struct sockaddr *src_addr, socklen_t *addrlen);
+/**
+ * Receive a message on a socket. A wrapper for recvfrom(), but
+ * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
+ */
+ssize_t
+sio_recvfrom(int fd, void *buf, size_t len, int flags,
+ struct sockaddr *src_addr, socklen_t *addrlen);
#endif /* defined(__cplusplus) */
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 02/11] sio: remove unused functions, restyle code
2018-11-30 15:39 ` [PATCH 02/11] sio: remove unused functions, restyle code Vladislav Shpilevoy
@ 2018-12-03 12:28 ` Vladimir Davydov
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 12:28 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:34PM +0300, Vladislav Shpilevoy wrote:
> -/** Read up to 'count' bytes from a socket. */
> ssize_t
> sio_read(int fd, void *buf, size_t count)
> {
> ssize_t n = read(fd, buf, count);
> - if (n < 0) {
> - if (errno == EWOULDBLOCK)
> - errno = EINTR;
> - switch (errno) {
> - case EAGAIN:
> - case EINTR:
> - break;
> - /*
> - * Happens typically when the client closes
> - * socket on timeout without reading the previous
> - * query's response completely. Treat the same as
> - * EOF.
> - */
> - case ECONNRESET:
> - errno = 0;
> - n = 0;
> - break;
> - default:
> - tnt_raise(SocketError, sio_socketname(fd),
> - "read(%zd)", count);
> - }
> + if (n >= 0)
> + return n;
> + if (errno == EWOULDBLOCK)
> + errno = EINTR;
> + switch (errno) {
> + case EAGAIN:
> + case EINTR:
> + break;
> + /*
> + * Happens typically when the client closes socket on
> + * timeout without reading the previous query's response
> + * completely. Treat the same as EOF.
> + */
> + case ECONNRESET:
> + errno = 0;
> + n = 0;
> + break;
> + default:
> + tnt_raise(SocketError, sio_socketname(fd), "read(%zd)", count);
If I had to write this function myself, I'd probably write it the same
way as you intend to patch it, however it isn't worth polluting the
history now. Please revert this change and all other changes that exist
solely to make the code conform to your taste. The only thing that's
worth doing now is removing unused functions, because otherwise you'd
have to remove exceptions from them, too. I wouldn't even touch the
comments - they look fine to me, as a matter of fact.
> -int sio_setfl(int fd, int flag, int on);
> +/**
> + * Set a file descriptor flag. A wrapper for
> + * fcntl(F_SETFL, flag | fcntl(F_GETFL)), but sets diagnostics on
> + * error.
> + */
> +int
> +sio_setfl(int fd, int flag);
This makes the API kinda lopsided - you can set a flag, but you can't
clear it. Please leave it as is.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 02/11] sio: remove unused functions, restyle code
2018-12-03 12:28 ` Vladimir Davydov
@ 2018-12-04 21:29 ` Vladislav Shpilevoy
2018-12-05 8:41 ` Vladimir Davydov
0 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:29 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
Hi! Thanks for the review.
On 03/12/2018 15:28, Vladimir Davydov wrote:
> On Fri, Nov 30, 2018 at 06:39:34PM +0300, Vladislav Shpilevoy wrote:
>> -/** Read up to 'count' bytes from a socket. */
>> ssize_t
>> sio_read(int fd, void *buf, size_t count)
>> {
>> ssize_t n = read(fd, buf, count);
>> - if (n < 0) {
>> - if (errno == EWOULDBLOCK)
>> - errno = EINTR;
>> - switch (errno) {
>> - case EAGAIN:
>> - case EINTR:
>> - break;
>> - /*
>> - * Happens typically when the client closes
>> - * socket on timeout without reading the previous
>> - * query's response completely. Treat the same as
>> - * EOF.
>> - */
>> - case ECONNRESET:
>> - errno = 0;
>> - n = 0;
>> - break;
>> - default:
>> - tnt_raise(SocketError, sio_socketname(fd),
>> - "read(%zd)", count);
>> - }
>> + if (n >= 0)
>> + return n;
>> + if (errno == EWOULDBLOCK)
>> + errno = EINTR;
>> + switch (errno) {
>> + case EAGAIN:
>> + case EINTR:
>> + break;
>> + /*
>> + * Happens typically when the client closes socket on
>> + * timeout without reading the previous query's response
>> + * completely. Treat the same as EOF.
>> + */
>> + case ECONNRESET:
>> + errno = 0;
>> + n = 0;
>> + break;
>> + default:
>> + tnt_raise(SocketError, sio_socketname(fd), "read(%zd)", count);
>
> If I had to write this function myself, I'd probably write it the same
> way as you intend to patch it, however it isn't worth polluting the
> history now. Please revert this change and all other changes that exist
> solely to make the code conform to your taste. The only thing that's
> worth doing now is removing unused functions, because otherwise you'd
> have to remove exceptions from them, too. I wouldn't even touch the
> comments - they look fine to me, as a matter of fact.
>
>> -int sio_setfl(int fd, int flag, int on);
>> +/**
>> + * Set a file descriptor flag. A wrapper for
>> + * fcntl(F_SETFL, flag | fcntl(F_GETFL)), but sets diagnostics on
>> + * error.
>> + */
>> +int
>> +sio_setfl(int fd, int flag);
>
> This makes the API kinda lopsided - you can set a flag, but you can't
> clear it. Please leave it as is.
It is always 1 everywhere. Makes no sense to provide an ability
to unset and I can not imagine when we could need such 'feature'.
But as you wish. Done. See v2 in a new thread. Too many changes to
paste them here.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 02/11] sio: remove unused functions, restyle code
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-05 8:41 ` Vladimir Davydov
0 siblings, 0 replies; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-05 8:41 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:29:07AM +0300, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review.
>
> On 03/12/2018 15:28, Vladimir Davydov wrote:
> > On Fri, Nov 30, 2018 at 06:39:34PM +0300, Vladislav Shpilevoy wrote:
> > > -/** Read up to 'count' bytes from a socket. */
> > > ssize_t
> > > sio_read(int fd, void *buf, size_t count)
> > > {
> > > ssize_t n = read(fd, buf, count);
> > > - if (n < 0) {
> > > - if (errno == EWOULDBLOCK)
> > > - errno = EINTR;
> > > - switch (errno) {
> > > - case EAGAIN:
> > > - case EINTR:
> > > - break;
> > > - /*
> > > - * Happens typically when the client closes
> > > - * socket on timeout without reading the previous
> > > - * query's response completely. Treat the same as
> > > - * EOF.
> > > - */
> > > - case ECONNRESET:
> > > - errno = 0;
> > > - n = 0;
> > > - break;
> > > - default:
> > > - tnt_raise(SocketError, sio_socketname(fd),
> > > - "read(%zd)", count);
> > > - }
> > > + if (n >= 0)
> > > + return n;
> > > + if (errno == EWOULDBLOCK)
> > > + errno = EINTR;
> > > + switch (errno) {
> > > + case EAGAIN:
> > > + case EINTR:
> > > + break;
> > > + /*
> > > + * Happens typically when the client closes socket on
> > > + * timeout without reading the previous query's response
> > > + * completely. Treat the same as EOF.
> > > + */
> > > + case ECONNRESET:
> > > + errno = 0;
> > > + n = 0;
> > > + break;
> > > + default:
> > > + tnt_raise(SocketError, sio_socketname(fd), "read(%zd)", count);
> >
> > If I had to write this function myself, I'd probably write it the same
> > way as you intend to patch it, however it isn't worth polluting the
> > history now. Please revert this change and all other changes that exist
> > solely to make the code conform to your taste. The only thing that's
> > worth doing now is removing unused functions, because otherwise you'd
> > have to remove exceptions from them, too. I wouldn't even touch the
> > comments - they look fine to me, as a matter of fact.
> >
> > > -int sio_setfl(int fd, int flag, int on);
> > > +/**
> > > + * Set a file descriptor flag. A wrapper for
> > > + * fcntl(F_SETFL, flag | fcntl(F_GETFL)), but sets diagnostics on
> > > + * error.
> > > + */
> > > +int
> > > +sio_setfl(int fd, int flag);
> >
> > This makes the API kinda lopsided - you can set a flag, but you can't
> > clear it. Please leave it as is.
>
> It is always 1 everywhere. Makes no sense to provide an ability
I know, but it has nothing to do with your goal. There are a lot of
places in the code that I don't like - malformed indentation here and
there, grammatical mistakes and typos in comments (there are plenty of
them), weird function prototypes and names, but I refrain from patching
them just in case, because this pollutes the git history, wastes
reviewer's time, and complicates cherry-pickying.
> to unset and I can not imagine when we could need such 'feature'.
>
> But as you wish. Done. See v2 in a new thread. Too many changes to
> paste them here.
Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 03/11] sio: remove exceptions
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (3 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 02/11] sio: remove unused functions, restyle code Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-12-03 12:36 ` Vladimir Davydov
2018-11-30 15:39 ` [PATCH 04/11] sio: fix passing negative size_t to sio_add_to_iov Vladislav Shpilevoy
` (6 subsequent siblings)
11 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
To turn sio into C it should not have exceptions.
Replace them with diagnostics setting. In some cases
errors are not critical so several functions return
an error criticalness along with setting or not
setting diag.
---
src/box/iproto.cc | 31 ++++++++++--------
src/coio.cc | 54 +++++++++++++++++++------------
src/evio.cc | 13 +++++---
src/sio.cc | 72 ++++++++++++++++++++++++++++-------------
src/sio.h | 81 +++++++++++++++++++++++++++++++++++++++--------
5 files changed, 176 insertions(+), 75 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index cd613932e..07ef23cac 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -857,8 +857,12 @@ iproto_connection_on_input(ev_loop *loop, struct ev_io *watcher,
return;
}
/* Read input. */
- int nrd = sio_read(fd, in->wpos, ibuf_unused(in));
+ bool is_error_fatal;
+ int nrd = sio_read(fd, in->wpos, ibuf_unused(in),
+ &is_error_fatal);
if (nrd < 0) { /* Socket is not ready. */
+ if (is_error_fatal)
+ diag_raise();
ev_io_start(loop, &con->input);
return;
}
@@ -922,11 +926,12 @@ iproto_flush(struct iproto_connection *con)
/* *Overwrite* iov_len of the last pos as it may be garbage. */
iov[iovcnt-1].iov_len = end->iov_len - begin->iov_len * (iovcnt == 1);
- ssize_t nwr = sio_writev(fd, iov, iovcnt);
-
- /* Count statistics */
- rmean_collect(rmean_net, IPROTO_SENT, nwr);
- if (nwr > 0) {
+ bool is_error_fatal;
+ ssize_t nwr = sio_writev(fd, iov, iovcnt, &is_error_fatal);
+ if (nwr < 0 && is_error_fatal)
+ diag_raise();
+ else if (nwr > 0) {
+ rmean_collect(rmean_net, IPROTO_SENT, nwr);
if (begin->used + nwr == end->used) {
*begin = *end;
return 0;
@@ -1752,15 +1757,13 @@ net_send_greeting(struct cmsg *m)
struct iproto_connection *con = msg->connection;
if (msg->close_connection) {
struct obuf *out = msg->wpos.obuf;
- try {
- int64_t nwr = sio_writev(con->output.fd, out->iov,
- obuf_iovcnt(out));
-
- /* Count statistics */
+ bool is_error_fatal;
+ int64_t nwr = sio_writev(con->output.fd, out->iov,
+ obuf_iovcnt(out), &is_error_fatal);
+ if (nwr < 0 && is_error_fatal)
+ diag_log();
+ else if (nwr > 0)
rmean_collect(rmean_net, IPROTO_SENT, nwr);
- } catch (Exception *e) {
- e->log();
- }
assert(iproto_connection_is_idle(con));
iproto_connection_close(con);
iproto_msg_delete(msg);
diff --git a/src/coio.cc b/src/coio.cc
index e91de40b5..b69f5e31a 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -78,11 +78,9 @@ coio_connect_addr(struct ev_io *coio, struct sockaddr *addr,
{
ev_loop *loop = loop();
evio_socket(coio, addr->sa_family, SOCK_STREAM, 0);
- auto coio_guard = make_scoped_guard([=]{ evio_close(loop, coio); });
- if (sio_connect(coio->fd, addr, len) == 0) {
- coio_guard.is_active = false;
+ if (sio_connect(coio->fd, addr, len) == 0)
return 0;
- }
+ auto coio_guard = make_scoped_guard([=]{ evio_close(loop, coio); });
if (errno != EINPROGRESS)
diag_raise();
/*
@@ -252,12 +250,16 @@ coio_accept(struct ev_io *coio, struct sockaddr *addr,
while (true) {
/* Assume that there are waiting clients
* available */
- int fd = sio_accept(coio->fd, addr, &addrlen);
+ bool is_error_fatal;
+ int fd = sio_accept(coio->fd, addr, &addrlen,
+ &is_error_fatal);
if (fd >= 0) {
evio_setsockopt_client(fd, addr->sa_family,
SOCK_STREAM);
return fd;
}
+ if (is_error_fatal)
+ diag_raise();
/* The socket is not ready, yield */
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_READ);
@@ -303,7 +305,9 @@ coio_read_ahead_timeout(struct ev_io *coio, void *buf, size_t sz,
* the user called read(), some data must
* be expected.
*/
- ssize_t nrd = sio_read(coio->fd, buf, bufsiz);
+ bool is_error_fatal;
+ ssize_t nrd = sio_read(coio->fd, buf, bufsiz,
+ &is_error_fatal);
if (nrd > 0) {
to_read -= nrd;
if (to_read <= 0)
@@ -313,6 +317,8 @@ coio_read_ahead_timeout(struct ev_io *coio, void *buf, size_t sz,
} else if (nrd == 0) {
errno = 0;
return sz - to_read;
+ } else if (is_error_fatal) {
+ diag_raise();
}
/* The socket is not ready, yield */
@@ -397,13 +403,17 @@ coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz,
* Sic: write as much data as possible,
* assuming the socket is ready.
*/
- ssize_t nwr = sio_write(coio->fd, buf, towrite);
+ bool is_error_fatal;
+ ssize_t nwr = sio_write(coio->fd, buf, towrite,
+ &is_error_fatal);
if (nwr > 0) {
/* Go past the data just written. */
if (nwr >= towrite)
return sz;
towrite -= nwr;
buf = (char *) buf + nwr;
+ } else if (is_error_fatal) {
+ diag_raise();
}
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_WRITE);
@@ -433,15 +443,12 @@ coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz,
static inline ssize_t
coio_flush(int fd, struct iovec *iov, ssize_t offset, int iovcnt)
{
- ssize_t nwr;
- try {
- sio_add_to_iov(iov, -offset);
- nwr = sio_writev(fd, iov, iovcnt);
- sio_add_to_iov(iov, offset);
- } catch (SocketError *e) {
- sio_add_to_iov(iov, offset);
- throw;
- }
+ sio_add_to_iov(iov, -offset);
+ bool is_error_fatal;
+ ssize_t nwr = sio_writev(fd, iov, iovcnt, &is_error_fatal);
+ sio_add_to_iov(iov, offset);
+ if (nwr < 0 && is_error_fatal)
+ diag_raise();
return nwr;
}
@@ -518,10 +525,13 @@ coio_sendto_timeout(struct ev_io *coio, const void *buf, size_t sz, int flags,
* Sic: write as much data as possible,
* assuming the socket is ready.
*/
- ssize_t nwr = sio_sendto(coio->fd, buf, sz,
- flags, dest_addr, addrlen);
+ bool is_error_fatal;
+ ssize_t nwr = sio_sendto(coio->fd, buf, sz, flags, dest_addr,
+ addrlen, &is_error_fatal);
if (nwr > 0)
return nwr;
+ if (is_error_fatal)
+ diag_raise();
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_WRITE);
ev_io_start(loop(), coio);
@@ -561,11 +571,13 @@ coio_recvfrom_timeout(struct ev_io *coio, void *buf, size_t sz, int flags,
* Read as much data as possible,
* assuming the socket is ready.
*/
- ssize_t nrd = sio_recvfrom(coio->fd, buf, sz, flags,
- src_addr, &addrlen);
+ bool is_error_fatal;
+ ssize_t nrd = sio_recvfrom(coio->fd, buf, sz, flags, src_addr,
+ &addrlen, &is_error_fatal);
if (nrd >= 0)
return nrd;
-
+ if (is_error_fatal)
+ diag_raise();
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_READ);
ev_io_start(loop(), coio);
diff --git a/src/evio.cc b/src/evio.cc
index a8475e0d1..401e71155 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -181,11 +181,15 @@ evio_service_accept_cb(ev_loop * /* loop */, ev_io *watcher,
try {
struct sockaddr_storage addr;
socklen_t addrlen = sizeof(addr);
+ bool is_error_fatal;
fd = sio_accept(service->ev.fd,
- (struct sockaddr *)&addr, &addrlen);
-
- if (fd < 0) /* EAGAIN, EWOULDLOCK, EINTR */
+ (struct sockaddr *)&addr, &addrlen,
+ &is_error_fatal);
+ if (fd < 0) {
+ if (is_error_fatal)
+ diag_raise();
return;
+ }
/* set common client socket options */
evio_setsockopt_client(fd, service->addr.sa_family, SOCK_STREAM);
/*
@@ -294,7 +298,8 @@ evio_service_listen(struct evio_service *service)
sio_strfaddr(&service->addr, service->addr_len));
int fd = service->ev.fd;
- sio_listen(fd);
+ if (sio_listen(fd) < 0)
+ diag_raise();
ev_io_start(service->loop, &service->ev);
}
diff --git a/src/sio.cc b/src/sio.cc
index b2f05e5c8..09f413c2d 100644
--- a/src/sio.cc
+++ b/src/sio.cc
@@ -206,23 +206,27 @@ sio_listen(int fd)
{
int rc = listen(fd, sio_listen_backlog());
if (rc < 0)
- tnt_raise(SocketError, sio_socketname(fd), "listen");
+ diag_set(SocketError, sio_socketname(fd), "listen");
return rc;
}
int
-sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen)
+sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen,
+ bool *is_error_fatal)
{
/* Accept a connection. */
int newfd = accept(fd, addr, addrlen);
- if (newfd < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
- errno != EINTR)
- tnt_raise(SocketError, sio_socketname(fd), "accept");
+ if (newfd < 0) {
+ *is_error_fatal = errno != EAGAIN && errno != EWOULDBLOCK &&
+ errno != EINTR;
+ if (*is_error_fatal)
+ diag_set(SocketError, sio_socketname(fd), "accept");
+ }
return newfd;
}
ssize_t
-sio_read(int fd, void *buf, size_t count)
+sio_read(int fd, void *buf, size_t count, bool *is_error_fatal)
{
ssize_t n = read(fd, buf, count);
if (n >= 0)
@@ -232,6 +236,7 @@ sio_read(int fd, void *buf, size_t count)
switch (errno) {
case EAGAIN:
case EINTR:
+ *is_error_fatal = false;
break;
/*
* Happens typically when the client closes socket on
@@ -241,55 +246,78 @@ sio_read(int fd, void *buf, size_t count)
case ECONNRESET:
errno = 0;
n = 0;
+ *is_error_fatal = false;
break;
default:
- tnt_raise(SocketError, sio_socketname(fd), "read(%zd)", count);
+ diag_set(SocketError, sio_socketname(fd), "read(%zd)", count);
+ *is_error_fatal = true;
}
return n;
}
ssize_t
-sio_write(int fd, const void *buf, size_t count)
+sio_write(int fd, const void *buf, size_t count, bool *is_error_fatal)
{
ssize_t n = write(fd, buf, count);
- if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR)
- tnt_raise(SocketError, sio_socketname(fd), "write(%zd)", count);
+ if (n < 0) {
+ *is_error_fatal = errno != EAGAIN && errno != EWOULDBLOCK &&
+ errno != EINTR;
+ if (*is_error_fatal) {
+ diag_set(SocketError, sio_socketname(fd), "write(%zd)",
+ count);
+ }
+ }
return n;
}
ssize_t
-sio_writev(int fd, const struct iovec *iov, int iovcnt)
+sio_writev(int fd, const struct iovec *iov, int iovcnt, bool *is_error_fatal)
{
int cnt = iovcnt < IOV_MAX ? iovcnt : IOV_MAX;
ssize_t n = writev(fd, iov, cnt);
- if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
- errno != EINTR) {
- tnt_raise(SocketError, sio_socketname(fd),
- "writev(%d)", iovcnt);
+ if (n < 0) {
+ *is_error_fatal = errno != EAGAIN && errno != EWOULDBLOCK &&
+ errno != EINTR;
+ if (*is_error_fatal) {
+ diag_set(SocketError, sio_socketname(fd), "writev(%d)",
+ iovcnt);
+ }
}
return n;
}
ssize_t
sio_sendto(int fd, const void *buf, size_t len, int flags,
- const struct sockaddr *dest_addr, socklen_t addrlen)
+ const struct sockaddr *dest_addr, socklen_t addrlen,
+ bool *is_error_fatal)
{
ssize_t n = sendto(fd, buf, len, flags, (struct sockaddr*)dest_addr,
addrlen);
- if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR)
- tnt_raise(SocketError, sio_socketname(fd), "sendto(%zd)", len);
+ if (n < 0) {
+ *is_error_fatal = errno != EAGAIN && errno != EWOULDBLOCK &&
+ errno != EINTR;
+ if (*is_error_fatal) {
+ diag_set(SocketError, sio_socketname(fd), "sendto(%zd)",
+ len);
+ }
+ }
return n;
}
ssize_t
sio_recvfrom(int fd, void *buf, size_t len, int flags,
- struct sockaddr *src_addr, socklen_t *addrlen)
+ struct sockaddr *src_addr, socklen_t *addrlen,
+ bool *is_error_fatal)
{
ssize_t n = recvfrom(fd, buf, len, flags, (struct sockaddr*)src_addr,
addrlen);
- if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR) {
- tnt_raise(SocketError, sio_socketname(fd), "recvfrom(%zd)",
- len);
+ if (n < 0) {
+ *is_error_fatal = errno != EAGAIN && errno != EWOULDBLOCK &&
+ errno != EINTR;
+ if (*is_error_fatal) {
+ diag_set(SocketError, sio_socketname(fd),
+ "recvfrom(%zd)", len);
+ }
}
return n;
}
diff --git a/src/sio.h b/src/sio.h
index d937cfd3d..ab0a243cd 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -151,57 +151,110 @@ sio_bind(int fd, struct sockaddr *addr, socklen_t addrlen);
/**
* Mark a socket as accepting connections. A wrapper for listen(),
- * but throws exception on error.
+ * but sets diagnostics on error.
*/
int
sio_listen(int fd);
/**
* Accept a client connection on a server socket. A wrapper for
- * accept(), but throws exception on error except EAGAIN, EINTR,
+ * accept(), but sets diagnostics on error except EAGAIN, EINTR,
* EWOULDBLOCK.
+ * @param fd Server socket.
+ * @param[out] addr Accepted client's address.
+ * @param[in, out] addrlen Size of @a addr.
+ * @param[out] is_error_fatal Set to true, if an error occured and
+ * it was not EAGAIN, EINTR and EWOULDBLOCK.
+ *
+ * @retval Client socket, or -1 on error.
*/
int
-sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen);
+sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen,
+ bool *is_error_fatal);
/**
* Read up to @a count bytes from a socket. A wrapper for read(),
- * but throws exception on error except EWOULDBLOCK, EINTR,
+ * but sets diagnostics on error except EWOULDBLOCK, EINTR,
* EAGAIN, ECONNRESET.
+ * @param fd Socket.
+ * @param buf Buffer to read into.
+ * @param count How many bytes to read.
+ * @param[out] is_error_fatal Set to true, if an error occured and
+ * it was not EAGAIN, EINTR, ECONNRESET and
+ * EWOULDBLOCK.
+ *
+ * @retval How many bytes are read, or -1 on error.
*/
ssize_t
-sio_read(int fd, void *buf, size_t count);
+sio_read(int fd, void *buf, size_t count, bool *is_error_fatal);
/**
* Write up to @a count bytes to a socket. A wrapper for write(),
- * but throws exception on error except EAGAIN, EINTR,
+ * but sets diagnostics on error except EAGAIN, EINTR,
* EWOULDBLOCK.
+ * @param fd Socket.
+ * @param buf Buffer to write.
+ * @param count How many bytes to write.
+ * @param[out] is_error_fatal Set to true, if an error occured and
+ * it was not EAGAIN, EINTR and EWOULDBLOCK.
+ *
+ * @retval How many bytes are written, or -1 on error.
*/
ssize_t
-sio_write(int fd, const void *buf, size_t count);
+sio_write(int fd, const void *buf, size_t count, bool *is_error_fatal);
/**
* Write @a iov vector to a socket. A wrapper for writev(), but
- * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
+ * sets diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
+ * @param fd Socket.
+ * @param iov Vector to write.
+ * @param iovcnt Size of @a iov.
+ * @param[out] is_error_fatal Set to true, if an error occured and
+ * it was not EAGAIN, EINTR and EWOULDBLOCK.
+ *
+ * @retval How many bytes are written, or -1 on error.
*/
ssize_t
-sio_writev(int fd, const struct iovec *iov, int iovcnt);
+sio_writev(int fd, const struct iovec *iov, int iovcnt,
+ bool *is_error_fatal);
/**
- * Send a message on a socket. A wrapper for sendto(), but throws
- * exception on error except EAGAIN, EINTR, EWOULDBLOCK.
+ * Send a message on a socket. A wrapper for sendto(), but sets
+ * diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
+ * @param fd Socket to send to.
+ * @param buf Buffer to send.
+ * @param len Size of @a buf.
+ * @param flags sendto() flags.
+ * @param dest_addr Destination address.
+ * @param addrlen Size of @a dest_addr.
+ * @param[out] is_error_fatal Set to true, if an error occured and
+ * it was not EAGAIN, EINTR and EWOULDBLOCK.
+ *
+ * @param How many bytes are sent, or -1 on error.
*/
ssize_t
sio_sendto(int fd, const void *buf, size_t len, int flags,
- const struct sockaddr *dest_addr, socklen_t addrlen);
+ const struct sockaddr *dest_addr, socklen_t addrlen,
+ bool *is_error_fatal);
/**
* Receive a message on a socket. A wrapper for recvfrom(), but
- * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
+ * sets diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
+ * @param fd Socket to receive from.
+ * @param buf Buffer to save message.
+ * @param len Size of @a buf.
+ * @param flags recvfrom() flags.
+ * @param[out] src_addr Source address.
+ * @param[in, out] addrlen Size of @a src_addr.
+ * @param[out] is_error_fatal Set to true, if an error occured and
+ * it was not EAGAIN, EINTR and EWOULDBLOCK.
+ *
+ * @param How many bytes are received, or -1 on error.
*/
ssize_t
sio_recvfrom(int fd, void *buf, size_t len, int flags,
- struct sockaddr *src_addr, socklen_t *addrlen);
+ struct sockaddr *src_addr, socklen_t *addrlen,
+ bool *is_error_fatal);
#endif /* defined(__cplusplus) */
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 03/11] sio: remove exceptions
2018-11-30 15:39 ` [PATCH 03/11] sio: remove exceptions Vladislav Shpilevoy
@ 2018-12-03 12:36 ` Vladimir Davydov
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 12:36 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:35PM +0300, Vladislav Shpilevoy wrote:
> diff --git a/src/sio.h b/src/sio.h
> index d937cfd3d..ab0a243cd 100644
> --- a/src/sio.h
> +++ b/src/sio.h
> @@ -151,57 +151,110 @@ sio_bind(int fd, struct sockaddr *addr, socklen_t addrlen);
>
> /**
> * Mark a socket as accepting connections. A wrapper for listen(),
> - * but throws exception on error.
> + * but sets diagnostics on error.
> */
> int
> sio_listen(int fd);
>
> /**
> * Accept a client connection on a server socket. A wrapper for
> - * accept(), but throws exception on error except EAGAIN, EINTR,
> + * accept(), but sets diagnostics on error except EAGAIN, EINTR,
> * EWOULDBLOCK.
> + * @param fd Server socket.
> + * @param[out] addr Accepted client's address.
> + * @param[in, out] addrlen Size of @a addr.
> + * @param[out] is_error_fatal Set to true, if an error occured and
> + * it was not EAGAIN, EINTR and EWOULDBLOCK.
> + *
> + * @retval Client socket, or -1 on error.
> */
> int
> -sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen);
> +sio_accept(int fd, struct sockaddr *addr, socklen_t *addrlen,
> + bool *is_error_fatal);
>
> /**
> * Read up to @a count bytes from a socket. A wrapper for read(),
> - * but throws exception on error except EWOULDBLOCK, EINTR,
> + * but sets diagnostics on error except EWOULDBLOCK, EINTR,
> * EAGAIN, ECONNRESET.
> + * @param fd Socket.
> + * @param buf Buffer to read into.
> + * @param count How many bytes to read.
> + * @param[out] is_error_fatal Set to true, if an error occured and
> + * it was not EAGAIN, EINTR, ECONNRESET and
> + * EWOULDBLOCK.
> + *
> + * @retval How many bytes are read, or -1 on error.
> */
> ssize_t
> -sio_read(int fd, void *buf, size_t count);
> +sio_read(int fd, void *buf, size_t count, bool *is_error_fatal);
>
> /**
> * Write up to @a count bytes to a socket. A wrapper for write(),
> - * but throws exception on error except EAGAIN, EINTR,
> + * but sets diagnostics on error except EAGAIN, EINTR,
> * EWOULDBLOCK.
> + * @param fd Socket.
> + * @param buf Buffer to write.
> + * @param count How many bytes to write.
> + * @param[out] is_error_fatal Set to true, if an error occured and
> + * it was not EAGAIN, EINTR and EWOULDBLOCK.
> + *
> + * @retval How many bytes are written, or -1 on error.
> */
> ssize_t
> -sio_write(int fd, const void *buf, size_t count);
> +sio_write(int fd, const void *buf, size_t count, bool *is_error_fatal);
>
> /**
> * Write @a iov vector to a socket. A wrapper for writev(), but
> - * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
> + * sets diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
> + * @param fd Socket.
> + * @param iov Vector to write.
> + * @param iovcnt Size of @a iov.
> + * @param[out] is_error_fatal Set to true, if an error occured and
> + * it was not EAGAIN, EINTR and EWOULDBLOCK.
> + *
> + * @retval How many bytes are written, or -1 on error.
> */
> ssize_t
> -sio_writev(int fd, const struct iovec *iov, int iovcnt);
> +sio_writev(int fd, const struct iovec *iov, int iovcnt,
> + bool *is_error_fatal);
>
> /**
> - * Send a message on a socket. A wrapper for sendto(), but throws
> - * exception on error except EAGAIN, EINTR, EWOULDBLOCK.
> + * Send a message on a socket. A wrapper for sendto(), but sets
> + * diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
> + * @param fd Socket to send to.
> + * @param buf Buffer to send.
> + * @param len Size of @a buf.
> + * @param flags sendto() flags.
> + * @param dest_addr Destination address.
> + * @param addrlen Size of @a dest_addr.
> + * @param[out] is_error_fatal Set to true, if an error occured and
> + * it was not EAGAIN, EINTR and EWOULDBLOCK.
> + *
> + * @param How many bytes are sent, or -1 on error.
> */
> ssize_t
> sio_sendto(int fd, const void *buf, size_t len, int flags,
> - const struct sockaddr *dest_addr, socklen_t addrlen);
> + const struct sockaddr *dest_addr, socklen_t addrlen,
> + bool *is_error_fatal);
>
> /**
> * Receive a message on a socket. A wrapper for recvfrom(), but
> - * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
> + * sets diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
> + * @param fd Socket to receive from.
> + * @param buf Buffer to save message.
> + * @param len Size of @a buf.
> + * @param flags recvfrom() flags.
> + * @param[out] src_addr Source address.
> + * @param[in, out] addrlen Size of @a src_addr.
> + * @param[out] is_error_fatal Set to true, if an error occured and
> + * it was not EAGAIN, EINTR and EWOULDBLOCK.
> + *
> + * @param How many bytes are received, or -1 on error.
> */
> ssize_t
> sio_recvfrom(int fd, void *buf, size_t len, int flags,
> - struct sockaddr *src_addr, socklen_t *addrlen);
> + struct sockaddr *src_addr, socklen_t *addrlen,
> + bool *is_error_fatal);
This new API doesn't look good to me. Let's instead set diag and return
-1 on every error and introduce a helper to check if a particular error
code means that the socket would block (the name might need to be
refined):
sio_wouldblock(errno)
?
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 03/11] sio: remove exceptions
2018-12-03 12:36 ` Vladimir Davydov
@ 2018-12-04 21:29 ` Vladislav Shpilevoy
2018-12-05 8:42 ` Vladimir Davydov
0 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:29 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
>> /**
>> * Receive a message on a socket. A wrapper for recvfrom(), but
>> - * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
>> + * sets diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
>> + * @param fd Socket to receive from.
>> + * @param buf Buffer to save message.
>> + * @param len Size of @a buf.
>> + * @param flags recvfrom() flags.
>> + * @param[out] src_addr Source address.
>> + * @param[in, out] addrlen Size of @a src_addr.
>> + * @param[out] is_error_fatal Set to true, if an error occured and
>> + * it was not EAGAIN, EINTR and EWOULDBLOCK.
>> + *
>> + * @param How many bytes are received, or -1 on error.
>> */
>> ssize_t
>> sio_recvfrom(int fd, void *buf, size_t len, int flags,
>> - struct sockaddr *src_addr, socklen_t *addrlen);
>> + struct sockaddr *src_addr, socklen_t *addrlen,
>> + bool *is_error_fatal);
>
> This new API doesn't look good to me. Let's instead set diag and return
> -1 on every error and introduce a helper to check if a particular error
> code means that the socket would block (the name might need to be
> refined):>
> sio_wouldblock(errno)
>
> ?
>
The only purpose of the out flag was to hide from a user details
of how exactly to check if an error is not critical: via errno, or
via checking diag_is_empty(diag_get()) or via any other way. So a
user wouldn't need to include errno or use complex constructions
like with diag.
But as you wish. Lets find another way.
Any names having 'wouldblock', 'block' or describing other concrete
behaviour can not be used. At least, sio_bind/listen can set EADDRINUSE,
which is not about blocking.
Also, notion of 'critical' is different for various functions. I've
grouped errors in 4 groups:
EINPROGRESS - sio_connect
EADDRINUSE - sio_bind, sio_listen
EAGAIN - sio_accept, sio_write,
EWOULDBLOCK sio_writev, sio_readn_ahead,
EINTR sio_sendto, sio_recvfrom
ECONNRESET - sio_read
EAGAIN
EWOULDBLOCK
EINTR
I can not implement one function for checking of any
error via errno because
* EADDRINUSE is not critical for bind/listen, but
critical for connect;
* EAGAIN, EINTR are not critical for IO, but critical
for connect;
* ECONNRESET is not critical for sio_read, but critical
for sio_sendto.
The list is not complete I think, and I do not want to fix some
crutches in a single error checking function each time when
something changed in any other sio function.
I can not use the simplest way !diag_is_empty(diag_get()) as well
because as I discovered, iproto thread does not clean diag in
some places after processing an error. For example,
iproto_connection_resume on error leaves an error in diag and
the error lives in the thread until next error. The same in
iproto_connection_on_input(), where catch() block does not
clears diag.
I could use diag if I fixed these places, but I do not want
to rely on cleanness of external code.
I decided to return from some functions a special code that
can be checked by sio_is_error_fatal().
See v2 thread for a new version of the commit. I do not paste
diff here, since in fact the patch is rewritten completely.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 03/11] sio: remove exceptions
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-05 8:42 ` Vladimir Davydov
0 siblings, 0 replies; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-05 8:42 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:29:10AM +0300, Vladislav Shpilevoy wrote:
>
> > > /**
> > > * Receive a message on a socket. A wrapper for recvfrom(), but
> > > - * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
> > > + * sets diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
> > > + * @param fd Socket to receive from.
> > > + * @param buf Buffer to save message.
> > > + * @param len Size of @a buf.
> > > + * @param flags recvfrom() flags.
> > > + * @param[out] src_addr Source address.
> > > + * @param[in, out] addrlen Size of @a src_addr.
> > > + * @param[out] is_error_fatal Set to true, if an error occured and
> > > + * it was not EAGAIN, EINTR and EWOULDBLOCK.
> > > + *
> > > + * @param How many bytes are received, or -1 on error.
> > > */
> > > ssize_t
> > > sio_recvfrom(int fd, void *buf, size_t len, int flags,
> > > - struct sockaddr *src_addr, socklen_t *addrlen);
> > > + struct sockaddr *src_addr, socklen_t *addrlen,
> > > + bool *is_error_fatal);
> >
> > This new API doesn't look good to me. Let's instead set diag and return
> > -1 on every error and introduce a helper to check if a particular error
> > code means that the socket would block (the name might need to be
> > refined):> sio_wouldblock(errno)
> >
> > ?
> >
>
> The only purpose of the out flag was to hide from a user details
> of how exactly to check if an error is not critical: via errno, or
> via checking diag_is_empty(diag_get()) or via any other way. So a
> user wouldn't need to include errno or use complex constructions
> like with diag.
>
> But as you wish. Lets find another way.
>
> Any names having 'wouldblock', 'block' or describing other concrete
> behaviour can not be used. At least, sio_bind/listen can set EADDRINUSE,
> which is not about blocking.
>
> Also, notion of 'critical' is different for various functions. I've
> grouped errors in 4 groups:
>
> EINPROGRESS - sio_connect
>
> EADDRINUSE - sio_bind, sio_listen
>
> EAGAIN - sio_accept, sio_write,
> EWOULDBLOCK sio_writev, sio_readn_ahead,
> EINTR sio_sendto, sio_recvfrom
>
> ECONNRESET - sio_read
> EAGAIN
> EWOULDBLOCK
> EINTR
>
> I can not implement one function for checking of any
> error via errno because
>
> * EADDRINUSE is not critical for bind/listen, but
> critical for connect;
>
> * EAGAIN, EINTR are not critical for IO, but critical
> for connect;
>
> * ECONNRESET is not critical for sio_read, but critical
> for sio_sendto.
>
> The list is not complete I think, and I do not want to fix some
> crutches in a single error checking function each time when
> something changed in any other sio function.
OK, I see. I'll think what we can do about it.
>
> I can not use the simplest way !diag_is_empty(diag_get()) as well
> because as I discovered, iproto thread does not clean diag in
> some places after processing an error. For example,
> iproto_connection_resume on error leaves an error in diag and
> the error lives in the thread until next error. The same in
> iproto_connection_on_input(), where catch() block does not
> clears diag.
>
> I could use diag if I fixed these places, but I do not want
> to rely on cleanness of external code.
>
> I decided to return from some functions a special code that
> can be checked by sio_is_error_fatal().
>
> See v2 thread for a new version of the commit. I do not paste
> diff here, since in fact the patch is rewritten completely.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 04/11] sio: fix passing negative size_t to sio_add_to_iov
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (4 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 03/11] sio: remove exceptions Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-12-03 13:50 ` Vladimir Davydov
2018-11-30 15:39 ` [PATCH 05/11] sio: turn into C Vladislav Shpilevoy
` (5 subsequent siblings)
11 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
sio_add_to_iov moves struct iov position on a
specified offset, positive or negative. But its offset
argument has size_t type, which is unsigned. Make it
be ssize_t.
This worked before thanks to how negative numbers are
stored. For example, consider
uint8_t value = 100;
uint8_t offset = -5;
Value is stored as 0110 0100.
Offset is stored as 1111 1011. (Yes, 1011, not 1010).
Sum of the values above is 0001 0101 1111 - first quad
overflows and is truncated, so the result is
0101 1111 = 95 - correct.
---
src/sio.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/sio.h b/src/sio.h
index ab0a243cd..ff383aa36 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -84,7 +84,7 @@ sio_move_iov(struct iovec *iov, size_t nwr, size_t *iov_len)
* to adjust to a partial write.
*/
static inline void
-sio_add_to_iov(struct iovec *iov, size_t size)
+sio_add_to_iov(struct iovec *iov, ssize_t size)
{
iov->iov_len += size;
iov->iov_base = (char *) iov->iov_base - size;
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 04/11] sio: fix passing negative size_t to sio_add_to_iov
2018-11-30 15:39 ` [PATCH 04/11] sio: fix passing negative size_t to sio_add_to_iov Vladislav Shpilevoy
@ 2018-12-03 13:50 ` Vladimir Davydov
2018-12-04 21:29 ` Vladislav Shpilevoy
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 13:50 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:36PM +0300, Vladislav Shpilevoy wrote:
> sio_add_to_iov moves struct iov position on a
> specified offset, positive or negative. But its offset
> argument has size_t type, which is unsigned. Make it
> be ssize_t.
>
> This worked before thanks to how negative numbers are
> stored. For example, consider
>
> uint8_t value = 100;
> uint8_t offset = -5;
>
> Value is stored as 0110 0100.
> Offset is stored as 1111 1011. (Yes, 1011, not 1010).
>
> Sum of the values above is 0001 0101 1111 - first quad
> overflows and is truncated, so the result is
> 0101 1111 = 95 - correct.
> ---
> src/sio.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/sio.h b/src/sio.h
> index ab0a243cd..ff383aa36 100644
> --- a/src/sio.h
> +++ b/src/sio.h
> @@ -84,7 +84,7 @@ sio_move_iov(struct iovec *iov, size_t nwr, size_t *iov_len)
> * to adjust to a partial write.
> */
> static inline void
> -sio_add_to_iov(struct iovec *iov, size_t size)
> +sio_add_to_iov(struct iovec *iov, ssize_t size)
> {
> iov->iov_len += size;
'iov_len' has type size_t so 'size' will be converted to size_t before
the operation, in other words this patch has, in fact, no effect.
Anyway, it's OK to apply unary minus to an unsigned variable: no matter
how integer types are stored, whether the machine uses two's-complement
or not, it should work so that (-x + x) equals 0.
That being said, I don't think we need this patch.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 04/11] sio: fix passing negative size_t to sio_add_to_iov
2018-12-03 13:50 ` Vladimir Davydov
@ 2018-12-04 21:29 ` Vladislav Shpilevoy
2018-12-05 8:48 ` Vladimir Davydov
0 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:29 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
On 03/12/2018 16:50, Vladimir Davydov wrote:
> On Fri, Nov 30, 2018 at 06:39:36PM +0300, Vladislav Shpilevoy wrote:
>> sio_add_to_iov moves struct iov position on a
>> specified offset, positive or negative. But its offset
>> argument has size_t type, which is unsigned. Make it
>> be ssize_t.
>>
>> This worked before thanks to how negative numbers are
>> stored. For example, consider
>>
>> uint8_t value = 100;
>> uint8_t offset = -5;
>>
>> Value is stored as 0110 0100.
>> Offset is stored as 1111 1011. (Yes, 1011, not 1010).
>>
>> Sum of the values above is 0001 0101 1111 - first quad
>> overflows and is truncated, so the result is
>> 0101 1111 = 95 - correct.
>> ---
>> src/sio.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/sio.h b/src/sio.h
>> index ab0a243cd..ff383aa36 100644
>> --- a/src/sio.h
>> +++ b/src/sio.h
>> @@ -84,7 +84,7 @@ sio_move_iov(struct iovec *iov, size_t nwr, size_t *iov_len)
>> * to adjust to a partial write.
>> */
>> static inline void
>> -sio_add_to_iov(struct iovec *iov, size_t size)
>> +sio_add_to_iov(struct iovec *iov, ssize_t size)
>> {
>> iov->iov_len += size;
>
> 'iov_len' has type size_t so 'size' will be converted to size_t before
> the operation, in other words this patch has, in fact, no effect.
It fixes corrupted logic - you should not accept negative numbers in
positive types. Even if they are the same internally.
>
> Anyway, it's OK to apply unary minus to an unsigned variable: no matter
> how integer types are stored, whether the machine uses two's-complement
> or not, it should work so that (-x + x) equals 0.
I do not argue with it. But then why do we have signed types? Lets use
unsigned everywhere (<sarcasm>).
It does not matter does a compiler allow to turn a number into
the complement or not. Logic of storing negative numbers in
positive types is corrupted by definition.
>
> That being said, I don't think we need this patch.
>
As you wish. Removed in v2.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 04/11] sio: fix passing negative size_t to sio_add_to_iov
2018-12-04 21:29 ` Vladislav Shpilevoy
@ 2018-12-05 8:48 ` Vladimir Davydov
0 siblings, 0 replies; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-05 8:48 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:29:20AM +0300, Vladislav Shpilevoy wrote:
>
>
> On 03/12/2018 16:50, Vladimir Davydov wrote:
> > On Fri, Nov 30, 2018 at 06:39:36PM +0300, Vladislav Shpilevoy wrote:
> > > sio_add_to_iov moves struct iov position on a
> > > specified offset, positive or negative. But its offset
> > > argument has size_t type, which is unsigned. Make it
> > > be ssize_t.
> > >
> > > This worked before thanks to how negative numbers are
> > > stored. For example, consider
> > >
> > > uint8_t value = 100;
> > > uint8_t offset = -5;
> > >
> > > Value is stored as 0110 0100.
> > > Offset is stored as 1111 1011. (Yes, 1011, not 1010).
> > >
> > > Sum of the values above is 0001 0101 1111 - first quad
> > > overflows and is truncated, so the result is
> > > 0101 1111 = 95 - correct.
> > > ---
> > > src/sio.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/sio.h b/src/sio.h
> > > index ab0a243cd..ff383aa36 100644
> > > --- a/src/sio.h
> > > +++ b/src/sio.h
> > > @@ -84,7 +84,7 @@ sio_move_iov(struct iovec *iov, size_t nwr, size_t *iov_len)
> > > * to adjust to a partial write.
> > > */
> > > static inline void
> > > -sio_add_to_iov(struct iovec *iov, size_t size)
> > > +sio_add_to_iov(struct iovec *iov, ssize_t size)
> > > {
> > > iov->iov_len += size;
> >
> > 'iov_len' has type size_t so 'size' will be converted to size_t before
> > the operation, in other words this patch has, in fact, no effect.
>
> It fixes corrupted logic - you should not accept negative numbers in
> positive types. Even if they are the same internally.
>
> >
> > Anyway, it's OK to apply unary minus to an unsigned variable: no matter
> > how integer types are stored, whether the machine uses two's-complement
> > or not, it should work so that (-x + x) equals 0.
>
> I do not argue with it. But then why do we have signed types? Lets use
> unsigned everywhere (<sarcasm>).
>
> It does not matter does a compiler allow to turn a number into
> the complement or not. Logic of storing negative numbers in
> positive types is corrupted by definition.
But we do it all the time when adding a negative value to an unsigned
variable. Nobody cares since it conforms to the C standard.
>
> >
> > That being said, I don't think we need this patch.
> >
>
> As you wish. Removed in v2.
Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 05/11] sio: turn into C
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (5 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 04/11] sio: fix passing negative size_t to sio_add_to_iov Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-11-30 15:39 ` [PATCH 06/11] evio: make on_accept be nothrow Vladislav Shpilevoy
` (4 subsequent siblings)
11 siblings, 0 replies; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
---
src/CMakeLists.txt | 2 +-
src/exception.h | 2 +-
src/{sio.cc => sio.c} | 0
src/sio.h | 5 ++---
4 files changed, 4 insertions(+), 5 deletions(-)
rename src/{sio.cc => sio.c} (100%)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 7d0734f55..77353b244 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -88,7 +88,7 @@ set (core_sources
fiber_cond.c
fiber_channel.c
latch.c
- sio.cc
+ sio.c
evio.cc
coio.cc
coio_task.c
diff --git a/src/exception.h b/src/exception.h
index f08d946b5..e680e40af 100644
--- a/src/exception.h
+++ b/src/exception.h
@@ -50,6 +50,7 @@ extern const struct type_info type_LuajitError;
extern const struct type_info type_IllegalParams;
extern const struct type_info type_SystemError;
extern const struct type_info type_CollationError;
+extern const struct type_info type_SocketError;
const char *
exception_get_string(struct error *e, const struct method_info *method);
@@ -97,7 +98,6 @@ protected:
int m_errno;
};
-extern const struct type_info type_SocketError;
class SocketError: public SystemError {
public:
SocketError(const char *file, unsigned line, const char *socketname,
diff --git a/src/sio.cc b/src/sio.c
similarity index 100%
rename from src/sio.cc
rename to src/sio.c
diff --git a/src/sio.h b/src/sio.h
index ff383aa36..1978ca56d 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -90,9 +90,6 @@ sio_add_to_iov(struct iovec *iov, ssize_t size)
iov->iov_base = (char *) iov->iov_base - size;
}
-#if defined(__cplusplus)
-} /* extern "C" */
-
/** Pretty format socket name and peer. */
const char *
sio_socketname(int fd);
@@ -256,6 +253,8 @@ sio_recvfrom(int fd, void *buf, size_t len, int flags,
struct sockaddr *src_addr, socklen_t *addrlen,
bool *is_error_fatal);
+#if defined(__cplusplus)
+} /* extern "C" */
#endif /* defined(__cplusplus) */
#endif /* TARANTOOL_SIO_H_INCLUDED */
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 06/11] evio: make on_accept be nothrow
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (6 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 05/11] sio: turn into C Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-12-03 14:58 ` Vladimir Davydov
2018-11-30 15:39 ` [PATCH 07/11] coio: fix file descriptor leak on accept Vladislav Shpilevoy
` (3 subsequent siblings)
11 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Evio is going to be C, because it is needed in SWIM to
1) support UNIX sockets in future;
2) do not care about setting SocketError directly. It
is not possible via bare sio, because sio_bind ignores
EADDRINUSE.
A first step to make evio C - eliminate exceptions
from its callbacks available to other modules. The
only callback it has - on_accept.
Needed for #3234
---
src/box/iproto.cc | 17 +++++++----------
src/coio.cc | 13 ++++++-------
src/evio.cc | 13 ++++++-------
src/evio.h | 15 ++++++++-------
4 files changed, 27 insertions(+), 31 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 07ef23cac..dd76e28bd 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1791,7 +1791,7 @@ static const struct cmsg_hop connect_route[] = {
/**
* Create a connection and start input.
*/
-static void
+static int
iproto_on_accept(struct evio_service * /* service */, int fd,
struct sockaddr *addr, socklen_t addrlen)
{
@@ -1800,26 +1800,23 @@ iproto_on_accept(struct evio_service * /* service */, int fd,
struct iproto_msg *msg;
struct iproto_connection *con = iproto_connection_new(fd);
if (con == NULL)
- goto error_conn;
+ return -1;
/*
* Ignore msg allocation failure - the queue size is
* fixed so there is a limited number of msgs in
* use, all stored in just a few blocks of the memory pool.
*/
msg = iproto_msg_new(con);
- if (msg == NULL)
- goto error_msg;
+ if (msg == NULL) {
+ mempool_free(&iproto_connection_pool, con);
+ return -1;
+ }
cmsg_init(&msg->base, connect_route);
msg->p_ibuf = con->p_ibuf;
msg->wpos = con->wpos;
msg->close_connection = false;
cpipe_push(&tx_pipe, &msg->base);
- return;
-error_msg:
- mempool_free(&iproto_connection_pool, con);
-error_conn:
- close(fd);
- return;
+ return 0;
}
static struct evio_service binary; /* iproto binary listener */
diff --git a/src/coio.cc b/src/coio.cc
index b69f5e31a..49ac10b70 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -596,7 +596,7 @@ coio_recvfrom_timeout(struct ev_io *coio, void *buf, size_t sz, int flags,
}
}
-void
+static int
coio_service_on_accept(struct evio_service *evio_service,
int fd, struct sockaddr *addr, socklen_t addrlen)
{
@@ -612,14 +612,12 @@ coio_service_on_accept(struct evio_service *evio_service,
"%s/%s", evio_service->name, sio_strfaddr(addr, addrlen));
/* Create the worker fiber. */
- struct fiber *f;
- try {
- f = fiber_new_xc(fiber_name, service->handler);
- } catch (struct error *e) {
- error_log(e);
+ struct fiber *f = fiber_new(fiber_name, service->handler);
+ if (f == NULL) {
+ diag_log();
say_error("can't create a handler fiber, dropping client connection");
evio_close(loop(), &coio);
- throw;
+ return -1;
}
/*
* The coio is passed into the created fiber, reset the
@@ -631,6 +629,7 @@ coio_service_on_accept(struct evio_service *evio_service,
* and will have to close it and free before termination.
*/
fiber_start(f, coio, addr, addrlen, service->handler_param);
+ return 0;
}
void
diff --git a/src/evio.cc b/src/evio.cc
index 401e71155..4b7d37281 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -196,8 +196,10 @@ evio_service_accept_cb(ev_loop * /* loop */, ev_io *watcher,
* Invoke the callback and pass it the accepted
* socket.
*/
- service->on_accept(service, fd, (struct sockaddr *)&addr, addrlen);
-
+ if (service->on_accept(service, fd,
+ (struct sockaddr *)&addr,
+ addrlen) != 0)
+ diag_raise();
} catch (Exception *e) {
if (fd >= 0)
close(fd);
@@ -304,11 +306,8 @@ evio_service_listen(struct evio_service *service)
}
void
-evio_service_init(ev_loop *loop,
- struct evio_service *service, const char *name,
- void (*on_accept)(struct evio_service *, int,
- struct sockaddr *, socklen_t),
- void *on_accept_param)
+evio_service_init(ev_loop *loop, struct evio_service *service, const char *name,
+ evio_accept_f on_accept, void *on_accept_param)
{
memset(service, 0, sizeof(struct evio_service));
snprintf(service->name, sizeof(service->name), "%s", name);
diff --git a/src/evio.h b/src/evio.h
index e91ba11fc..f6c3a3a3e 100644
--- a/src/evio.h
+++ b/src/evio.h
@@ -62,6 +62,11 @@
* If a service is not started, but only initialized, no
* dedicated cleanup/destruction is necessary.
*/
+struct evio_service;
+
+typedef int (*evio_accept_f)(struct evio_service *, int, struct sockaddr *,
+ socklen_t);
+
struct evio_service
{
/** Service name. E.g. 'primary', 'secondary', etc. */
@@ -83,8 +88,7 @@ struct evio_service
* when it happens, the exception is logged, and the
* accepted socket is closed.
*/
- void (*on_accept)(struct evio_service *, int,
- struct sockaddr *, socklen_t);
+ evio_accept_f on_accept;
void *on_accept_param;
/** libev io object for the acceptor socket. */
@@ -94,11 +98,8 @@ struct evio_service
/** Initialize the service. Don't bind to the port yet. */
void
-evio_service_init(ev_loop *loop,
- struct evio_service *service, const char *name,
- void (*on_accept)(struct evio_service *,
- int, struct sockaddr *, socklen_t),
- void *on_accept_param);
+evio_service_init(ev_loop *loop, struct evio_service *service, const char *name,
+ evio_accept_f on_accept, void *on_accept_param);
/** Bind service to specified uri */
void
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 06/11] evio: make on_accept be nothrow
2018-11-30 15:39 ` [PATCH 06/11] evio: make on_accept be nothrow Vladislav Shpilevoy
@ 2018-12-03 14:58 ` Vladimir Davydov
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 14:58 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:38PM +0300, Vladislav Shpilevoy wrote:
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index 07ef23cac..dd76e28bd 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1800,26 +1800,23 @@ iproto_on_accept(struct evio_service * /* service */, int fd,
> struct iproto_msg *msg;
> struct iproto_connection *con = iproto_connection_new(fd);
> if (con == NULL)
> - goto error_conn;
> + return -1;
> /*
> * Ignore msg allocation failure - the queue size is
> * fixed so there is a limited number of msgs in
> * use, all stored in just a few blocks of the memory pool.
> */
> msg = iproto_msg_new(con);
> - if (msg == NULL)
> - goto error_msg;
> + if (msg == NULL) {
> + mempool_free(&iproto_connection_pool, con);
> + return -1;
> + }
> cmsg_init(&msg->base, connect_route);
> msg->p_ibuf = con->p_ibuf;
> msg->wpos = con->wpos;
> msg->close_connection = false;
> cpipe_push(&tx_pipe, &msg->base);
> - return;
> -error_msg:
> - mempool_free(&iproto_connection_pool, con);
> -error_conn:
> - close(fd);
> - return;
> + return 0;
You don't close the file descriptor on error anymore. I guess it's OK,
because evio_service_accept_cb will close it anyway.
> @@ -612,14 +612,12 @@ coio_service_on_accept(struct evio_service *evio_service,
> "%s/%s", evio_service->name, sio_strfaddr(addr, addrlen));
>
> /* Create the worker fiber. */
> - struct fiber *f;
> - try {
> - f = fiber_new_xc(fiber_name, service->handler);
> - } catch (struct error *e) {
> - error_log(e);
> + struct fiber *f = fiber_new(fiber_name, service->handler);
> + if (f == NULL) {
> + diag_log();
> say_error("can't create a handler fiber, dropping client connection");
> evio_close(loop(), &coio);
However, you do close fd here, via evio_close(). Care to remove it?
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 06/11] evio: make on_accept be nothrow
2018-12-03 14:58 ` Vladimir Davydov
@ 2018-12-04 21:29 ` Vladislav Shpilevoy
2018-12-05 8:52 ` Vladimir Davydov
0 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:29 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
On 03/12/2018 17:58, Vladimir Davydov wrote:
> On Fri, Nov 30, 2018 at 06:39:38PM +0300, Vladislav Shpilevoy wrote:
>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
>> index 07ef23cac..dd76e28bd 100644
>> --- a/src/box/iproto.cc
>> +++ b/src/box/iproto.cc
>> @@ -1800,26 +1800,23 @@ iproto_on_accept(struct evio_service * /* service */, int fd,
>> struct iproto_msg *msg;
>> struct iproto_connection *con = iproto_connection_new(fd);
>> if (con == NULL)
>> - goto error_conn;
>> + return -1;
>> /*
>> * Ignore msg allocation failure - the queue size is
>> * fixed so there is a limited number of msgs in
>> * use, all stored in just a few blocks of the memory pool.
>> */
>> msg = iproto_msg_new(con);
>> - if (msg == NULL)
>> - goto error_msg;
>> + if (msg == NULL) {
>> + mempool_free(&iproto_connection_pool, con);
>> + return -1;
>> + }
>> cmsg_init(&msg->base, connect_route);
>> msg->p_ibuf = con->p_ibuf;
>> msg->wpos = con->wpos;
>> msg->close_connection = false;
>> cpipe_push(&tx_pipe, &msg->base);
>> - return;
>> -error_msg:
>> - mempool_free(&iproto_connection_pool, con);
>> -error_conn:
>> - close(fd);
>> - return;
>> + return 0;
>
> You don't close the file descriptor on error anymore. I guess it's OK,
> because evio_service_accept_cb will close it anyway.
Yes, I do not close it since evio_service_accept_cb closes it now. Before
my patch the socket was closed inside iproto_on_accept because it was
exception free. Evio never closed a socket, accepted by iproto_on_accept.
Now a necessity to close a socket is determined by returned value instead of
an exception and I can not leave this close here.
>
>> @@ -612,14 +612,12 @@ coio_service_on_accept(struct evio_service *evio_service,
>> "%s/%s", evio_service->name, sio_strfaddr(addr, addrlen));
>>
>> /* Create the worker fiber. */
>> - struct fiber *f;
>> - try {
>> - f = fiber_new_xc(fiber_name, service->handler);
>> - } catch (struct error *e) {
>> - error_log(e);
>> + struct fiber *f = fiber_new(fiber_name, service->handler);
>> + if (f == NULL) {
>> + diag_log();
>> say_error("can't create a handler fiber, dropping client connection");
>> evio_close(loop(), &coio);
>
> However, you do close fd here, via evio_close(). Care to remove it?
>
It is a separate bug that I found and fixed in a next commit. This patch
does not fix any bugs. It is pure refactoring.
In v2 the patch is left as is.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 06/11] evio: make on_accept be nothrow
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-05 8:52 ` Vladimir Davydov
0 siblings, 0 replies; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-05 8:52 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:29:14AM +0300, Vladislav Shpilevoy wrote:
>
>
> On 03/12/2018 17:58, Vladimir Davydov wrote:
> > On Fri, Nov 30, 2018 at 06:39:38PM +0300, Vladislav Shpilevoy wrote:
> > > diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> > > index 07ef23cac..dd76e28bd 100644
> > > --- a/src/box/iproto.cc
> > > +++ b/src/box/iproto.cc
> > > @@ -1800,26 +1800,23 @@ iproto_on_accept(struct evio_service * /* service */, int fd,
> > > struct iproto_msg *msg;
> > > struct iproto_connection *con = iproto_connection_new(fd);
> > > if (con == NULL)
> > > - goto error_conn;
> > > + return -1;
> > > /*
> > > * Ignore msg allocation failure - the queue size is
> > > * fixed so there is a limited number of msgs in
> > > * use, all stored in just a few blocks of the memory pool.
> > > */
> > > msg = iproto_msg_new(con);
> > > - if (msg == NULL)
> > > - goto error_msg;
> > > + if (msg == NULL) {
> > > + mempool_free(&iproto_connection_pool, con);
> > > + return -1;
> > > + }
> > > cmsg_init(&msg->base, connect_route);
> > > msg->p_ibuf = con->p_ibuf;
> > > msg->wpos = con->wpos;
> > > msg->close_connection = false;
> > > cpipe_push(&tx_pipe, &msg->base);
> > > - return;
> > > -error_msg:
> > > - mempool_free(&iproto_connection_pool, con);
> > > -error_conn:
> > > - close(fd);
> > > - return;
> > > + return 0;
> >
> > You don't close the file descriptor on error anymore. I guess it's OK,
> > because evio_service_accept_cb will close it anyway.
>
> Yes, I do not close it since evio_service_accept_cb closes it now. Before
> my patch the socket was closed inside iproto_on_accept because it was
> exception free. Evio never closed a socket, accepted by iproto_on_accept.
> Now a necessity to close a socket is determined by returned value instead of
> an exception and I can not leave this close here.
OK, I see. Turns out the iproto_on_accept function doesn't raise an
exception on error, which isn't correct from the point of on_accept
callback protocol. I didn't notice that. Worth mentioning in the
commit message.
>
> >
> > > @@ -612,14 +612,12 @@ coio_service_on_accept(struct evio_service *evio_service,
> > > "%s/%s", evio_service->name, sio_strfaddr(addr, addrlen));
> > > /* Create the worker fiber. */
> > > - struct fiber *f;
> > > - try {
> > > - f = fiber_new_xc(fiber_name, service->handler);
> > > - } catch (struct error *e) {
> > > - error_log(e);
> > > + struct fiber *f = fiber_new(fiber_name, service->handler);
> > > + if (f == NULL) {
> > > + diag_log();
> > > say_error("can't create a handler fiber, dropping client connection");
> > > evio_close(loop(), &coio);
> >
> > However, you do close fd here, via evio_close(). Care to remove it?
> >
>
> It is a separate bug that I found and fixed in a next commit. This patch
> does not fix any bugs. It is pure refactoring.
>
> In v2 the patch is left as is.
OK.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 07/11] coio: fix file descriptor leak on accept
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (7 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 06/11] evio: make on_accept be nothrow Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-12-03 16:16 ` Vladimir Davydov
2018-11-30 15:39 ` [PATCH 08/11] coio: fix double close of a file descriptor Vladislav Shpilevoy
` (2 subsequent siblings)
11 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
coio_accept() calls evio_setsockopt_client, which
throws an exception and just accepted socket leaks.
Yes, server socket is protected, but not new client
socket.
---
src/coio.cc | 7 +++++--
src/evio.cc | 49 ++++++++++++++++++++++++++++---------------------
src/evio.h | 2 +-
3 files changed, 34 insertions(+), 24 deletions(-)
diff --git a/src/coio.cc b/src/coio.cc
index 49ac10b70..575bae712 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -254,8 +254,11 @@ coio_accept(struct ev_io *coio, struct sockaddr *addr,
int fd = sio_accept(coio->fd, addr, &addrlen,
&is_error_fatal);
if (fd >= 0) {
- evio_setsockopt_client(fd, addr->sa_family,
- SOCK_STREAM);
+ if (evio_setsockopt_client(fd, addr->sa_family,
+ SOCK_STREAM) != 0) {
+ close(fd);
+ diag_raise();
+ }
return fd;
}
if (is_error_fatal)
diff --git a/src/evio.cc b/src/evio.cc
index 4b7d37281..25f699bab 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -64,14 +64,17 @@ void
evio_socket(struct ev_io *coio, int domain, int type, int protocol)
{
assert(coio->fd == -1);
- /* Don't leak fd if setsockopt fails. */
coio->fd = sio_socket(domain, type, protocol);
if (coio->fd < 0)
diag_raise();
- evio_setsockopt_client(coio->fd, domain, type);
+ if (evio_setsockopt_client(coio->fd, domain, type) != 0) {
+ close(coio->fd);
+ coio->fd = -1;
+ diag_raise();
+ }
}
-static void
+static int
evio_setsockopt_keepalive(int fd)
{
int on = 1;
@@ -79,9 +82,8 @@ evio_setsockopt_keepalive(int fd)
* SO_KEEPALIVE to ensure connections don't hang
* around for too long when a link goes away.
*/
- if (sio_setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE,
- &on, sizeof(on)))
- diag_raise();
+ if (sio_setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, &on, sizeof(on)) != 0)
+ return -1;
#ifdef __linux__
/*
* On Linux, we are able to fine-tune keepalive
@@ -90,44 +92,46 @@ evio_setsockopt_keepalive(int fd)
*/
int keepcnt = 5;
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &keepcnt,
- sizeof(int)))
- diag_raise();
+ sizeof(int)) != 0)
+ return -1;
int keepidle = 30;
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle,
- sizeof(int)))
- diag_raise();
+ sizeof(int)) != 0)
+ return -1;
int keepintvl = 60;
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, &keepintvl,
- sizeof(int)))
- diag_raise();
+ sizeof(int)) != 0)
+ return -1;
#endif
+ return 0;
}
/** Set common client socket options. */
-void
+int
evio_setsockopt_client(int fd, int family, int type)
{
int on = 1;
- /* In case this throws, the socket is not leaked. */
if (sio_setfl(fd, O_NONBLOCK) != 0)
- diag_raise();
+ return -1;
if (type == SOCK_STREAM && family != AF_UNIX) {
/*
* SO_KEEPALIVE to ensure connections don't hang
* around for too long when a link goes away.
*/
- evio_setsockopt_keepalive(fd);
+ if (evio_setsockopt_keepalive(fd) != 0)
+ return -1;
/*
* Lower latency is more important than higher
* bandwidth, and we usually write entire
* request/response in a single syscall.
*/
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_NODELAY,
- &on, sizeof(on)))
- diag_raise();
+ &on, sizeof(on)) != 0)
+ return -1;
}
+ return 0;
}
/** Set options for server sockets. */
@@ -150,8 +154,9 @@ evio_setsockopt_server(int fd, int family, int type)
if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
&linger, sizeof(linger)))
diag_raise();
- if (type == SOCK_STREAM && family != AF_UNIX)
- evio_setsockopt_keepalive(fd);
+ if (type == SOCK_STREAM && family != AF_UNIX &&
+ evio_setsockopt_keepalive(fd) != 0)
+ diag_raise();
}
static inline const char *
@@ -191,7 +196,9 @@ evio_service_accept_cb(ev_loop * /* loop */, ev_io *watcher,
return;
}
/* set common client socket options */
- evio_setsockopt_client(fd, service->addr.sa_family, SOCK_STREAM);
+ if (evio_setsockopt_client(fd, service->addr.sa_family,
+ SOCK_STREAM) != 0)
+ diag_raise();
/*
* Invoke the callback and pass it the accepted
* socket.
diff --git a/src/evio.h b/src/evio.h
index f6c3a3a3e..9f80e84a5 100644
--- a/src/evio.h
+++ b/src/evio.h
@@ -150,7 +150,7 @@ evio_timeout_update(ev_loop *loop, ev_tstamp start, ev_tstamp *delay)
*delay = (elapsed >= *delay) ? 0 : *delay - elapsed;
}
-void
+int
evio_setsockopt_client(int fd, int family, int type);
#endif /* TARANTOOL_EVIO_H_INCLUDED */
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 07/11] coio: fix file descriptor leak on accept
2018-11-30 15:39 ` [PATCH 07/11] coio: fix file descriptor leak on accept Vladislav Shpilevoy
@ 2018-12-03 16:16 ` Vladimir Davydov
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 16:16 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:39PM +0300, Vladislav Shpilevoy wrote:
> @@ -90,44 +92,46 @@ evio_setsockopt_keepalive(int fd)
> */
> int keepcnt = 5;
> if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &keepcnt,
> - sizeof(int)))
> - diag_raise();
> + sizeof(int)) != 0)
> + return -1;
> int keepidle = 30;
>
> if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle,
> - sizeof(int)))
> - diag_raise();
> + sizeof(int)) != 0)
> + return -1;
>
> int keepintvl = 60;
> if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, &keepintvl,
> - sizeof(int)))
> - diag_raise();
> + sizeof(int)) != 0)
> + return -1;
> #endif
> + return 0;
> }
>
> /** Set common client socket options. */
> -void
> +int
> evio_setsockopt_client(int fd, int family, int type)
> {
> int on = 1;
> - /* In case this throws, the socket is not leaked. */
> if (sio_setfl(fd, O_NONBLOCK) != 0)
> - diag_raise();
> + return -1;
> if (type == SOCK_STREAM && family != AF_UNIX) {
> /*
> * SO_KEEPALIVE to ensure connections don't hang
> * around for too long when a link goes away.
> */
> - evio_setsockopt_keepalive(fd);
> + if (evio_setsockopt_keepalive(fd) != 0)
> + return -1;
> /*
> * Lower latency is more important than higher
> * bandwidth, and we usually write entire
> * request/response in a single syscall.
> */
> if (sio_setsockopt(fd, IPPROTO_TCP, TCP_NODELAY,
> - &on, sizeof(on)))
> - diag_raise();
> + &on, sizeof(on)) != 0)
> + return -1;
> }
> + return 0;
> }
>
> /** Set options for server sockets. */
> @@ -150,8 +154,9 @@ evio_setsockopt_server(int fd, int family, int type)
> if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
> &linger, sizeof(linger)))
> diag_raise();
> - if (type == SOCK_STREAM && family != AF_UNIX)
> - evio_setsockopt_keepalive(fd);
> + if (type == SOCK_STREAM && family != AF_UNIX &&
> + evio_setsockopt_keepalive(fd) != 0)
> + diag_raise();
> }
I don't like this patch, because apart from fixing the leak, it also
removes exceptions from some evio functions, but not all of them, e.g.
evio_setsockopt_keepalive and evio_setsockopt_client are now exception
free while evio_setsockopt_server is not. And then you have a separate
patch that removes the rest of exception from evio.
Let's please first remove all exceptions from evio in one go, and only
then fix the leak?
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 07/11] coio: fix file descriptor leak on accept
2018-12-03 16:16 ` Vladimir Davydov
@ 2018-12-04 21:29 ` Vladislav Shpilevoy
0 siblings, 0 replies; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:29 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
On 03/12/2018 19:16, Vladimir Davydov wrote:
> On Fri, Nov 30, 2018 at 06:39:39PM +0300, Vladislav Shpilevoy wrote:
>> @@ -90,44 +92,46 @@ evio_setsockopt_keepalive(int fd)
>> */
>> int keepcnt = 5;
>> if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &keepcnt,
>> - sizeof(int)))
>> - diag_raise();
>> + sizeof(int)) != 0)
>> + return -1;
>> int keepidle = 30;
>>
>> if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle,
>> - sizeof(int)))
>> - diag_raise();
>> + sizeof(int)) != 0)
>> + return -1;
>>
>> int keepintvl = 60;
>> if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, &keepintvl,
>> - sizeof(int)))
>> - diag_raise();
>> + sizeof(int)) != 0)
>> + return -1;
>> #endif
>> + return 0;
>> }
>>
>> /** Set common client socket options. */
>> -void
>> +int
>> evio_setsockopt_client(int fd, int family, int type)
>> {
>> int on = 1;
>> - /* In case this throws, the socket is not leaked. */
>> if (sio_setfl(fd, O_NONBLOCK) != 0)
>> - diag_raise();
>> + return -1;
>> if (type == SOCK_STREAM && family != AF_UNIX) {
>> /*
>> * SO_KEEPALIVE to ensure connections don't hang
>> * around for too long when a link goes away.
>> */
>> - evio_setsockopt_keepalive(fd);
>> + if (evio_setsockopt_keepalive(fd) != 0)
>> + return -1;
>> /*
>> * Lower latency is more important than higher
>> * bandwidth, and we usually write entire
>> * request/response in a single syscall.
>> */
>> if (sio_setsockopt(fd, IPPROTO_TCP, TCP_NODELAY,
>> - &on, sizeof(on)))
>> - diag_raise();
>> + &on, sizeof(on)) != 0)
>> + return -1;
>> }
>> + return 0;
>> }
>>
>> /** Set options for server sockets. */
>> @@ -150,8 +154,9 @@ evio_setsockopt_server(int fd, int family, int type)
>> if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
>> &linger, sizeof(linger)))
>> diag_raise();
>> - if (type == SOCK_STREAM && family != AF_UNIX)
>> - evio_setsockopt_keepalive(fd);
>> + if (type == SOCK_STREAM && family != AF_UNIX &&
>> + evio_setsockopt_keepalive(fd) != 0)
>> + diag_raise();
>> }
>
> I don't like this patch, because apart from fixing the leak, it also
> removes exceptions from some evio functions, but not all of them, e.g.
> evio_setsockopt_keepalive and evio_setsockopt_client are now exception
> free while evio_setsockopt_server is not. And then you have a separate
> patch that removes the rest of exception from evio.
>
> Let's please first remove all exceptions from evio in one go, and only
> then fix the leak?
>
Done. See v2.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 08/11] coio: fix double close of a file descriptor
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (8 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 07/11] coio: fix file descriptor leak on accept Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-12-03 16:19 ` Vladimir Davydov
2018-11-30 15:39 ` [PATCH 09/11] evio: refactoring Vladislav Shpilevoy
2018-12-03 9:47 ` [PATCH 00/11] SWIM preparation Vladimir Davydov
11 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
coio_service_on_accept is called by evio by an
on_accept pointer. If evio obtains not zero from
on_accept pointer, it closes accepted socket. But
coio_service_on_accept closes it too, when fiber_new
fails. It is double close.
Note that the bug existed even when on_accept was able
to throw.
---
src/coio.cc | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/coio.cc b/src/coio.cc
index 575bae712..a888a54dd 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -605,9 +605,6 @@ coio_service_on_accept(struct evio_service *evio_service,
{
struct coio_service *service = (struct coio_service *)
evio_service->on_accept_param;
- struct ev_io coio;
-
- coio_create(&coio, fd);
/* Set connection name. */
char fiber_name[SERVICE_NAME_MAXLEN];
@@ -619,9 +616,10 @@ coio_service_on_accept(struct evio_service *evio_service,
if (f == NULL) {
diag_log();
say_error("can't create a handler fiber, dropping client connection");
- evio_close(loop(), &coio);
return -1;
}
+ struct ev_io coio;
+ coio_create(&coio, fd);
/*
* The coio is passed into the created fiber, reset the
* libev callback param to point at it.
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 08/11] coio: fix double close of a file descriptor
2018-11-30 15:39 ` [PATCH 08/11] coio: fix double close of a file descriptor Vladislav Shpilevoy
@ 2018-12-03 16:19 ` Vladimir Davydov
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 16:19 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:40PM +0300, Vladislav Shpilevoy wrote:
> coio_service_on_accept is called by evio by an
> on_accept pointer. If evio obtains not zero from
> on_accept pointer, it closes accepted socket. But
> coio_service_on_accept closes it too, when fiber_new
> fails. It is double close.
>
> Note that the bug existed even when on_accept was able
> to throw.
> ---
> src/coio.cc | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/src/coio.cc b/src/coio.cc
> index 575bae712..a888a54dd 100644
> --- a/src/coio.cc
> +++ b/src/coio.cc
> @@ -605,9 +605,6 @@ coio_service_on_accept(struct evio_service *evio_service,
> {
> struct coio_service *service = (struct coio_service *)
> evio_service->on_accept_param;
> - struct ev_io coio;
> -
> - coio_create(&coio, fd);
>
> /* Set connection name. */
> char fiber_name[SERVICE_NAME_MAXLEN];
> @@ -619,9 +616,10 @@ coio_service_on_accept(struct evio_service *evio_service,
> if (f == NULL) {
> diag_log();
> say_error("can't create a handler fiber, dropping client connection");
> - evio_close(loop(), &coio);
> return -1;
> }
> + struct ev_io coio;
> + coio_create(&coio, fd);
> /*
> * The coio is passed into the created fiber, reset the
> * libev callback param to point at it.
I see now that you remove an extra close() from coio_service_on_accept
in a separate patch. This is OK, but then please remove an extra close()
from iproto_on_accept in the same patch, not in patch 6, where you make
on_accept exception free.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 08/11] coio: fix double close of a file descriptor
2018-12-03 16:19 ` Vladimir Davydov
@ 2018-12-04 21:29 ` Vladislav Shpilevoy
0 siblings, 0 replies; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:29 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
On 03/12/2018 19:19, Vladimir Davydov wrote:
> On Fri, Nov 30, 2018 at 06:39:40PM +0300, Vladislav Shpilevoy wrote:
>> coio_service_on_accept is called by evio by an
>> on_accept pointer. If evio obtains not zero from
>> on_accept pointer, it closes accepted socket. But
>> coio_service_on_accept closes it too, when fiber_new
>> fails. It is double close.
>>
>> Note that the bug existed even when on_accept was able
>> to throw.
>> ---
>> src/coio.cc | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/coio.cc b/src/coio.cc
>> index 575bae712..a888a54dd 100644
>> --- a/src/coio.cc
>> +++ b/src/coio.cc
>> @@ -605,9 +605,6 @@ coio_service_on_accept(struct evio_service *evio_service,
>> {
>> struct coio_service *service = (struct coio_service *)
>> evio_service->on_accept_param;
>> - struct ev_io coio;
>> -
>> - coio_create(&coio, fd);
>>
>> /* Set connection name. */
>> char fiber_name[SERVICE_NAME_MAXLEN];
>> @@ -619,9 +616,10 @@ coio_service_on_accept(struct evio_service *evio_service,
>> if (f == NULL) {
>> diag_log();
>> say_error("can't create a handler fiber, dropping client connection");
>> - evio_close(loop(), &coio);
>> return -1;
>> }
>> + struct ev_io coio;
>> + coio_create(&coio, fd);
>> /*
>> * The coio is passed into the created fiber, reset the
>> * libev callback param to point at it.
>
> I see now that you remove an extra close() from coio_service_on_accept
> in a separate patch. This is OK, but then please remove an extra close()
> from iproto_on_accept in the same patch, not in patch 6, where you make
> on_accept exception free.
>
If I left close in iproto_on_accept in the patch making on_accept
nothrow, I would introduce a new bug - double close in iproto_on_accept.
So I can not do it. Even temporary. Patch about nothrow on_accept is
refactoring, this patch is bugfix.
In v2 the patch is left as is.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 09/11] evio: refactoring
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (9 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 08/11] coio: fix double close of a file descriptor Vladislav Shpilevoy
@ 2018-11-30 15:39 ` Vladislav Shpilevoy
2018-12-03 16:37 ` Vladimir Davydov
2018-12-03 9:47 ` [PATCH 00/11] SWIM preparation Vladimir Davydov
11 siblings, 1 reply; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-30 15:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Remove useless, wrong and obvious comments. Correct
code and comment style. Reuse some code, unnecessary
try/catch.
---
src/evio.cc | 186 ++++++++++++++++++++--------------------------------
src/evio.h | 40 ++++++-----
2 files changed, 89 insertions(+), 137 deletions(-)
diff --git a/src/evio.cc b/src/evio.cc
index 25f699bab..166ba7f95 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -29,7 +29,6 @@
* SUCH DAMAGE.
*/
#include "evio.h"
-#include "uri.h"
#include "scoped_guard.h"
#include <stdio.h>
#include <sys/socket.h>
@@ -41,41 +40,33 @@
#include <trivia/util.h>
#include "exception.h"
-static void
-evio_setsockopt_server(int fd, int family, int type);
-
-/** Note: this function does not throw. */
void
evio_close(ev_loop *loop, struct ev_io *evio)
{
/* Stop I/O events. Safe to do even if not started. */
ev_io_stop(loop, evio);
- /* Close the socket. */
- close(evio->fd);
- /* Make sure evio_has_fd() returns a proper value. */
- evio->fd = -1;
+ if (evio_has_fd(evio)) {
+ close(evio->fd);
+ ev_io_set(evio, -1, 0);
+ }
}
-/**
- * Create an endpoint for communication.
- * Set socket as non-block and apply protocol specific options.
- */
void
-evio_socket(struct ev_io *coio, int domain, int type, int protocol)
+evio_socket(struct ev_io *evio, int domain, int type, int protocol)
{
- assert(coio->fd == -1);
- coio->fd = sio_socket(domain, type, protocol);
- if (coio->fd < 0)
+ assert(! evio_has_fd(evio));
+ evio->fd = sio_socket(domain, type, protocol);
+ if (evio->fd < 0)
diag_raise();
- if (evio_setsockopt_client(coio->fd, domain, type) != 0) {
- close(coio->fd);
- coio->fd = -1;
+ if (evio_setsockopt_client(evio->fd, domain, type) != 0) {
+ close(evio->fd);
+ ev_io_set(evio, -1, 0);
diag_raise();
}
}
static int
-evio_setsockopt_keepalive(int fd)
+evio_setsockopt_keeping(int fd)
{
int on = 1;
/*
@@ -94,8 +85,8 @@ evio_setsockopt_keepalive(int fd)
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &keepcnt,
sizeof(int)) != 0)
return -1;
- int keepidle = 30;
+ int keepidle = 30;
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle,
sizeof(int)) != 0)
return -1;
@@ -108,7 +99,6 @@ evio_setsockopt_keepalive(int fd)
return 0;
}
-/** Set common client socket options. */
int
evio_setsockopt_client(int fd, int family, int type)
{
@@ -116,11 +106,7 @@ evio_setsockopt_client(int fd, int family, int type)
if (sio_setfl(fd, O_NONBLOCK) != 0)
return -1;
if (type == SOCK_STREAM && family != AF_UNIX) {
- /*
- * SO_KEEPALIVE to ensure connections don't hang
- * around for too long when a link goes away.
- */
- if (evio_setsockopt_keepalive(fd) != 0)
+ if (evio_setsockopt_keeping(fd) != 0)
return -1;
/*
* Lower latency is more important than higher
@@ -139,81 +125,63 @@ static void
evio_setsockopt_server(int fd, int family, int type)
{
int on = 1;
- /* In case this throws, the socket is not leaked. */
if (sio_setfl(fd, O_NONBLOCK) != 0)
diag_raise();
- /* Allow reuse local adresses. */
- if (sio_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
- &on, sizeof(on)))
+ if (sio_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)) != 0)
diag_raise();
- /* Send all buffered messages on socket before take
- * control out from close(2) or shutdown(2). */
+ /*
+ * Send all buffered messages on socket before take
+ * control out from close() or shutdown().
+ */
struct linger linger = { 0, 0 };
-
- if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
- &linger, sizeof(linger)))
+ if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER, &linger,
+ sizeof(linger)) != 0)
diag_raise();
if (type == SOCK_STREAM && family != AF_UNIX &&
- evio_setsockopt_keepalive(fd) != 0)
+ evio_setsockopt_keeping(fd) != 0)
diag_raise();
}
-static inline const char *
-evio_service_name(struct evio_service *service)
-{
- return service->name;
-}
-
/**
* A callback invoked by libev when acceptor socket is ready.
* Accept the socket, initialize it and pass to the on_accept
* callback.
*/
static void
-evio_service_accept_cb(ev_loop * /* loop */, ev_io *watcher,
- int /* revents */)
+evio_service_accept_cb(ev_loop *loop, ev_io *watcher, int events)
{
+ (void) loop;
+ (void) events;
struct evio_service *service = (struct evio_service *) watcher->data;
-
+ int fd;
while (1) {
/*
- * Accept all pending connections from backlog during event
- * loop iteration. Significally speed up acceptor with enabled
+ * Accept all pending connections from backlog
+ * during event loop iteration. Significally
+ * speed up acceptor with enabled
* io_collect_interval.
*/
- int fd = -1;
- try {
- struct sockaddr_storage addr;
- socklen_t addrlen = sizeof(addr);
- bool is_error_fatal;
- fd = sio_accept(service->ev.fd,
- (struct sockaddr *)&addr, &addrlen,
- &is_error_fatal);
- if (fd < 0) {
- if (is_error_fatal)
- diag_raise();
- return;
- }
- /* set common client socket options */
- if (evio_setsockopt_client(fd, service->addr.sa_family,
- SOCK_STREAM) != 0)
- diag_raise();
- /*
- * Invoke the callback and pass it the accepted
- * socket.
- */
- if (service->on_accept(service, fd,
- (struct sockaddr *)&addr,
- addrlen) != 0)
- diag_raise();
- } catch (Exception *e) {
- if (fd >= 0)
- close(fd);
- e->log();
+ struct sockaddr_storage addr;
+ socklen_t addrlen = sizeof(addr);
+ bool is_error_fatal;
+ fd = sio_accept(service->ev.fd, (struct sockaddr *)&addr,
+ &addrlen, &is_error_fatal);
+ if (fd < 0) {
+ if (is_error_fatal)
+ break;
return;
}
+ if (evio_setsockopt_client(fd, service->addr.sa_family,
+ SOCK_STREAM) != 0)
+ break;
+ if (service->on_accept(service, fd, (struct sockaddr *)&addr,
+ addrlen) != 0)
+ break;
}
+ diag_log();
+ if (fd >= 0)
+ close(fd);
}
/*
@@ -240,7 +208,7 @@ evio_service_reuse_addr(struct evio_service *service, int fd)
if (errno != ECONNREFUSED)
goto err;
- if (unlink(((struct sockaddr_un *)(&service->addr))->sun_path))
+ if (unlink(((struct sockaddr_un *)(&service->addr))->sun_path) != 0)
goto err;
close(cl_fd);
@@ -260,11 +228,10 @@ err:
static void
evio_service_bind_addr(struct evio_service *service)
{
- say_debug("%s: binding to %s...", evio_service_name(service),
+ say_debug("%s: binding to %s...", service->name,
sio_strfaddr(&service->addr, service->addr_len));
/* Create a socket. */
- int fd = sio_socket(service->addr.sa_family,
- SOCK_STREAM, IPPROTO_TCP);
+ int fd = sio_socket(service->addr.sa_family, SOCK_STREAM, IPPROTO_TCP);
if (fd < 0)
diag_raise();
@@ -286,7 +253,7 @@ evio_service_bind_addr(struct evio_service *service)
}
}
- say_info("%s: bound to %s", evio_service_name(service),
+ say_info("%s: bound to %s", service->name,
sio_strfaddr(&service->addr, service->addr_len));
/* Register the socket in the event loop. */
@@ -295,15 +262,10 @@ evio_service_bind_addr(struct evio_service *service)
fd_guard.is_active = false;
}
-/**
- * Listen on bounded port.
- *
- * @retval 0 for success
- */
void
evio_service_listen(struct evio_service *service)
{
- say_debug("%s: listening on %s...", evio_service_name(service),
+ say_debug("%s: listening on %s...", service->name,
sio_strfaddr(&service->addr, service->addr_len));
int fd = service->ev.fd;
@@ -332,28 +294,25 @@ evio_service_init(ev_loop *loop, struct evio_service *service, const char *name,
service->ev.data = service;
}
-/**
- * Try to bind.
- */
void
evio_service_bind(struct evio_service *service, const char *uri)
{
struct uri u;
- if (uri_parse(&u, uri) || u.service == NULL)
+ if (uri_parse(&u, uri) || u.service == NULL) {
tnt_raise(SocketError, sio_socketname(-1),
"invalid uri for bind: %s", uri);
+ }
snprintf(service->serv, sizeof(service->serv), "%.*s",
(int) u.service_len, u.service);
if (u.host != NULL && strncmp(u.host, "*", u.host_len) != 0) {
snprintf(service->host, sizeof(service->host), "%.*s",
- (int) u.host_len, u.host);
- } /* else { service->host[0] = '\0'; } */
+ (int) u.host_len, u.host);
+ }
assert(! ev_is_active(&service->ev));
if (strcmp(service->host, URI_HOST_UNIX) == 0) {
- /* UNIX domain socket */
struct sockaddr_un *un = (struct sockaddr_un *) &service->addr;
service->addr_len = sizeof(*un);
snprintf(un->sun_path, sizeof(un->sun_path), "%s",
@@ -362,18 +321,22 @@ evio_service_bind(struct evio_service *service, const char *uri)
return evio_service_bind_addr(service);
}
- /* IP socket */
+ /* IP socket. */
struct addrinfo hints, *res;
memset(&hints, 0, sizeof(hints));
hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_STREAM;
hints.ai_flags = AI_PASSIVE|AI_ADDRCONFIG;
- /* make no difference between empty string and NULL for host */
+ /*
+ * Make no difference between empty string and NULL for
+ * host.
+ */
if (getaddrinfo(*service->host ? service->host : NULL, service->serv,
- &hints, &res) != 0 || res == NULL)
+ &hints, &res) != 0 || res == NULL) {
tnt_raise(SocketError, sio_socketname(-1),
"can't resolve uri for bind");
+ }
auto addrinfo_guard = make_scoped_guard([=]{ freeaddrinfo(res); });
for (struct addrinfo *ai = res; ai != NULL; ai = ai->ai_next) {
@@ -382,32 +345,23 @@ evio_service_bind(struct evio_service *service, const char *uri)
try {
return evio_service_bind_addr(service);
} catch (SocketError *e) {
- say_error("%s: failed to bind on %s: %s",
- evio_service_name(service),
+ say_error("%s: failed to bind on %s: %s", service->name,
sio_strfaddr(ai->ai_addr, ai->ai_addrlen),
e->get_errmsg());
/* ignore */
}
}
tnt_raise(SocketError, sio_socketname(-1), "%s: failed to bind",
- evio_service_name(service));
+ service->name);
}
-/** It's safe to stop a service which is not started yet. */
void
evio_service_stop(struct evio_service *service)
{
- say_info("%s: stopped", evio_service_name(service));
-
- if (ev_is_active(&service->ev)) {
- ev_io_stop(service->loop, &service->ev);
- }
-
- if (service->ev.fd >= 0) {
- close(service->ev.fd);
- ev_io_set(&service->ev, -1, 0);
- if (service->addr.sa_family == AF_UNIX) {
- unlink(((struct sockaddr_un *) &service->addr)->sun_path);
- }
- }
+ say_info("%s: stopped", service->name);
+ bool unlink_unix = evio_has_fd(&service->ev) &&
+ service->addr.sa_family == AF_UNIX;
+ evio_close(service->loop, &service->ev);
+ if (unlink_unix)
+ unlink(((struct sockaddr_un *) &service->addr)->sun_path);
}
diff --git a/src/evio.h b/src/evio.h
index 9f80e84a5..ae2b9b9a8 100644
--- a/src/evio.h
+++ b/src/evio.h
@@ -39,15 +39,14 @@
#include "sio.h"
#include "uri.h"
/**
- * Exception-aware way to add a listening socket to the event
- * loop. Callbacks are invoked on bind and accept events.
+ * Exception-aware way to add a socket to the event loop.
*
- * Coroutines/fibers are not used for port listeners
- * since listener's job is usually simple and only involves
- * creating a session for the accepted socket. The session itself
- * can be built around simple libev callbacks, or around
- * cooperative multitasking (on_accept callback can create
- * a fiber and use coio.h (cooperative multi-tasking I/O)) API.
+ * Coroutines/fibers are not used for port listeners since
+ * listener's job is usually simple and only involves creating a
+ * session for the accepted socket. The session itself can be
+ * built around simple libev callbacks, or around cooperative
+ * multitasking (on_accept callback can create a fiber and use
+ * coio.h (cooperative multi-tasking I/O)) API.
*
* How to use a service:
* struct evio_service *service;
@@ -58,9 +57,6 @@
* ...
* evio_service_stop(service);
* free(service);
- *
- * If a service is not started, but only initialized, no
- * dedicated cleanup/destruction is necessary.
*/
struct evio_service;
@@ -101,15 +97,11 @@ void
evio_service_init(ev_loop *loop, struct evio_service *service, const char *name,
evio_accept_f on_accept, void *on_accept_param);
-/** Bind service to specified uri */
+/** Bind service to specified uri. */
void
evio_service_bind(struct evio_service *service, const char *uri);
-/**
- * Listen on bounded socket
- *
- * @retval 0 for success
- */
+/** Listen on bounded socket. */
void
evio_service_listen(struct evio_service *service);
@@ -117,22 +109,27 @@ evio_service_listen(struct evio_service *service);
void
evio_service_stop(struct evio_service *service);
+/**
+ * Create a client socket. Sets keepalive, nonblock and nodelay
+ * options.
+ */
void
evio_socket(struct ev_io *coio, int domain, int type, int protocol);
+/** Close evio service socket and detach from event loop. */
void
evio_close(ev_loop *loop, struct ev_io *evio);
static inline bool
-evio_service_is_active(struct evio_service *service)
+evio_has_fd(struct ev_io *ev)
{
- return service->ev.fd >= 0;
+ return ev->fd >= 0;
}
static inline bool
-evio_has_fd(struct ev_io *ev)
+evio_service_is_active(struct evio_service *service)
{
- return ev->fd >= 0;
+ return evio_has_fd(&service->ev);
}
static inline void
@@ -150,6 +147,7 @@ evio_timeout_update(ev_loop *loop, ev_tstamp start, ev_tstamp *delay)
*delay = (elapsed >= *delay) ? 0 : *delay - elapsed;
}
+/** Set nonblock, keepalive and nodelay options to socket. */
int
evio_setsockopt_client(int fd, int family, int type);
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 09/11] evio: refactoring
2018-11-30 15:39 ` [PATCH 09/11] evio: refactoring Vladislav Shpilevoy
@ 2018-12-03 16:37 ` Vladimir Davydov
2018-12-04 21:29 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 16:37 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:41PM +0300, Vladislav Shpilevoy wrote:
> Remove useless, wrong and obvious comments. Correct
> code and comment style. Reuse some code, unnecessary
> try/catch.
> ---
> src/evio.cc | 186 ++++++++++++++++++++--------------------------------
> src/evio.h | 40 ++++++-----
> 2 files changed, 89 insertions(+), 137 deletions(-)
>
> diff --git a/src/evio.cc b/src/evio.cc
> index 25f699bab..166ba7f95 100644
> --- a/src/evio.cc
> +++ b/src/evio.cc
> @@ -29,7 +29,6 @@
> * SUCH DAMAGE.
> */
> #include "evio.h"
> -#include "uri.h"
> #include "scoped_guard.h"
> #include <stdio.h>
> #include <sys/socket.h>
> @@ -41,41 +40,33 @@
> #include <trivia/util.h>
> #include "exception.h"
>
> -static void
> -evio_setsockopt_server(int fd, int family, int type);
> -
> -/** Note: this function does not throw. */
> void
> evio_close(ev_loop *loop, struct ev_io *evio)
> {
> /* Stop I/O events. Safe to do even if not started. */
> ev_io_stop(loop, evio);
> - /* Close the socket. */
> - close(evio->fd);
> - /* Make sure evio_has_fd() returns a proper value. */
> - evio->fd = -1;
> + if (evio_has_fd(evio)) {
> + close(evio->fd);
> + ev_io_set(evio, -1, 0);
> + }
> }
>
> -/**
> - * Create an endpoint for communication.
> - * Set socket as non-block and apply protocol specific options.
> - */
> void
> -evio_socket(struct ev_io *coio, int domain, int type, int protocol)
> +evio_socket(struct ev_io *evio, int domain, int type, int protocol)
> {
> - assert(coio->fd == -1);
> - coio->fd = sio_socket(domain, type, protocol);
> - if (coio->fd < 0)
> + assert(! evio_has_fd(evio));
> + evio->fd = sio_socket(domain, type, protocol);
> + if (evio->fd < 0)
> diag_raise();
> - if (evio_setsockopt_client(coio->fd, domain, type) != 0) {
> - close(coio->fd);
> - coio->fd = -1;
> + if (evio_setsockopt_client(evio->fd, domain, type) != 0) {
> + close(evio->fd);
> + ev_io_set(evio, -1, 0);
> diag_raise();
> }
> }
>
> static int
> -evio_setsockopt_keepalive(int fd)
> +evio_setsockopt_keeping(int fd)
> {
> int on = 1;
> /*
Refactoring/renaming done by this patch looks dubious to me. It isn't
worth polluting the history. Please drop it.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [tarantool-patches] Re: [PATCH 09/11] evio: refactoring
2018-12-03 16:37 ` Vladimir Davydov
@ 2018-12-04 21:29 ` Vladislav Shpilevoy
0 siblings, 0 replies; 37+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:29 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
On 03/12/2018 19:37, Vladimir Davydov wrote:
> On Fri, Nov 30, 2018 at 06:39:41PM +0300, Vladislav Shpilevoy wrote:
>> Remove useless, wrong and obvious comments. Correct
>> code and comment style. Reuse some code, unnecessary
>> try/catch.
>> ---
>> src/evio.cc | 186 ++++++++++++++++++++--------------------------------
>> src/evio.h | 40 ++++++-----
>> 2 files changed, 89 insertions(+), 137 deletions(-)
>>
>> diff --git a/src/evio.cc b/src/evio.cc
>> index 25f699bab..166ba7f95 100644
>> --- a/src/evio.cc
>> +++ b/src/evio.cc
>> @@ -29,7 +29,6 @@
>> * SUCH DAMAGE.
>> */
>> #include "evio.h"
>> -#include "uri.h"
>> #include "scoped_guard.h"
>> #include <stdio.h>
>> #include <sys/socket.h>
>> @@ -41,41 +40,33 @@
>> #include <trivia/util.h>
>> #include "exception.h"
>>
>> -static void
>> -evio_setsockopt_server(int fd, int family, int type);
>> -
>> -/** Note: this function does not throw. */
>> void
>> evio_close(ev_loop *loop, struct ev_io *evio)
>> {
>> /* Stop I/O events. Safe to do even if not started. */
>> ev_io_stop(loop, evio);
>> - /* Close the socket. */
>> - close(evio->fd);
>> - /* Make sure evio_has_fd() returns a proper value. */
>> - evio->fd = -1;
>> + if (evio_has_fd(evio)) {
>> + close(evio->fd);
>> + ev_io_set(evio, -1, 0);
>> + }
>> }
>>
>> -/**
>> - * Create an endpoint for communication.
>> - * Set socket as non-block and apply protocol specific options.
>> - */
>> void
>> -evio_socket(struct ev_io *coio, int domain, int type, int protocol)
>> +evio_socket(struct ev_io *evio, int domain, int type, int protocol)
>> {
>> - assert(coio->fd == -1);
>> - coio->fd = sio_socket(domain, type, protocol);
>> - if (coio->fd < 0)
>> + assert(! evio_has_fd(evio));
>> + evio->fd = sio_socket(domain, type, protocol);
>> + if (evio->fd < 0)
>> diag_raise();
>> - if (evio_setsockopt_client(coio->fd, domain, type) != 0) {
>> - close(coio->fd);
>> - coio->fd = -1;
>> + if (evio_setsockopt_client(evio->fd, domain, type) != 0) {
>> + close(evio->fd);
>> + ev_io_set(evio, -1, 0);
>> diag_raise();
>> }
>> }
>>
>> static int
>> -evio_setsockopt_keepalive(int fd)
>> +evio_setsockopt_keeping(int fd)
>> {
>> int on = 1;
>> /*
>
> Refactoring/renaming done by this patch looks dubious to me. It isn't
> worth polluting the history. Please drop it.
>
As you wish. See v2.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 00/11] SWIM preparation
2018-11-30 15:39 [PATCH 00/11] SWIM preparation Vladislav Shpilevoy
` (10 preceding siblings ...)
2018-11-30 15:39 ` [PATCH 09/11] evio: refactoring Vladislav Shpilevoy
@ 2018-12-03 9:47 ` Vladimir Davydov
2018-12-03 10:27 ` [tarantool-patches] " Vladislav Shpilevoy
11 siblings, 1 reply; 37+ messages in thread
From: Vladimir Davydov @ 2018-12-03 9:47 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Nov 30, 2018 at 06:39:30PM +0300, Vladislav Shpilevoy wrote:
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-3234-swim-preparation
The branch doesn't exist. Please fix.
^ permalink raw reply [flat|nested] 37+ messages in thread