From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 10 May 2018 22:27:54 +0300 From: Konstantin Osipov Subject: Re: [tarantool-patches] [PATCH v2 08/10] session: introduce text box.session.push Message-ID: <20180510192754.GH30593@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: tarantool-patches@freelists.org Cc: vdavydov.dev@gmail.com List-ID: * Vladislav Shpilevoy [18/04/20 16:31]: > +/** > + * Push a tagged YAML document into a console socket. > + * @param session Console session. > + * @param port Port with YAML to push. > + * > + * @retval 0 Success. > + * @retval -1 Error. > + */ > +static int > +console_session_push(struct session *session, struct port *port) > +{ > + assert(session_vtab_registry[session->type].push == > + console_session_push); > + uint32_t text_len; > + const char *text = port_dump_plain(port, &text_len); > + if (text == NULL) > + return -1; > + int fd = session_fd(session); > + while (text_len > 0) { > + while (coio_wait(fd, COIO_WRITE, > + TIMEOUT_INFINITY) != COIO_WRITE); Nitpick: the socket is ready in 99% of cases, there is no reason to call coio_wait() unless you get EINTR. Second, why the choice of fio_writev() rather than write() or send? What is wrong with coio_writev or similar? > + const struct iovec iov = { > + .iov_base = (void *) text, > + .iov_len = text_len > + }; > + ssize_t rc = fio_writev(fd, &iov, 1); > + if (rc < 0) > + return -1; > + text_len -= rc; > + text += rc; > + } > + return 0; > +} > @@ -92,13 +93,27 @@ local text_connection_mt = { > -- > eval = function(self, text) > text = text..'$EOF$\n' > - if self:write(text) then > + if not self:write(text) then > + error(self:set_error()) > + end > + while true do > local rc = self:read() > - if rc then > + if not rc then > + break > + end > + local handle, prefix = yaml.decode_tag(rc) > + assert(handle or not prefix) > + if not handle then > + -- Can not fail - tags are encoded with no > + -- user participation and are correct always. > + assert(not prefix) In Lua, asserts take CPU time. Please don't use them unless in a test. > return rc > end > + if handle == PUSH_TAG_HANDLE and self.print_f then > + self.print_f(rc) > + end > end > - error(self:set_error()) > + return rc > end, > -- > -- Make the connection be in error state, set error > @@ -121,15 +136,18 @@ local text_connection_mt = { > -- netbox-like object. > -- @param connection Socket to wrap. > -- @param url Parsed destination URL. > +-- @param print_f Function to print push messages. > +-- > -- @retval nil, err Error, and err contains an error message. > -- @retval not nil Netbox-like object. > -- > -local function wrap_text_socket(connection, url) > +local function wrap_text_socket(connection, url, print_f) > local conn = setmetatable({ > _socket = connection, > state = 'active', > host = url.host or 'localhost', > port = url.service, > + print_f = print_f, > }, text_connection_mt) > if not conn:write('require("console").delimiter("$EOF$")\n') or > not conn:read() then > @@ -369,7 +387,8 @@ local function connect(uri, opts) > end > local remote > if greeting.protocol == 'Lua console' then > - remote = wrap_text_socket(connection, u) > + remote = wrap_text_socket(connection, u, > + function(msg) self:print(msg) end) > else > opts = { > connect_timeout = opts.timeout, > diff --git a/src/box/lua/session.c b/src/box/lua/session.c > index 5fe5f08d4..306271809 100644 > --- a/src/box/lua/session.c > +++ b/src/box/lua/session.c > @@ -41,6 +41,8 @@ > #include "box/session.h" > #include "box/user.h" > #include "box/schema.h" > +#include "box/port.h" > +#include "box/lua/console.h" > > static const char *sessionlib_name = "box.session"; > > @@ -355,6 +357,52 @@ lbox_push_on_access_denied_event(struct lua_State *L, void *event) > return 3; > } > > +/** > + * Port to push a message from Lua. > + */ > +struct lua_push_port { > + const struct port_vtab *vtab; > + /** > + * Lua state, containing data to dump on top of the stack. > + */ > + struct lua_State *L; > +}; > + > +static const char * > +lua_push_port_dump_plain(struct port *port, uint32_t *size); > + > +static const struct port_vtab lua_push_port_vtab = { > + .dump_msgpack = NULL, > + /* > + * Dump_16 has no sense, since push appears since 1.10 > + * protocol. > + */ > + .dump_msgpack_16 = NULL, > + .dump_plain = lua_push_port_dump_plain, > + .destroy = NULL, > +}; > + > +static const char * > +lua_push_port_dump_plain(struct port *port, uint32_t *size) > +{ > + struct lua_push_port *lua_port = (struct lua_push_port *) port; > + assert(lua_port->vtab == &lua_push_port_vtab); > + struct lua_State *L = lua_port->L; > + int rc = console_encode_push(L); > + if (rc == 2) { > + assert(lua_isnil(L, -2)); > + assert(lua_isstring(L, -1)); > + diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1)); > + return NULL; > + } > + assert(rc == 1); > + assert(lua_isstring(L, -1)); > + size_t len; > + const char *result = lua_tolstring(L, -1, &len); > + *size = (uint32_t) len; > + return result; > +} > + > /** > * Push a message using a protocol, depending on a session type. > * @param data Data to push, first argument on a stack. > @@ -367,7 +415,11 @@ lbox_session_push(struct lua_State *L) > if (lua_gettop(L) != 1) > return luaL_error(L, "Usage: box.session.push(data)"); > > - if (session_push(current_session(), NULL) != 0) { > + struct lua_push_port port; > + port.vtab = &lua_push_port_vtab; Why do you need a separate port? And why do you need to create a port for every push? Can't you reuse the same port as is used for the Lua call itself? > + port.L = L; > + > + if (session_push(current_session(), (struct port *) &port) != 0) { > lua_pushnil(L); > luaT_pusherror(L, box_error_last()); > return 2; > diff --git a/src/box/port.c b/src/box/port.c > index 255eb732c..f9b655840 100644 -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov