* [PATCH v2 00/11] SWIM preparation
@ 2018-12-04 21:28 Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 01/11] sio: remove unused functions Vladislav Shpilevoy
` (11 more replies)
0 siblings, 12 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
SWIM is going to use evio to bind to an address, specified by a
user. Evio encapsulates bind/rebind, diagnostics, socket family.
But evio is C++ and SWIM is C. The patchset converts evio to C
alongside with sio, which is used in evio.
During conversion several not critical bugs were found and fixed
in separate commits.
Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-3234-swim-preparation
Issue: https://github.com/tarantool/tarantool/issues/3234
V1: https://www.freelists.org/post/tarantool-patches/PATCH-0011-SWIM-preparation
Changes in v2:
- Removed unnecessary refactoring to save git history;
- Changed a way how to say that a sio error is critical;
- Dropped a bugfix about negative size_t.
Vladislav Shpilevoy (11):
sio: remove unused functions
sio: treat EADDRINUSE in sio_listen as error
sio: remove exceptions
sio: make code compatible with C
sio: turn into C
evio: make on_accept be nothrow
coio: fix double close of a file descriptor
evio: remove exceptions
coio: fix file descriptor leak on accept
evio: make code C compatible
evio: turn nto c
src/CMakeLists.txt | 4 +-
src/box/iproto.cc | 46 ++++----
src/coio.cc | 59 ++++++-----
src/{evio.cc => evio.c} | 183 ++++++++++++++++----------------
src/evio.h | 41 +++++---
src/{sio.cc => sio.c} | 226 +++++++---------------------------------
src/sio.h | 66 ++----------
7 files changed, 221 insertions(+), 404 deletions(-)
rename src/{evio.cc => evio.c} (76%)
rename src/{sio.cc => sio.c} (64%)
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 01/11] sio: remove unused functions
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-09 12:10 ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 10/11] evio: make code C compatible Vladislav Shpilevoy
` (10 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Next patches remove exceptions from sio and convert it
to C. So as to do not care about unused functions they
are deleted.
---
src/sio.cc | 166 +----------------------------------------------------
src/sio.h | 54 -----------------
2 files changed, 1 insertion(+), 219 deletions(-)
diff --git a/src/sio.cc b/src/sio.cc
index 0475b9e83..d79ad5c01 100644
--- a/src/sio.cc
+++ b/src/sio.cc
@@ -33,18 +33,9 @@
#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"
@@ -107,22 +98,12 @@ 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.
* 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
@@ -305,151 +286,6 @@ 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,
diff --git a/src/sio.h b/src/sio.h
index f728af547..2843c0c45 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -95,8 +95,6 @@ sio_add_to_iov(struct iovec *iov, size_t size)
const char *sio_socketname(int fd);
int sio_socket(int domain, int type, int protocol);
-int sio_shutdown(int fd, int how);
-
int sio_getfl(int fd);
int sio_setfl(int fd, int flag, int on);
@@ -110,7 +108,6 @@ sio_getsockopt(int fd, int level, int optname,
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);
@@ -118,57 +115,6 @@ 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);
-ssize_t sio_write_total(int fd, const void *buf, size_t count, size_t total);
-
-/**
- * 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.
- */
-ssize_t
-sio_readn_ahead(int fd, void *buf, size_t count, size_t buf_size);
-
-/**
- * Read count bytes from fd.
- * Throw an exception on error or disconnect.
- *
- * @return count of bytes actually read.
- */
-static inline ssize_t
-sio_readn(int fd, void *buf, size_t count)
-{
- return sio_readn_ahead(fd, buf, count, count);
-}
-
-/**
- * Write count bytes to fd.
- * Throw an exception on error or disconnect.
- *
- * @return count of bytes actually written.
- */
-ssize_t
-sio_writen(int fd, const void *buf, size_t count);
-
-/* Only for blocked I/O */
-ssize_t
-sio_writev_all(int fd, struct iovec *iov, int iovcnt);
-
-/**
- * A wrapper over sendfile.
- * Throw if send file failed.
- */
-ssize_t
-sio_sendfile(int sock_fd, int file_fd, off_t *offset, size_t size);
-
-/**
- * Receive a file sent by sendfile
- * Throw if receiving failed
- */
-ssize_t
-sio_recvfile(int sock_fd, int file_fd, off_t *offset, size_t size);
-
-
ssize_t sio_sendto(int fd, const void *buf, size_t len, int flags,
const struct sockaddr *dest_addr, socklen_t addrlen);
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 10/11] evio: make code C compatible
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 01/11] sio: remove unused functions Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-05 8:56 ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 11/11] evio: turn nto c Vladislav Shpilevoy
` (9 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
---
src/evio.cc | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/evio.cc b/src/evio.cc
index a3d1d8a13..8f5022bca 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -167,9 +167,10 @@ evio_service_name(struct evio_service *service)
* 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) {
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 11/11] evio: turn nto c
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 01/11] sio: remove unused functions Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 10/11] evio: make code C compatible Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 02/11] sio: treat EADDRINUSE in sio_listen as error Vladislav Shpilevoy
` (8 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
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 e431d1f57..04de5ad04 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -90,7 +90,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 6c5e54ea4..69d641a60 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 listening socket to the event loop. Callbacks
* are invoked on bind and accept events.
@@ -152,4 +157,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] 23+ messages in thread
* [PATCH v2 02/11] sio: treat EADDRINUSE in sio_listen as error
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (2 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 11/11] evio: turn nto c Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-09 12:57 ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 03/11] sio: remove exceptions Vladislav Shpilevoy
` (7 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Earlier maybe it made sense - not to throw an error
on EADDRINUSE from listen(), but now it just
complicates exceptions removal.
---
src/evio.cc | 5 +----
src/sio.cc | 2 +-
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/evio.cc b/src/evio.cc
index a6ac65daf..9df797c78 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -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 d79ad5c01..aa44b4912 100644
--- a/src/sio.cc
+++ b/src/sio.cc
@@ -213,7 +213,7 @@ 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;
}
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 03/11] sio: remove exceptions
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (3 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 02/11] sio: treat EADDRINUSE in sio_listen as error Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-09 12:54 ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 04/11] sio: make code compatible with C Vladislav Shpilevoy
` (6 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 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 do not
set diagnosticts on a number of specific errors.
To check if an error, returned from a function, is
critical, a new function is introduced:
sio_is_error_fatal(). It takes a code, returned by a
function, and checks if it means a critical error.
Needed for #3234
---
src/box/iproto.cc | 21 ++++++++---------
src/coio.cc | 25 ++++++++++++--------
src/evio.cc | 18 ++++++++++-----
src/sio.cc | 58 ++++++++++++++++++++++++++++++-----------------
src/sio.h | 7 ++++++
5 files changed, 81 insertions(+), 48 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 7c11d05b3..fe42bb250 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -859,6 +859,8 @@ iproto_connection_on_input(ev_loop *loop, struct ev_io *watcher,
/* Read input. */
int nrd = sio_read(fd, in->wpos, ibuf_unused(in));
if (nrd < 0) { /* Socket is not ready. */
+ if (sio_is_error_fatal(nrd))
+ diag_raise();
ev_io_start(loop, &con->input);
return;
}
@@ -923,10 +925,8 @@ iproto_flush(struct iproto_connection *con)
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) {
+ rmean_collect(rmean_net, IPROTO_SENT, nwr);
if (begin->used + nwr == end->used) {
*begin = *end;
return 0;
@@ -938,6 +938,8 @@ iproto_flush(struct iproto_connection *con)
begin->iov_len = advance == 0 ? begin->iov_len + offset: offset;
begin->pos += advance;
assert(begin->pos <= end->pos);
+ } else if (sio_is_error_fatal(nwr)) {
+ diag_raise();
}
return -1;
}
@@ -1760,15 +1762,12 @@ 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 */
+ int64_t nwr = sio_writev(con->output.fd, out->iov,
+ obuf_iovcnt(out));
+ if (nwr > 0)
rmean_collect(rmean_net, IPROTO_SENT, nwr);
- } catch (Exception *e) {
- e->log();
- }
+ else if (sio_is_error_fatal(nwr))
+ diag_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..a546f647d 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -258,6 +258,8 @@ coio_accept(struct ev_io *coio, struct sockaddr *addr,
SOCK_STREAM);
return fd;
}
+ if (sio_is_error_fatal(fd))
+ diag_raise();
/* The socket is not ready, yield */
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_READ);
@@ -313,6 +315,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 (sio_is_error_fatal(nrd)) {
+ diag_raise();
}
/* The socket is not ready, yield */
@@ -404,6 +408,8 @@ coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz,
return sz;
towrite -= nwr;
buf = (char *) buf + nwr;
+ } else if (sio_is_error_fatal(nwr)) {
+ diag_raise();
}
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_WRITE);
@@ -433,15 +439,11 @@ 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);
+ ssize_t nwr = sio_writev(fd, iov, iovcnt);
+ sio_add_to_iov(iov, offset);
+ if (nwr < 0 && sio_is_error_fatal(nwr))
+ diag_raise();
return nwr;
}
@@ -522,6 +524,8 @@ coio_sendto_timeout(struct ev_io *coio, const void *buf, size_t sz, int flags,
flags, dest_addr, addrlen);
if (nwr > 0)
return nwr;
+ if (sio_is_error_fatal(nwr))
+ diag_raise();
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_WRITE);
ev_io_start(loop(), coio);
@@ -565,7 +569,8 @@ coio_recvfrom_timeout(struct ev_io *coio, void *buf, size_t sz, int flags,
src_addr, &addrlen);
if (nrd >= 0)
return nrd;
-
+ if (sio_is_error_fatal(nrd))
+ 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 9df797c78..32aece0f0 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -184,8 +184,11 @@ evio_service_accept_cb(ev_loop * /* loop */, ev_io *watcher,
fd = sio_accept(service->ev.fd,
(struct sockaddr *)&addr, &addrlen);
- if (fd < 0) /* EAGAIN, EWOULDLOCK, EINTR */
+ if (fd < 0) {
+ if (sio_is_error_fatal(fd))
+ diag_raise();
return;
+ }
/* set common client socket options */
evio_setsockopt_client(fd, service->addr.sa_family, SOCK_STREAM);
/*
@@ -259,13 +262,15 @@ evio_service_bind_addr(struct evio_service *service)
evio_setsockopt_server(fd, service->addr.sa_family, SOCK_STREAM);
- if (sio_bind(fd, &service->addr, service->addr_len)) {
- if (errno != EADDRINUSE)
+ int rc = sio_bind(fd, &service->addr, service->addr_len);
+ if (rc != 0) {
+ if (sio_is_error_fatal(rc))
diag_raise();
if (evio_service_reuse_addr(service, fd))
diag_raise();
- if (sio_bind(fd, &service->addr, service->addr_len)) {
- if (errno == EADDRINUSE) {
+ rc = sio_bind(fd, &service->addr, service->addr_len);
+ if (rc != 0) {
+ if (! sio_is_error_fatal(rc)) {
diag_set(SocketError, sio_socketname(fd),
"bind");
}
@@ -294,7 +299,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 aa44b4912..a13e7d9de 100644
--- a/src/sio.cc
+++ b/src/sio.cc
@@ -40,6 +40,14 @@
#include "trivia/util.h"
#include "exception.h"
+enum { SIO_ERROR_FATAL = -2 };
+
+bool
+sio_is_error_fatal(int retcode)
+{
+ return retcode == SIO_ERROR_FATAL;
+}
+
/** Pretty print socket name and peer (for exceptions) */
const char *
sio_socketname(int fd)
@@ -213,8 +221,10 @@ int
sio_listen(int fd)
{
int rc = listen(fd, sio_listen_backlog());
- if (rc < 0)
- tnt_raise(SocketError, sio_socketname(fd), "listen");
+ if (rc < 0) {
+ diag_set(SocketError, sio_socketname(fd), "listen");
+ return SIO_ERROR_FATAL;
+ }
return rc;
}
@@ -224,9 +234,11 @@ 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))
- tnt_raise(SocketError, sio_socketname(fd), "accept");
+ if (newfd < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
+ errno != EINTR) {
+ diag_set(SocketError, sio_socketname(fd), "accept");
+ return SIO_ERROR_FATAL;
+ }
return newfd;
}
@@ -253,8 +265,9 @@ sio_read(int fd, void *buf, size_t count)
n = 0;
break;
default:
- tnt_raise(SocketError, sio_socketname(fd),
- "read(%zd)", count);
+ diag_set(SocketError, sio_socketname(fd), "read(%zd)",
+ count);
+ return SIO_ERROR_FATAL;
}
}
return n;
@@ -265,10 +278,11 @@ 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) {
+ diag_set(SocketError, sio_socketname(fd), "write(%zd)", count);
+ return SIO_ERROR_FATAL;
+ }
return n;
}
@@ -280,8 +294,8 @@ sio_writev(int fd, const struct iovec *iov, int iovcnt)
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);
+ diag_set(SocketError, sio_socketname(fd), "writev(%d)", iovcnt);
+ return SIO_ERROR_FATAL;
}
return n;
}
@@ -293,10 +307,11 @@ sio_sendto(int fd, const void *buf, size_t len, int flags,
{
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) {
+ diag_set(SocketError, sio_socketname(fd), "sendto(%zd)", len);
+ return SIO_ERROR_FATAL;
+ }
return n;
}
@@ -307,10 +322,11 @@ sio_recvfrom(int fd, void *buf, size_t len, int flags,
{
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) {
+ diag_set(SocketError, sio_socketname(fd), "recvfrom(%zd)", len);
+ return SIO_ERROR_FATAL;
+ }
return n;
}
diff --git a/src/sio.h b/src/sio.h
index 2843c0c45..5d12f35f9 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -48,6 +48,13 @@ extern "C" {
enum { SERVICE_NAME_MAXLEN = 32 };
+/**
+ * Check if a code, returned from a sio function, means a
+ * critical error.
+ */
+bool
+sio_is_error_fatal(int retcode);
+
const char *
sio_strfaddr(struct sockaddr *addr, socklen_t addrlen);
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 04/11] sio: make code compatible with C
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (4 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 03/11] sio: remove exceptions Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-05 8:57 ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 05/11] sio: turn into C Vladislav Shpilevoy
` (5 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
---
src/sio.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/sio.cc b/src/sio.cc
index a13e7d9de..d0ae75dbc 100644
--- a/src/sio.cc
+++ b/src/sio.cc
@@ -355,7 +355,7 @@ 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)) {
+ if (addrlen >= sizeof(struct sockaddr_un)) {
snprintf(name, sizeof(name), "unix/:%s",
((struct sockaddr_un *)addr)->sun_path);
} else {
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 05/11] sio: turn into C
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (5 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 04/11] sio: make code compatible with C Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 06/11] evio: make on_accept be nothrow Vladislav Shpilevoy
` (4 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
Needed for #3234
---
src/CMakeLists.txt | 2 +-
src/{sio.cc => sio.c} | 0
src/sio.h | 5 ++---
3 files changed, 3 insertions(+), 4 deletions(-)
rename src/{sio.cc => sio.c} (100%)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 6ce80a55e..e431d1f57 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -89,7 +89,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/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 5d12f35f9..7da2787a8 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -96,9 +96,6 @@ sio_add_to_iov(struct iovec *iov, size_t size)
iov->iov_base = (char *) iov->iov_base - size;
}
-#if defined(__cplusplus)
-} /* extern "C" */
-
const char *sio_socketname(int fd);
int sio_socket(int domain, int type, int protocol);
@@ -128,6 +125,8 @@ ssize_t sio_sendto(int fd, const void *buf, size_t len, int flags,
ssize_t sio_recvfrom(int fd, void *buf, size_t len, int flags,
struct sockaddr *src_addr, socklen_t *addrlen);
+#if defined(__cplusplus)
+} /* extern "C" */
#endif /* defined(__cplusplus) */
#endif /* TARANTOOL_SIO_H_INCLUDED */
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 06/11] evio: make on_accept be nothrow
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (6 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 05/11] sio: turn into C Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 07/11] coio: fix double close of a file descriptor Vladislav Shpilevoy
` (3 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 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 | 20 ++++++++++----------
4 files changed, 29 insertions(+), 34 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index fe42bb250..f4051e791 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1795,7 +1795,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)
{
@@ -1804,26 +1804,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 a546f647d..9b9c71c79 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -589,7 +589,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)
{
@@ -605,14 +605,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
@@ -624,6 +622,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 32aece0f0..1d906db8e 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -195,8 +195,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);
@@ -305,11 +307,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..e56e2b8a7 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. */
@@ -79,12 +84,10 @@ 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 a callback returned != 0, the accepted socket is
+ * closed and the error is logged.
*/
- 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 +97,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] 23+ messages in thread
* [PATCH v2 07/11] coio: fix double close of a file descriptor
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (7 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 06/11] evio: make on_accept be nothrow Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 08/11] evio: remove exceptions Vladislav Shpilevoy
` (2 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 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 9b9c71c79..355c7a42e 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -595,9 +595,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];
@@ -609,9 +606,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] 23+ messages in thread
* [PATCH v2 08/11] evio: remove exceptions
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (8 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 07/11] coio: fix double close of a file descriptor Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 09/11] coio: fix file descriptor leak on accept Vladislav Shpilevoy
2018-12-11 8:47 ` [PATCH v2 00/11] SWIM preparation Vladimir Davydov
11 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 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 | 8 +--
src/coio.cc | 13 ++--
src/evio.cc | 164 +++++++++++++++++++++++-----------------------
src/evio.h | 12 ++--
4 files changed, 99 insertions(+), 98 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index f4051e791..0f64410a4 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -2031,10 +2031,10 @@ iproto_do_cfg_f(struct cbus_call_msg *m)
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);
- }
+ if (cfg_msg->uri != NULL &&
+ (evio_service_bind(&binary, cfg_msg->uri) != 0 ||
+ evio_service_listen(&binary) != 0))
+ diag_raise();
break;
default:
unreachable();
diff --git a/src/coio.cc b/src/coio.cc
index 355c7a42e..82bf72e69 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();
auto coio_guard = make_scoped_guard([=]{ evio_close(loop, coio); });
if (sio_connect(coio->fd, addr, len) == 0) {
coio_guard.is_active = false;
@@ -254,8 +255,9 @@ coio_accept(struct ev_io *coio, struct sockaddr *addr,
* available */
int fd = sio_accept(coio->fd, addr, &addrlen);
if (fd >= 0) {
- evio_setsockopt_client(fd, addr->sa_family,
- SOCK_STREAM);
+ if (evio_setsockopt_client(fd, addr->sa_family,
+ SOCK_STREAM) != 0)
+ diag_raise();
return fd;
}
if (sio_is_error_fatal(fd))
@@ -636,8 +638,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 1d906db8e..a3d1d8a13 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -30,7 +30,6 @@
*/
#include "evio.h"
#include "uri.h"
-#include "scoped_guard.h"
#include <stdio.h>
#include <sys/socket.h>
#include <sys/un.h>
@@ -41,9 +40,6 @@
#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)
@@ -60,18 +56,18 @@ evio_close(ev_loop *loop, struct ev_io *evio)
* Create an endpoint for communication.
* Set socket as non-block and apply protocol specific options.
*/
-void
+int
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);
+ return -1;
+ return evio_setsockopt_client(coio->fd, domain, type);
}
-static void
+static int
evio_setsockopt_keepalive(int fd)
{
int on = 1;
@@ -81,7 +77,7 @@ evio_setsockopt_keepalive(int fd)
*/
if (sio_setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE,
&on, sizeof(on)))
- diag_raise();
+ return -1;
#ifdef __linux__
/*
* On Linux, we are able to fine-tune keepalive
@@ -91,34 +87,36 @@ evio_setsockopt_keepalive(int fd)
int keepcnt = 5;
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &keepcnt,
sizeof(int)))
- diag_raise();
+ return -1;
int keepidle = 30;
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle,
sizeof(int)))
- diag_raise();
+ return -1;
int keepintvl = 60;
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, &keepintvl,
sizeof(int)))
- diag_raise();
+ 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, on))
- 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
@@ -126,22 +124,23 @@ evio_setsockopt_client(int fd, int family, int type)
*/
if (sio_setsockopt(fd, IPPROTO_TCP, TCP_NODELAY,
&on, sizeof(on)))
- diag_raise();
+ return -1;
}
+ return 0;
}
/** Set options for server sockets. */
-static void
+static int
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))
- diag_raise();
+ return -1;
/* Allow reuse local adresses. */
if (sio_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
&on, sizeof(on)))
- diag_raise();
+ return -1;
/* Send all buffered messages on socket before take
* control out from close(2) or shutdown(2). */
@@ -149,9 +148,11 @@ 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);
+ return -1;
+ if (type == SOCK_STREAM && family != AF_UNIX &&
+ evio_setsockopt_keepalive(fd) != 0)
+ return -1;
+ return 0;
}
static inline const char *
@@ -170,42 +171,33 @@ evio_service_accept_cb(ev_loop * /* loop */, ev_io *watcher,
int /* revents */)
{
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
* io_collect_interval.
*/
- int fd = -1;
- try {
- struct sockaddr_storage addr;
- socklen_t addrlen = sizeof(addr);
- fd = sio_accept(service->ev.fd,
- (struct sockaddr *)&addr, &addrlen);
-
- if (fd < 0) {
- if (sio_is_error_fatal(fd))
- diag_raise();
- return;
- }
- /* set common client socket options */
- evio_setsockopt_client(fd, service->addr.sa_family, SOCK_STREAM);
- /*
- * 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);
+ fd = sio_accept(service->ev.fd, (struct sockaddr *)&addr,
+ &addrlen);
+
+ if (fd < 0) {
+ if (sio_is_error_fatal(fd))
+ 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;
}
+ if (fd >= 0)
+ close(fd);
+ diag_log();
}
/*
@@ -249,34 +241,34 @@ err:
*
* Throws an exception if error.
*/
-static void
+static int
evio_service_bind_addr(struct evio_service *service)
{
say_debug("%s: binding to %s...", evio_service_name(service),
sio_strfaddr(&service->addr, service->addr_len));
/* Create a socket. */
- int fd = sio_socket(service->addr.sa_family,
- SOCK_STREAM, IPPROTO_TCP);
+ int rc, fd = sio_socket(service->addr.sa_family, SOCK_STREAM,
+ IPPROTO_TCP);
if (fd < 0)
- diag_raise();
-
- auto fd_guard = make_scoped_guard([=]{ close(fd); });
+ return -1;
- evio_setsockopt_server(fd, service->addr.sa_family, SOCK_STREAM);
+ if (evio_setsockopt_server(fd, service->addr.sa_family,
+ SOCK_STREAM) != 0)
+ goto error;
- int rc = sio_bind(fd, &service->addr, service->addr_len);
+ rc = sio_bind(fd, &service->addr, service->addr_len);
if (rc != 0) {
if (sio_is_error_fatal(rc))
- diag_raise();
+ goto error;
if (evio_service_reuse_addr(service, fd))
- diag_raise();
+ goto error;
rc = sio_bind(fd, &service->addr, service->addr_len);
if (rc != 0) {
if (! sio_is_error_fatal(rc)) {
diag_set(SocketError, sio_socketname(fd),
"bind");
}
- diag_raise();
+ goto error;
}
}
@@ -285,8 +277,10 @@ 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;
}
/**
@@ -294,7 +288,7 @@ evio_service_bind_addr(struct evio_service *service)
*
* @retval 0 for success
*/
-void
+int
evio_service_listen(struct evio_service *service)
{
say_debug("%s: listening on %s...", evio_service_name(service),
@@ -302,8 +296,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
@@ -329,13 +324,15 @@ evio_service_init(ev_loop *loop, struct evio_service *service, const char *name,
/**
* Try to bind.
*/
-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);
+ if (uri_parse(&u, uri) || u.service == NULL) {
+ diag_set(SocketError, sio_socketname(-1),
+ "invalid uri for bind: %s", uri);
+ return -1;
+ }
snprintf(service->serv, sizeof(service->serv), "%.*s",
(int) u.service_len, u.service);
@@ -365,26 +362,27 @@ evio_service_bind(struct evio_service *service, const char *uri)
/* make no difference between empty string and NULL for host */
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");
- auto addrinfo_guard = make_scoped_guard([=]{ freeaddrinfo(res); });
-
+ &hints, &res) != 0 || res == NULL) {
+ diag_set(SocketError, sio_socketname(-1),
+ "can't resolve uri for bind");
+ return -1;
+ }
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",
- evio_service_name(service),
- sio_strfaddr(ai->ai_addr, ai->ai_addrlen),
- e->get_errmsg());
- /* ignore */
+ if (evio_service_bind_addr(service) == 0) {
+ freeaddrinfo(res);
+ return 0;
}
+ say_error("%s: failed to bind on %s: %s",
+ evio_service_name(service),
+ sio_strfaddr(ai->ai_addr, ai->ai_addrlen),
+ diag_last_error(diag_get())->errmsg);
}
- tnt_raise(SocketError, sio_socketname(-1), "%s: failed to bind",
- evio_service_name(service));
+ freeaddrinfo(res);
+ diag_set(SocketError, sio_socketname(-1), "%s: failed to bind",
+ evio_service_name(service));
+ return -1;
}
/** It's safe to stop a service which is not started yet. */
diff --git a/src/evio.h b/src/evio.h
index e56e2b8a7..6c5e54ea4 100644
--- a/src/evio.h
+++ b/src/evio.h
@@ -39,8 +39,8 @@
#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.
+ * A way to add a listening socket to the event loop. Callbacks
+ * are invoked on bind and accept events.
*
* Coroutines/fibers are not used for port listeners
* since listener's job is usually simple and only involves
@@ -101,7 +101,7 @@ 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);
/**
@@ -109,14 +109,14 @@ evio_service_bind(struct evio_service *service, const char *uri);
*
* @retval 0 for success
*/
-void
+int
evio_service_listen(struct evio_service *service);
/** If started, stop event flow and close the acceptor socket. */
void
evio_service_stop(struct evio_service *service);
-void
+int
evio_socket(struct ev_io *coio, int domain, int type, int protocol);
void
@@ -149,7 +149,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] 23+ messages in thread
* [PATCH v2 09/11] coio: fix file descriptor leak on accept
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (9 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 08/11] evio: remove exceptions Vladislav Shpilevoy
@ 2018-12-04 21:28 ` Vladislav Shpilevoy
2018-12-11 8:47 ` [PATCH v2 00/11] SWIM preparation Vladimir Davydov
11 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-04 21:28 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.
The bug existed even before exceptions are removed
from evio.
---
src/coio.cc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/coio.cc b/src/coio.cc
index 82bf72e69..c45e72507 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -256,8 +256,10 @@ coio_accept(struct ev_io *coio, struct sockaddr *addr,
int fd = sio_accept(coio->fd, addr, &addrlen);
if (fd >= 0) {
if (evio_setsockopt_client(fd, addr->sa_family,
- SOCK_STREAM) != 0)
+ SOCK_STREAM) != 0) {
+ close(fd);
diag_raise();
+ }
return fd;
}
if (sio_is_error_fatal(fd))
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 10/11] evio: make code C compatible
2018-12-04 21:28 ` [PATCH v2 10/11] evio: make code C compatible Vladislav Shpilevoy
@ 2018-12-05 8:56 ` Vladimir Davydov
2018-12-05 20:07 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2018-12-05 8:56 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:28:47AM +0300, Vladislav Shpilevoy wrote:
> ---
> src/evio.cc | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/evio.cc b/src/evio.cc
> index a3d1d8a13..8f5022bca 100644
> --- a/src/evio.cc
> +++ b/src/evio.cc
> @@ -167,9 +167,10 @@ evio_service_name(struct evio_service *service)
> * 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) {
This doesn't deserve a separate patch. Squash it into patch 11 please.
No need to resend the patch.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 04/11] sio: make code compatible with C
2018-12-04 21:28 ` [PATCH v2 04/11] sio: make code compatible with C Vladislav Shpilevoy
@ 2018-12-05 8:57 ` Vladimir Davydov
2018-12-05 20:07 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2018-12-05 8:57 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:28:51AM +0300, Vladislav Shpilevoy wrote:
> ---
> src/sio.cc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/sio.cc b/src/sio.cc
> index a13e7d9de..d0ae75dbc 100644
> --- a/src/sio.cc
> +++ b/src/sio.cc
> @@ -355,7 +355,7 @@ 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)) {
> + if (addrlen >= sizeof(struct sockaddr_un)) {
> snprintf(name, sizeof(name), "unix/:%s",
> ((struct sockaddr_un *)addr)->sun_path);
> } else {
This doesn't deserve a separate patch. Squash it into patch 5 please.
No need to resend the patch.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 10/11] evio: make code C compatible
2018-12-05 8:56 ` Vladimir Davydov
@ 2018-12-05 20:07 ` Vladislav Shpilevoy
0 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-05 20:07 UTC (permalink / raw)
To: tarantool-patches, Vladimir Davydov
On 05/12/2018 11:56, Vladimir Davydov wrote:
> On Wed, Dec 05, 2018 at 12:28:47AM +0300, Vladislav Shpilevoy wrote:
>> ---
>> src/evio.cc | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/evio.cc b/src/evio.cc
>> index a3d1d8a13..8f5022bca 100644
>> --- a/src/evio.cc
>> +++ b/src/evio.cc
>> @@ -167,9 +167,10 @@ evio_service_name(struct evio_service *service)
>> * 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) {
>
> This doesn't deserve a separate patch. Squash it into patch 11 please.
> No need to resend the patch.
>
Done on the branch.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 04/11] sio: make code compatible with C
2018-12-05 8:57 ` Vladimir Davydov
@ 2018-12-05 20:07 ` Vladislav Shpilevoy
0 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-05 20:07 UTC (permalink / raw)
To: tarantool-patches, Vladimir Davydov
On 05/12/2018 11:57, Vladimir Davydov wrote:
> On Wed, Dec 05, 2018 at 12:28:51AM +0300, Vladislav Shpilevoy wrote:
>> ---
>> src/sio.cc | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/sio.cc b/src/sio.cc
>> index a13e7d9de..d0ae75dbc 100644
>> --- a/src/sio.cc
>> +++ b/src/sio.cc
>> @@ -355,7 +355,7 @@ 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)) {
>> + if (addrlen >= sizeof(struct sockaddr_un)) {
>> snprintf(name, sizeof(name), "unix/:%s",
>> ((struct sockaddr_un *)addr)->sun_path);
>> } else {
>
> This doesn't deserve a separate patch. Squash it into patch 5 please.
> No need to resend the patch.
>
Done on the branch.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 01/11] sio: remove unused functions
2018-12-04 21:28 ` [PATCH v2 01/11] sio: remove unused functions Vladislav Shpilevoy
@ 2018-12-09 12:10 ` Vladimir Davydov
0 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2018-12-09 12:10 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:28:46AM +0300, Vladislav Shpilevoy wrote:
> Next patches remove exceptions from sio and convert it
> to C. So as to do not care about unused functions they
> are deleted.
> ---
> src/sio.cc | 166 +----------------------------------------------------
> src/sio.h | 54 -----------------
> 2 files changed, 1 insertion(+), 219 deletions(-)
>
> diff --git a/src/sio.cc b/src/sio.cc
> index 0475b9e83..d79ad5c01 100644
> --- a/src/sio.cc
> +++ b/src/sio.cc
> @@ -33,18 +33,9 @@
> #include <sys/un.h>
> #include <sys/uio.h>
> #include <errno.h>
> -#include <stdio.h>
We still need stdio.h because of snprintf. I put it back and pushed
the patch to 2.1.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 03/11] sio: remove exceptions
2018-12-04 21:28 ` [PATCH v2 03/11] sio: remove exceptions Vladislav Shpilevoy
@ 2018-12-09 12:54 ` Vladimir Davydov
2018-12-10 15:37 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2018-12-09 12:54 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:28:50AM +0300, Vladislav Shpilevoy wrote:
> @@ -40,6 +40,14 @@
> #include "trivia/util.h"
> #include "exception.h"
>
> +enum { SIO_ERROR_FATAL = -2 };
> +
> +bool
> +sio_is_error_fatal(int retcode)
> +{
> + return retcode == SIO_ERROR_FATAL;
> +}
> +
> /** Pretty print socket name and peer (for exceptions) */
> const char *
> sio_socketname(int fd)
> @@ -224,9 +234,11 @@ 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))
> - tnt_raise(SocketError, sio_socketname(fd), "accept");
> + if (newfd < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
> + errno != EINTR) {
> + diag_set(SocketError, sio_socketname(fd), "accept");
> + return SIO_ERROR_FATAL;
> + }
> return newfd;
> }
This looks bad. It's not just my opinion. I consulted with Kostja
verbally and he agrees with me.
Let's please set diag on any error in all sio functions and let the
caller decide which ones are critical and which are not by checking
errno. To simplify the caller's job we can introduce a helper function
checking errno for cetain types of errors, say
sio_wouldblock(errno): errno in EINTR, EAGAIN, EWOULDBLOCK
I've already suggested you to do this. Quoting your reply here:
> 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.
Yeah, I agree, we should only use sio_wouldblock for EAGAIN, EINTR, and
EWOULDBLOCK. Other error codes should be checked explicitly.
>
> Also, notion of 'critical' is different for various functions. I've
> grouped errors in 4 groups:
>
> EINPROGRESS - sio_connect
Check (errno != EINPROGRESS) after calling sio_connect().
>
> EADDRINUSE - sio_bind, sio_listen
errno != EADDRINUSE
>
> EAGAIN - sio_accept, sio_write,
> EWOULDBLOCK sio_writev, sio_readn_ahead,
> EINTR sio_sendto, sio_recvfrom
sio_wouldblock(errno)
>
> ECONNRESET - sio_read
> EAGAIN
> EWOULDBLOCK
> EINTR
Actually, sio_read() never returns ECONNRESET - it treats it as EOF
(i.e. returns 0) and doesn't raise an error. So for sio_read(), the
caller can use sio_wouldblock() as well.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 02/11] sio: treat EADDRINUSE in sio_listen as error
2018-12-04 21:28 ` [PATCH v2 02/11] sio: treat EADDRINUSE in sio_listen as error Vladislav Shpilevoy
@ 2018-12-09 12:57 ` Vladimir Davydov
2018-12-10 15:36 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2018-12-09 12:57 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:28:49AM +0300, Vladislav Shpilevoy wrote:
> Earlier maybe it made sense - not to throw an error
> on EADDRINUSE from listen(), but now it just
> complicates exceptions removal.
> ---
> src/evio.cc | 5 +----
> src/sio.cc | 2 +-
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/src/evio.cc b/src/evio.cc
> index a6ac65daf..9df797c78 100644
> --- a/src/evio.cc
> +++ b/src/evio.cc
> @@ -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 d79ad5c01..aa44b4912 100644
> --- a/src/sio.cc
> +++ b/src/sio.cc
> @@ -213,7 +213,7 @@ 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;
> }
Please squash this into the patch removing sio exceptions.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 02/11] sio: treat EADDRINUSE in sio_listen as error
2018-12-09 12:57 ` Vladimir Davydov
@ 2018-12-10 15:36 ` Vladislav Shpilevoy
0 siblings, 0 replies; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-10 15:36 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
On 09/12/2018 15:57, Vladimir Davydov wrote:
> On Wed, Dec 05, 2018 at 12:28:49AM +0300, Vladislav Shpilevoy wrote:
>> Earlier maybe it made sense - not to throw an error
>> on EADDRINUSE from listen(), but now it just
>> complicates exceptions removal.
>> ---
>> src/evio.cc | 5 +----
>> src/sio.cc | 2 +-
>> 2 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/evio.cc b/src/evio.cc
>> index a6ac65daf..9df797c78 100644
>> --- a/src/evio.cc
>> +++ b/src/evio.cc
>> @@ -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 d79ad5c01..aa44b4912 100644
>> --- a/src/sio.cc
>> +++ b/src/sio.cc
>> @@ -213,7 +213,7 @@ 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;
>> }
>
> Please squash this into the patch removing sio exceptions.
>
Done.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 03/11] sio: remove exceptions
2018-12-09 12:54 ` Vladimir Davydov
@ 2018-12-10 15:37 ` Vladislav Shpilevoy
2018-12-11 8:44 ` Vladimir Davydov
0 siblings, 1 reply; 23+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-10 15:37 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
On 09/12/2018 15:54, Vladimir Davydov wrote:
> On Wed, Dec 05, 2018 at 12:28:50AM +0300, Vladislav Shpilevoy wrote:
>> @@ -40,6 +40,14 @@
>> #include "trivia/util.h"
>> #include "exception.h"
>>
>> +enum { SIO_ERROR_FATAL = -2 };
>> +
>> +bool
>> +sio_is_error_fatal(int retcode)
>> +{
>> + return retcode == SIO_ERROR_FATAL;
>> +}
>> +
>> /** Pretty print socket name and peer (for exceptions) */
>> const char *
>> sio_socketname(int fd)
>> @@ -224,9 +234,11 @@ 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))
>> - tnt_raise(SocketError, sio_socketname(fd), "accept");
>> + if (newfd < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
>> + errno != EINTR) {
>> + diag_set(SocketError, sio_socketname(fd), "accept");
>> + return SIO_ERROR_FATAL;
>> + }
>> return newfd;
>> }
>
> This looks bad. It's not just my opinion. I consulted with Kostja
> verbally and he agrees with me.
>
> Let's please set diag on any error in all sio functions and let the
> caller decide which ones are critical and which are not by checking
There is a problem - each diag increments error counter visible in
stat. But I believe you know it, so as you wish.
> errno. To simplify the caller's job we can introduce a helper function
> checking errno for cetain types of errors, say
>
> sio_wouldblock(errno): errno in EINTR, EAGAIN, EWOULDBLOCK
>
> I've already suggested you to do this. Quoting your reply here:
>
>> 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.
>
> Yeah, I agree, we should only use sio_wouldblock for EAGAIN, EINTR, and
> EWOULDBLOCK. Other error codes should be checked explicitly.
I do not like that 1) sio user should rely on the fact that errno is not
reset already by another function, 2) sio user sometimes checks errno
explicitly, and sometimes via calling sio_wouldblock, 3) sio_wouldblock
is applicable not only to sio. But I am sure both you and especially
Kostja know it, so as you wish.
>
>>
>> Also, notion of 'critical' is different for various functions. I've
>> grouped errors in 4 groups:
>>
>> EINPROGRESS - sio_connect
>
> Check (errno != EINPROGRESS) after calling sio_connect().
>
>>
>> EADDRINUSE - sio_bind, sio_listen
>
> errno != EADDRINUSE
>
>>
>> EAGAIN - sio_accept, sio_write,
>> EWOULDBLOCK sio_writev, sio_readn_ahead,
>> EINTR sio_sendto, sio_recvfrom
>
> sio_wouldblock(errno)
>
>>
>> ECONNRESET - sio_read
>> EAGAIN
>> EWOULDBLOCK
>> EINTR
>
> Actually, sio_read() never returns ECONNRESET - it treats it as EOF
> (i.e. returns 0) and doesn't raise an error. So for sio_read(), the
> caller can use sio_wouldblock() as well.
>
If you say so.
Diff is right below. The whole patch follows the diff.
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index fe42bb250..108bb8665 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -859,7 +859,7 @@ iproto_connection_on_input(ev_loop *loop, struct ev_io *watcher,
/* Read input. */
int nrd = sio_read(fd, in->wpos, ibuf_unused(in));
if (nrd < 0) { /* Socket is not ready. */
- if (sio_is_error_fatal(nrd))
+ if (! sio_wouldblock(errno))
diag_raise();
ev_io_start(loop, &con->input);
return;
@@ -938,7 +938,7 @@ iproto_flush(struct iproto_connection *con)
begin->iov_len = advance == 0 ? begin->iov_len + offset: offset;
begin->pos += advance;
assert(begin->pos <= end->pos);
- } else if (sio_is_error_fatal(nwr)) {
+ } else if (! sio_wouldblock(errno)) {
diag_raise();
}
return -1;
@@ -1766,7 +1766,7 @@ net_send_greeting(struct cmsg *m)
obuf_iovcnt(out));
if (nwr > 0)
rmean_collect(rmean_net, IPROTO_SENT, nwr);
- else if (sio_is_error_fatal(nwr))
+ else if (! sio_wouldblock(errno))
diag_log();
assert(iproto_connection_is_idle(con));
iproto_connection_close(con);
diff --git a/src/coio.cc b/src/coio.cc
index a546f647d..18baa5444 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -35,6 +35,7 @@
#include <netinet/tcp.h>
#include <stdio.h>
#include <arpa/inet.h>
+#include <errno.h>
#include "sio.h"
#include "scoped_guard.h"
@@ -258,7 +259,7 @@ coio_accept(struct ev_io *coio, struct sockaddr *addr,
SOCK_STREAM);
return fd;
}
- if (sio_is_error_fatal(fd))
+ if (! sio_wouldblock(errno))
diag_raise();
/* The socket is not ready, yield */
if (! ev_is_active(coio)) {
@@ -315,7 +316,7 @@ 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 (sio_is_error_fatal(nrd)) {
+ } else if (! sio_wouldblock(errno)) {
diag_raise();
}
@@ -408,7 +409,7 @@ coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz,
return sz;
towrite -= nwr;
buf = (char *) buf + nwr;
- } else if (sio_is_error_fatal(nwr)) {
+ } else if (! sio_wouldblock(errno)) {
diag_raise();
}
if (! ev_is_active(coio)) {
@@ -442,7 +443,7 @@ coio_flush(int fd, struct iovec *iov, ssize_t offset, int iovcnt)
sio_add_to_iov(iov, -offset);
ssize_t nwr = sio_writev(fd, iov, iovcnt);
sio_add_to_iov(iov, offset);
- if (nwr < 0 && sio_is_error_fatal(nwr))
+ if (nwr < 0 && ! sio_wouldblock(errno))
diag_raise();
return nwr;
}
@@ -524,7 +525,7 @@ coio_sendto_timeout(struct ev_io *coio, const void *buf, size_t sz, int flags,
flags, dest_addr, addrlen);
if (nwr > 0)
return nwr;
- if (sio_is_error_fatal(nwr))
+ if (! sio_wouldblock(errno))
diag_raise();
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_WRITE);
@@ -569,7 +570,7 @@ coio_recvfrom_timeout(struct ev_io *coio, void *buf, size_t sz, int flags,
src_addr, &addrlen);
if (nrd >= 0)
return nrd;
- if (sio_is_error_fatal(nrd))
+ if (! sio_wouldblock(errno))
diag_raise();
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_READ);
diff --git a/src/evio.cc b/src/evio.cc
index 32aece0f0..55968f351 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -185,7 +185,7 @@ evio_service_accept_cb(ev_loop * /* loop */, ev_io *watcher,
(struct sockaddr *)&addr, &addrlen);
if (fd < 0) {
- if (sio_is_error_fatal(fd))
+ if (! sio_wouldblock(errno))
diag_raise();
return;
}
@@ -262,20 +262,13 @@ evio_service_bind_addr(struct evio_service *service)
evio_setsockopt_server(fd, service->addr.sa_family, SOCK_STREAM);
- int rc = sio_bind(fd, &service->addr, service->addr_len);
- if (rc != 0) {
- if (sio_is_error_fatal(rc))
+ if (sio_bind(fd, &service->addr, service->addr_len) != 0) {
+ if (errno != EADDRINUSE)
diag_raise();
if (evio_service_reuse_addr(service, fd))
diag_raise();
- rc = sio_bind(fd, &service->addr, service->addr_len);
- if (rc != 0) {
- if (! sio_is_error_fatal(rc)) {
- diag_set(SocketError, sio_socketname(fd),
- "bind");
- }
+ if (sio_bind(fd, &service->addr, service->addr_len) != 0)
diag_raise();
- }
}
say_info("%s: bound to %s", evio_service_name(service),
diff --git a/src/sio.cc b/src/sio.cc
index 6ca2f4802..125464192 100644
--- a/src/sio.cc
+++ b/src/sio.cc
@@ -41,12 +41,10 @@
#include "trivia/util.h"
#include "exception.h"
-enum { SIO_ERROR_FATAL = -2 };
-
bool
-sio_is_error_fatal(int retcode)
+sio_wouldblock(int err)
{
- return retcode == SIO_ERROR_FATAL;
+ return err == EAGAIN || err == EWOULDBLOCK || err == EINTR;
}
/** Pretty print socket name and peer (for exceptions) */
@@ -200,7 +198,7 @@ sio_connect(int fd, struct sockaddr *addr, socklen_t addrlen)
{
/* Establish the connection. */
int rc = connect(fd, (struct sockaddr *) addr, addrlen);
- if (rc < 0 && errno != EINPROGRESS) {
+ if (rc < 0) {
diag_set(SocketError, sio_socketname(fd), "connect to %s",
sio_strfaddr((struct sockaddr *)addr, addrlen));
}
@@ -212,7 +210,7 @@ int
sio_bind(int fd, struct sockaddr *addr, socklen_t addrlen)
{
int rc = bind(fd, addr, addrlen);
- if (rc < 0 && errno != EADDRINUSE)
+ if (rc < 0)
diag_set(SocketError, sio_socketname(fd), "bind");
return rc;
}
@@ -222,10 +220,8 @@ int
sio_listen(int fd)
{
int rc = listen(fd, sio_listen_backlog());
- if (rc < 0) {
+ if (rc < 0)
diag_set(SocketError, sio_socketname(fd), "listen");
- return SIO_ERROR_FATAL;
- }
return rc;
}
@@ -235,11 +231,8 @@ 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)
diag_set(SocketError, sio_socketname(fd), "accept");
- return SIO_ERROR_FATAL;
- }
return newfd;
}
@@ -249,26 +242,18 @@ 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:
+ if (errno == ECONNRESET) {
errno = 0;
n = 0;
- break;
- default:
+ } else {
diag_set(SocketError, sio_socketname(fd), "read(%zd)",
count);
- return SIO_ERROR_FATAL;
}
}
return n;
@@ -279,11 +264,8 @@ 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) {
+ if (n < 0)
diag_set(SocketError, sio_socketname(fd), "write(%zd)", count);
- return SIO_ERROR_FATAL;
- }
return n;
}
@@ -293,11 +275,8 @@ sio_writev(int fd, const struct iovec *iov, int iovcnt)
{
int cnt = iovcnt < IOV_MAX ? iovcnt : IOV_MAX;
ssize_t n = writev(fd, iov, cnt);
- if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
- errno != EINTR) {
+ if (n < 0)
diag_set(SocketError, sio_socketname(fd), "writev(%d)", iovcnt);
- return SIO_ERROR_FATAL;
- }
return n;
}
@@ -308,11 +287,8 @@ sio_sendto(int fd, const void *buf, size_t len, int flags,
{
ssize_t n = sendto(fd, buf, len, flags, (struct sockaddr*)dest_addr,
addrlen);
- if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
- errno != EINTR) {
+ if (n < 0)
diag_set(SocketError, sio_socketname(fd), "sendto(%zd)", len);
- return SIO_ERROR_FATAL;
- }
return n;
}
@@ -323,11 +299,8 @@ sio_recvfrom(int fd, void *buf, size_t len, int flags,
{
ssize_t n = recvfrom(fd, buf, len, flags, (struct sockaddr*)src_addr,
addrlen);
- if (n < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
- errno != EINTR) {
+ if (n < 0)
diag_set(SocketError, sio_socketname(fd), "recvfrom(%zd)", len);
- return SIO_ERROR_FATAL;
- }
return n;
}
diff --git a/src/sio.h b/src/sio.h
index 5d12f35f9..864fa16d9 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -49,11 +49,11 @@ extern "C" {
enum { SERVICE_NAME_MAXLEN = 32 };
/**
- * Check if a code, returned from a sio function, means a
- * critical error.
+ * Check if an errno, returned from a sio function, means a
+ * non-critical error: EAGAIN, EWOULDBLOCK, EINTR.
*/
bool
-sio_is_error_fatal(int retcode);
+sio_wouldblock(int err);
const char *
sio_strfaddr(struct sockaddr *addr, socklen_t addrlen);
========================================================================
Whole patch:
========================================================================
commit 6c901a731d5c7d63c31a36858e32052c7e94c57f
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Tue Dec 4 20:08:54 2018 +0300
sio: remove exceptions
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 can be
retried or called with another arguments. To check if
an error was not critical, a user's code can look at
errno or check errno via sio_wouldblock() which checks
EINTR, EWOULDBLOCK, EAGAIN.
Needed for #3234
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 7c11d05b3..108bb8665 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -859,6 +859,8 @@ iproto_connection_on_input(ev_loop *loop, struct ev_io *watcher,
/* Read input. */
int nrd = sio_read(fd, in->wpos, ibuf_unused(in));
if (nrd < 0) { /* Socket is not ready. */
+ if (! sio_wouldblock(errno))
+ diag_raise();
ev_io_start(loop, &con->input);
return;
}
@@ -923,10 +925,8 @@ iproto_flush(struct iproto_connection *con)
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) {
+ rmean_collect(rmean_net, IPROTO_SENT, nwr);
if (begin->used + nwr == end->used) {
*begin = *end;
return 0;
@@ -938,6 +938,8 @@ iproto_flush(struct iproto_connection *con)
begin->iov_len = advance == 0 ? begin->iov_len + offset: offset;
begin->pos += advance;
assert(begin->pos <= end->pos);
+ } else if (! sio_wouldblock(errno)) {
+ diag_raise();
}
return -1;
}
@@ -1760,15 +1762,12 @@ 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 */
+ int64_t nwr = sio_writev(con->output.fd, out->iov,
+ obuf_iovcnt(out));
+ if (nwr > 0)
rmean_collect(rmean_net, IPROTO_SENT, nwr);
- } catch (Exception *e) {
- e->log();
- }
+ else if (! sio_wouldblock(errno))
+ diag_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..18baa5444 100644
--- a/src/coio.cc
+++ b/src/coio.cc
@@ -35,6 +35,7 @@
#include <netinet/tcp.h>
#include <stdio.h>
#include <arpa/inet.h>
+#include <errno.h>
#include "sio.h"
#include "scoped_guard.h"
@@ -258,6 +259,8 @@ coio_accept(struct ev_io *coio, struct sockaddr *addr,
SOCK_STREAM);
return fd;
}
+ if (! sio_wouldblock(errno))
+ diag_raise();
/* The socket is not ready, yield */
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_READ);
@@ -313,6 +316,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 (! sio_wouldblock(errno)) {
+ diag_raise();
}
/* The socket is not ready, yield */
@@ -404,6 +409,8 @@ coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz,
return sz;
towrite -= nwr;
buf = (char *) buf + nwr;
+ } else if (! sio_wouldblock(errno)) {
+ diag_raise();
}
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_WRITE);
@@ -433,15 +440,11 @@ 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);
+ ssize_t nwr = sio_writev(fd, iov, iovcnt);
+ sio_add_to_iov(iov, offset);
+ if (nwr < 0 && ! sio_wouldblock(errno))
+ diag_raise();
return nwr;
}
@@ -522,6 +525,8 @@ coio_sendto_timeout(struct ev_io *coio, const void *buf, size_t sz, int flags,
flags, dest_addr, addrlen);
if (nwr > 0)
return nwr;
+ if (! sio_wouldblock(errno))
+ diag_raise();
if (! ev_is_active(coio)) {
ev_io_set(coio, coio->fd, EV_WRITE);
ev_io_start(loop(), coio);
@@ -565,7 +570,8 @@ coio_recvfrom_timeout(struct ev_io *coio, void *buf, size_t sz, int flags,
src_addr, &addrlen);
if (nrd >= 0)
return nrd;
-
+ if (! sio_wouldblock(errno))
+ 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 a6ac65daf..55968f351 100644
--- a/src/evio.cc
+++ b/src/evio.cc
@@ -184,8 +184,11 @@ evio_service_accept_cb(ev_loop * /* loop */, ev_io *watcher,
fd = sio_accept(service->ev.fd,
(struct sockaddr *)&addr, &addrlen);
- if (fd < 0) /* EAGAIN, EWOULDLOCK, EINTR */
+ if (fd < 0) {
+ if (! sio_wouldblock(errno))
+ diag_raise();
return;
+ }
/* set common client socket options */
evio_setsockopt_client(fd, service->addr.sa_family, SOCK_STREAM);
/*
@@ -259,18 +262,13 @@ evio_service_bind_addr(struct evio_service *service)
evio_setsockopt_server(fd, service->addr.sa_family, SOCK_STREAM);
- if (sio_bind(fd, &service->addr, service->addr_len)) {
+ if (sio_bind(fd, &service->addr, service->addr_len) != 0) {
if (errno != EADDRINUSE)
diag_raise();
if (evio_service_reuse_addr(service, fd))
diag_raise();
- if (sio_bind(fd, &service->addr, service->addr_len)) {
- if (errno == EADDRINUSE) {
- diag_set(SocketError, sio_socketname(fd),
- "bind");
- }
+ if (sio_bind(fd, &service->addr, service->addr_len) != 0)
diag_raise();
- }
}
say_info("%s: bound to %s", evio_service_name(service),
@@ -294,10 +292,8 @@ 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");
- }
+ 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 f2e103460..125464192 100644
--- a/src/sio.cc
+++ b/src/sio.cc
@@ -41,6 +41,12 @@
#include "trivia/util.h"
#include "exception.h"
+bool
+sio_wouldblock(int err)
+{
+ return err == EAGAIN || err == EWOULDBLOCK || err == EINTR;
+}
+
/** Pretty print socket name and peer (for exceptions) */
const char *
sio_socketname(int fd)
@@ -192,7 +198,7 @@ sio_connect(int fd, struct sockaddr *addr, socklen_t addrlen)
{
/* Establish the connection. */
int rc = connect(fd, (struct sockaddr *) addr, addrlen);
- if (rc < 0 && errno != EINPROGRESS) {
+ if (rc < 0) {
diag_set(SocketError, sio_socketname(fd), "connect to %s",
sio_strfaddr((struct sockaddr *)addr, addrlen));
}
@@ -204,7 +210,7 @@ int
sio_bind(int fd, struct sockaddr *addr, socklen_t addrlen)
{
int rc = bind(fd, addr, addrlen);
- if (rc < 0 && errno != EADDRINUSE)
+ if (rc < 0)
diag_set(SocketError, sio_socketname(fd), "bind");
return rc;
}
@@ -214,8 +220,8 @@ int
sio_listen(int fd)
{
int rc = listen(fd, sio_listen_backlog());
- if (rc < 0 && errno != EADDRINUSE)
- tnt_raise(SocketError, sio_socketname(fd), "listen");
+ if (rc < 0)
+ diag_set(SocketError, sio_socketname(fd), "listen");
return rc;
}
@@ -225,9 +231,8 @@ 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))
- tnt_raise(SocketError, sio_socketname(fd), "accept");
+ if (newfd < 0)
+ diag_set(SocketError, sio_socketname(fd), "accept");
return newfd;
}
@@ -237,25 +242,18 @@ 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:
+ if (errno == ECONNRESET) {
errno = 0;
n = 0;
- break;
- default:
- tnt_raise(SocketError, sio_socketname(fd),
- "read(%zd)", count);
+ } else {
+ diag_set(SocketError, sio_socketname(fd), "read(%zd)",
+ count);
}
}
return n;
@@ -266,10 +264,8 @@ 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)
+ diag_set(SocketError, sio_socketname(fd), "write(%zd)", count);
return n;
}
@@ -279,11 +275,8 @@ sio_writev(int fd, const struct iovec *iov, int iovcnt)
{
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)
+ diag_set(SocketError, sio_socketname(fd), "writev(%d)", iovcnt);
return n;
}
@@ -294,10 +287,8 @@ sio_sendto(int fd, const void *buf, size_t len, int flags,
{
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)
+ diag_set(SocketError, sio_socketname(fd), "sendto(%zd)", len);
return n;
}
@@ -308,10 +299,8 @@ sio_recvfrom(int fd, void *buf, size_t len, int flags,
{
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)
+ diag_set(SocketError, sio_socketname(fd), "recvfrom(%zd)", len);
return n;
}
diff --git a/src/sio.h b/src/sio.h
index 2843c0c45..864fa16d9 100644
--- a/src/sio.h
+++ b/src/sio.h
@@ -48,6 +48,13 @@ extern "C" {
enum { SERVICE_NAME_MAXLEN = 32 };
+/**
+ * Check if an errno, returned from a sio function, means a
+ * non-critical error: EAGAIN, EWOULDBLOCK, EINTR.
+ */
+bool
+sio_wouldblock(int err);
+
const char *
sio_strfaddr(struct sockaddr *addr, socklen_t addrlen);
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 03/11] sio: remove exceptions
2018-12-10 15:37 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-11 8:44 ` Vladimir Davydov
0 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2018-12-11 8:44 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Mon, Dec 10, 2018 at 06:37:37PM +0300, Vladislav Shpilevoy wrote:
> On 09/12/2018 15:54, Vladimir Davydov wrote:
> > On Wed, Dec 05, 2018 at 12:28:50AM +0300, Vladislav Shpilevoy wrote:
> > > @@ -40,6 +40,14 @@
> > > #include "trivia/util.h"
> > > #include "exception.h"
> > > +enum { SIO_ERROR_FATAL = -2 };
> > > +
> > > +bool
> > > +sio_is_error_fatal(int retcode)
> > > +{
> > > + return retcode == SIO_ERROR_FATAL;
> > > +}
> > > +
> > > /** Pretty print socket name and peer (for exceptions) */
> > > const char *
> > > sio_socketname(int fd)
> > > @@ -224,9 +234,11 @@ 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))
> > > - tnt_raise(SocketError, sio_socketname(fd), "accept");
> > > + if (newfd < 0 && errno != EAGAIN && errno != EWOULDBLOCK &&
> > > + errno != EINTR) {
> > > + diag_set(SocketError, sio_socketname(fd), "accept");
> > > + return SIO_ERROR_FATAL;
> > > + }
> > > return newfd;
> > > }
> >
> > This looks bad. It's not just my opinion. I consulted with Kostja
> > verbally and he agrees with me.
> >
> > Let's please set diag on any error in all sio functions and let the
> > caller decide which ones are critical and which are not by checking
>
> There is a problem - each diag increments error counter visible in
> stat. But I believe you know it, so as you wish.
Well, only ClientErrors are accounted in box.stat(), SocketErrors are
not. However, you're right - setting diag on each EWOULDBLOCK error does
seem to be expensive, because diag_set() implies malloc(). This would be
a pointless overhead.
So we've agreed that we shouldn't set diag on transient errors,
only errno. Kostja reworked and pushed this patch by himself.
For some reason, he also authored it, split it in 5, and did some
minor unnecessary changes (like moving comments).
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 00/11] SWIM preparation
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
` (10 preceding siblings ...)
2018-12-04 21:28 ` [PATCH v2 09/11] coio: fix file descriptor leak on accept Vladislav Shpilevoy
@ 2018-12-11 8:47 ` Vladimir Davydov
11 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2018-12-11 8:47 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Wed, Dec 05, 2018 at 12:28:45AM +0300, Vladislav Shpilevoy wrote:
> SWIM is going to use evio to bind to an address, specified by a
> user. Evio encapsulates bind/rebind, diagnostics, socket family.
>
> But evio is C++ and SWIM is C. The patchset converts evio to C
> alongside with sio, which is used in evio.
>
> During conversion several not critical bugs were found and fixed
> in separate commits.
>
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-3234-swim-preparation
> Issue: https://github.com/tarantool/tarantool/issues/3234
>
> V1: https://www.freelists.org/post/tarantool-patches/PATCH-0011-SWIM-preparation
>
> Changes in v2:
> - Removed unnecessary refactoring to save git history;
> - Changed a way how to say that a sio error is critical;
> - Dropped a bugfix about negative size_t.
>
> Vladislav Shpilevoy (11):
> sio: remove unused functions
> sio: treat EADDRINUSE in sio_listen as error
> sio: remove exceptions
> sio: make code compatible with C
> sio: turn into C
> evio: make on_accept be nothrow
> coio: fix double close of a file descriptor
> evio: remove exceptions
> coio: fix file descriptor leak on accept
> evio: make code C compatible
> evio: turn nto c
I pushed all the patches left to 2.1.
Thanks.
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-12-11 8:47 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-04 21:28 [PATCH v2 00/11] SWIM preparation Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 01/11] sio: remove unused functions Vladislav Shpilevoy
2018-12-09 12:10 ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 10/11] evio: make code C compatible Vladislav Shpilevoy
2018-12-05 8:56 ` Vladimir Davydov
2018-12-05 20:07 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 11/11] evio: turn nto c Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 02/11] sio: treat EADDRINUSE in sio_listen as error Vladislav Shpilevoy
2018-12-09 12:57 ` Vladimir Davydov
2018-12-10 15:36 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 03/11] sio: remove exceptions Vladislav Shpilevoy
2018-12-09 12:54 ` Vladimir Davydov
2018-12-10 15:37 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-11 8:44 ` Vladimir Davydov
2018-12-04 21:28 ` [PATCH v2 04/11] sio: make code compatible with C Vladislav Shpilevoy
2018-12-05 8:57 ` Vladimir Davydov
2018-12-05 20:07 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 05/11] sio: turn into C Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 06/11] evio: make on_accept be nothrow Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 07/11] coio: fix double close of a file descriptor Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 08/11] evio: remove exceptions Vladislav Shpilevoy
2018-12-04 21:28 ` [PATCH v2 09/11] coio: fix file descriptor leak on accept Vladislav Shpilevoy
2018-12-11 8:47 ` [PATCH v2 00/11] SWIM preparation Vladimir Davydov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox