From ba2f615cd40d5d809d1a141c7b098e3bd22ff7bb Mon Sep 17 00:00:00 2001 From: ZyX Date: Sat, 16 Jul 2016 02:26:04 +0300 Subject: functests: Test for error conditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During testing found the following bugs: 1. msgpack-gen.lua script is completely unprepared for Float values either in return type or in arguments. Specifically: 1. At the time of writing relevant code FLOAT_OBJ did not exist as well as FLOATING_OBJ, but it would be used by msgpack-gen.lua should return type be Float. I added FLOATING_OBJ macros later because did not know that msgpack-gen.lua uses these _OBJ macros, otherwise it would be FLOAT_OBJ. 2. msgpack-gen.lua should use .data.floating in place of .data.float. But it did not expect that .data subattribute may have name different from lowercased type name. 2. vim_replace_termcodes returned its argument as-is if it receives an empty string (as well as _vim_id*() functions did). But if something in returned argument lives in an allocated memory such action will cause double free: once when freeing arguments, then when freeing return value. It did not cause problems yet because msgpack bindings return empty string as {NULL, 0} and nothing was actually allocated. 3. New code in msgpack-gen.lua popped arguments in reversed order, making lua bindings’ signatures be different from API ones. --- src/nvim/msgpack_rpc/helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/helpers.c b/src/nvim/msgpack_rpc/helpers.c index 5137b375f0..64a018f5c3 100644 --- a/src/nvim/msgpack_rpc/helpers.c +++ b/src/nvim/msgpack_rpc/helpers.c @@ -117,7 +117,7 @@ bool msgpack_rpc_to_object(const msgpack_object *const obj, Object *const arg) case MSGPACK_OBJECT_FLOAT: { STATIC_ASSERT(sizeof(Float) == sizeof(cur.mobj->via.f64), "Msgpack floating-point size does not match API integer"); - *cur.aobj = FLOATING_OBJ(cur.mobj->via.f64); + *cur.aobj = FLOAT_OBJ(cur.mobj->via.f64); break; } #define STR_CASE(type, attr, obj, dest, conv) \ -- cgit From ca4c8b7f8a7b9543ef01157cb5ab94783f624ac6 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 22 Jan 2017 04:55:26 +0300 Subject: api: Allow kObjectTypeNil to be zero without breaking compatibility --- src/nvim/msgpack_rpc/helpers.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/helpers.c b/src/nvim/msgpack_rpc/helpers.c index 64a018f5c3..972d5b3441 100644 --- a/src/nvim/msgpack_rpc/helpers.c +++ b/src/nvim/msgpack_rpc/helpers.c @@ -20,13 +20,20 @@ static msgpack_zone zone; static msgpack_sbuffer sbuffer; +/// Value by which objects represented as EXT type are shifted +/// +/// Subtracted when packing, added when unpacking. Used to allow moving +/// buffer/window/tabpage block inside ObjectType enum. This block yet cannot be +/// split or reordered. +#define EXT_OBJECT_TYPE_SHIFT kObjectTypeBuffer + #define HANDLE_TYPE_CONVERSION_IMPL(t, lt) \ bool msgpack_rpc_to_##lt(const msgpack_object *const obj, \ Integer *const arg) \ FUNC_ATTR_NONNULL_ALL \ { \ if (obj->type != MSGPACK_OBJECT_EXT \ - || obj->via.ext.type != kObjectType##t) { \ + || obj->via.ext.type + EXT_OBJECT_TYPE_SHIFT != kObjectType##t) { \ return false; \ } \ \ @@ -51,7 +58,8 @@ static msgpack_sbuffer sbuffer; msgpack_packer pac; \ msgpack_packer_init(&pac, &sbuffer, msgpack_sbuffer_write); \ msgpack_pack_int64(&pac, (handle_T)o); \ - msgpack_pack_ext(res, sbuffer.size, kObjectType##t); \ + msgpack_pack_ext(res, sbuffer.size, \ + kObjectType##t - EXT_OBJECT_TYPE_SHIFT); \ msgpack_pack_ext_body(res, sbuffer.data, sbuffer.size); \ msgpack_sbuffer_clear(&sbuffer); \ } @@ -211,7 +219,7 @@ bool msgpack_rpc_to_object(const msgpack_object *const obj, Object *const arg) break; } case MSGPACK_OBJECT_EXT: { - switch (cur.mobj->via.ext.type) { + switch ((ObjectType)(cur.mobj->via.ext.type + EXT_OBJECT_TYPE_SHIFT)) { case kObjectTypeBuffer: { cur.aobj->type = kObjectTypeBuffer; ret = msgpack_rpc_to_buffer(cur.mobj, &cur.aobj->data.integer); @@ -227,6 +235,15 @@ bool msgpack_rpc_to_object(const msgpack_object *const obj, Object *const arg) ret = msgpack_rpc_to_tabpage(cur.mobj, &cur.aobj->data.integer); break; } + case kObjectTypeNil: + case kObjectTypeBoolean: + case kObjectTypeInteger: + case kObjectTypeFloat: + case kObjectTypeString: + case kObjectTypeArray: + case kObjectTypeDictionary: { + break; + } } break; } @@ -350,6 +367,9 @@ void msgpack_rpc_from_object(const Object result, msgpack_packer *const res) kv_push(stack, ((APIToMPObjectStackItem) { &result, false, 0 })); while (kv_size(stack)) { APIToMPObjectStackItem cur = kv_last(stack); + STATIC_ASSERT(kObjectTypeWindow == kObjectTypeBuffer + 1 + && kObjectTypeTabpage == kObjectTypeWindow + 1, + "Buffer, window and tabpage enum items are in order"); switch (cur.aobj->type) { case kObjectTypeNil: { msgpack_pack_nil(res); -- cgit From 22d3ce9c29c163470dbb652199acbb7c82e3dca1 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 22 Jan 2017 05:35:48 +0300 Subject: msgpack_rpc: Fix #HANDLE_TYPE_CONVERSION_IMPL Function declarations generator is able to handle properly only the *first* function definition that is in macros, and only if it is the first entity in the macros. So msgpack_rpc_from_* was already really a static function, additionally its attributes were useless. This commit switches to explicit declarations and makes generated functions static. --- src/nvim/msgpack_rpc/helpers.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/helpers.c b/src/nvim/msgpack_rpc/helpers.c index 972d5b3441..94a4f5ce3e 100644 --- a/src/nvim/msgpack_rpc/helpers.c +++ b/src/nvim/msgpack_rpc/helpers.c @@ -28,9 +28,11 @@ static msgpack_sbuffer sbuffer; #define EXT_OBJECT_TYPE_SHIFT kObjectTypeBuffer #define HANDLE_TYPE_CONVERSION_IMPL(t, lt) \ - bool msgpack_rpc_to_##lt(const msgpack_object *const obj, \ - Integer *const arg) \ - FUNC_ATTR_NONNULL_ALL \ + static bool msgpack_rpc_to_##lt(const msgpack_object *const obj, \ + Integer *const arg) \ + REAL_FATTR_NONNULL_ALL REAL_FATTR_WARN_UNUSED_RESULT; \ + static bool msgpack_rpc_to_##lt(const msgpack_object *const obj, \ + Integer *const arg) \ { \ if (obj->type != MSGPACK_OBJECT_EXT \ || obj->via.ext.type + EXT_OBJECT_TYPE_SHIFT != kObjectType##t) { \ @@ -52,8 +54,9 @@ static msgpack_sbuffer sbuffer; return true; \ } \ \ - void msgpack_rpc_from_##lt(Integer o, msgpack_packer *res) \ - FUNC_ATTR_NONNULL_ARG(2) \ + static void msgpack_rpc_from_##lt(Integer o, msgpack_packer *res) \ + REAL_FATTR_NONNULL_ARG(2); \ + static void msgpack_rpc_from_##lt(Integer o, msgpack_packer *res) \ { \ msgpack_packer pac; \ msgpack_packer_init(&pac, &sbuffer, msgpack_sbuffer_write); \ -- cgit From ae4adcc70735a89bffb110bcf9d5a993b0786c4d Mon Sep 17 00:00:00 2001 From: ZyX Date: Sat, 28 Jan 2017 03:47:15 +0300 Subject: gendeclarations: Make declarations generator work with macros funcs Now it checks functions also after every semicolon and closing figure brace, possibly preceded by whitespaces (tabs and spaces). This should make messing with declarations in macros not needed. --- src/nvim/msgpack_rpc/helpers.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/helpers.c b/src/nvim/msgpack_rpc/helpers.c index 94a4f5ce3e..ec35d56978 100644 --- a/src/nvim/msgpack_rpc/helpers.c +++ b/src/nvim/msgpack_rpc/helpers.c @@ -30,9 +30,7 @@ static msgpack_sbuffer sbuffer; #define HANDLE_TYPE_CONVERSION_IMPL(t, lt) \ static bool msgpack_rpc_to_##lt(const msgpack_object *const obj, \ Integer *const arg) \ - REAL_FATTR_NONNULL_ALL REAL_FATTR_WARN_UNUSED_RESULT; \ - static bool msgpack_rpc_to_##lt(const msgpack_object *const obj, \ - Integer *const arg) \ + FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT \ { \ if (obj->type != MSGPACK_OBJECT_EXT \ || obj->via.ext.type + EXT_OBJECT_TYPE_SHIFT != kObjectType##t) { \ @@ -55,8 +53,7 @@ static msgpack_sbuffer sbuffer; } \ \ static void msgpack_rpc_from_##lt(Integer o, msgpack_packer *res) \ - REAL_FATTR_NONNULL_ARG(2); \ - static void msgpack_rpc_from_##lt(Integer o, msgpack_packer *res) \ + FUNC_ATTR_NONNULL_ARG(2) \ { \ msgpack_packer pac; \ msgpack_packer_init(&pac, &sbuffer, msgpack_sbuffer_write); \ -- cgit From 62fde319360e27a86c0deba0053ff230f80ca772 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 29 Jan 2017 01:29:51 +0300 Subject: api: Also shift numbers in api_metadata output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes problem introduced by “api: Allow kObjectTypeNil to be zero without breaking compatibility”: apparently there are clients which use metadata and there are which aren’t. For the first that commit would not be needed, for the second that commit misses this critical piece. --- src/nvim/msgpack_rpc/helpers.c | 7 ------- src/nvim/msgpack_rpc/helpers.h | 7 +++++++ 2 files changed, 7 insertions(+), 7 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/helpers.c b/src/nvim/msgpack_rpc/helpers.c index ec35d56978..21acd6a394 100644 --- a/src/nvim/msgpack_rpc/helpers.c +++ b/src/nvim/msgpack_rpc/helpers.c @@ -20,13 +20,6 @@ static msgpack_zone zone; static msgpack_sbuffer sbuffer; -/// Value by which objects represented as EXT type are shifted -/// -/// Subtracted when packing, added when unpacking. Used to allow moving -/// buffer/window/tabpage block inside ObjectType enum. This block yet cannot be -/// split or reordered. -#define EXT_OBJECT_TYPE_SHIFT kObjectTypeBuffer - #define HANDLE_TYPE_CONVERSION_IMPL(t, lt) \ static bool msgpack_rpc_to_##lt(const msgpack_object *const obj, \ Integer *const arg) \ diff --git a/src/nvim/msgpack_rpc/helpers.h b/src/nvim/msgpack_rpc/helpers.h index 7d9f114140..0e4cd1be6d 100644 --- a/src/nvim/msgpack_rpc/helpers.h +++ b/src/nvim/msgpack_rpc/helpers.h @@ -9,6 +9,13 @@ #include "nvim/event/wstream.h" #include "nvim/api/private/defs.h" +/// Value by which objects represented as EXT type are shifted +/// +/// Subtracted when packing, added when unpacking. Used to allow moving +/// buffer/window/tabpage block inside ObjectType enum. This block yet cannot be +/// split or reordered. +#define EXT_OBJECT_TYPE_SHIFT kObjectTypeBuffer + #ifdef INCLUDE_GENERATED_DECLARATIONS # include "msgpack_rpc/helpers.h.generated.h" #endif -- cgit From 3ea10077534cb1dcb1597ffcf85e601fa0c0e27b Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 13 Mar 2017 15:02:37 +0100 Subject: api: nvim_get_mode() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asynchronous API functions are served immediately, which means pending input could change the state of Nvim shortly after an async API function result is returned. nvim_get_mode() is different: - If RPCs are known to be blocked, it responds immediately (without flushing the input/event queue) - else it is handled just-in-time before waiting for input, after pending input was processed. This makes the result more reliable (but not perfect). Internally this is handled as a special case, but _semantically_ nothing has changed: API users never know when input flushes, so this internal special-case doesn't violate that. As far as API users are concerned, nvim_get_mode() is just another asynchronous API function. In all cases nvim_get_mode() never blocks for more than the time it takes to flush the input/event queue (~µs). Note: This doesn't address #6166; nvim_get_mode() will provoke #6166 if e.g. `d` is operator-pending. Closes #6159 --- src/nvim/msgpack_rpc/channel.c | 10 ++++++++++ src/nvim/msgpack_rpc/helpers.c | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 59594357de..799e6eadef 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -28,7 +28,9 @@ #include "nvim/map.h" #include "nvim/log.h" #include "nvim/misc1.h" +#include "nvim/state.h" #include "nvim/lib/kvec.h" +#include "nvim/os/input.h" #define CHANNEL_BUFFER_SIZE 0xffff @@ -433,6 +435,14 @@ static void handle_request(Channel *channel, msgpack_object *request) handler.async = true; } + if (handler.async) { + char buf[256] = { 0 }; + memcpy(buf, method->via.bin.ptr, MIN(255, method->via.bin.size)); + if (strcmp("nvim_get_mode", buf) == 0) { + handler.async = input_blocking(); + } + } + RequestEvent *event_data = xmalloc(sizeof(RequestEvent)); event_data->channel = channel; event_data->handler = handler; diff --git a/src/nvim/msgpack_rpc/helpers.c b/src/nvim/msgpack_rpc/helpers.c index 0228582d37..91ef5524ea 100644 --- a/src/nvim/msgpack_rpc/helpers.c +++ b/src/nvim/msgpack_rpc/helpers.c @@ -76,7 +76,7 @@ typedef struct { size_t idx; } MPToAPIObjectStackItem; -/// Convert type used by msgpack parser to Neovim own API type +/// Convert type used by msgpack parser to Nvim API type. /// /// @param[in] obj Msgpack value to convert. /// @param[out] arg Location where result of conversion will be saved. -- cgit From acfd2a2a29ae852ecc965ca888eb5049400bf39d Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Tue, 14 Mar 2017 00:44:03 +0100 Subject: input.c: Process only safe events before blocking. Introduce multiqueue_process_priority() to process only events at or above a certain priority. --- src/nvim/msgpack_rpc/channel.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 799e6eadef..9e43924db1 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -435,24 +435,26 @@ static void handle_request(Channel *channel, msgpack_object *request) handler.async = true; } - if (handler.async) { - char buf[256] = { 0 }; - memcpy(buf, method->via.bin.ptr, MIN(255, method->via.bin.size)); - if (strcmp("nvim_get_mode", buf) == 0) { - handler.async = input_blocking(); - } - } - - RequestEvent *event_data = xmalloc(sizeof(RequestEvent)); - event_data->channel = channel; - event_data->handler = handler; - event_data->args = args; - event_data->request_id = request_id; + RequestEvent *evdata = xmalloc(sizeof(RequestEvent)); + evdata->channel = channel; + evdata->handler = handler; + evdata->args = args; + evdata->request_id = request_id; incref(channel); if (handler.async) { - on_request_event((void **)&event_data); + bool is_get_mode = sizeof("nvim_get_mode") - 1 == method->via.bin.size + && !strncmp("nvim_get_mode", method->via.bin.ptr, method->via.bin.size); + + if (is_get_mode && !input_blocking()) { + // Schedule on the main loop with special priority. #6247 + Event ev = event_create(kEvPriorityAsync, on_request_event, 1, evdata); + multiqueue_put_event(channel->events, ev); + } else { + // Invoke immediately. + on_request_event((void **)&evdata); + } } else { - multiqueue_put(channel->events, on_request_event, 1, event_data); + multiqueue_put(channel->events, on_request_event, 1, evdata); } } -- cgit From f17a8185191b778960953508a5bf9b5f95b0560c Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Thu, 27 Apr 2017 13:54:54 +0200 Subject: api/nvim_get_mode: Use child-queue instead of "priority". --- src/nvim/msgpack_rpc/channel.c | 7 +++---- src/nvim/msgpack_rpc/channel.h | 5 +++++ 2 files changed, 8 insertions(+), 4 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 9e43924db1..911f2a6fa4 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -28,7 +28,6 @@ #include "nvim/map.h" #include "nvim/log.h" #include "nvim/misc1.h" -#include "nvim/state.h" #include "nvim/lib/kvec.h" #include "nvim/os/input.h" @@ -91,6 +90,7 @@ static msgpack_sbuffer out_buffer; /// Initializes the module void channel_init(void) { + ch_before_blocking_events = multiqueue_new_child(main_loop.events); channels = pmap_new(uint64_t)(); event_strings = pmap_new(cstr_t)(); msgpack_sbuffer_init(&out_buffer); @@ -446,9 +446,8 @@ static void handle_request(Channel *channel, msgpack_object *request) && !strncmp("nvim_get_mode", method->via.bin.ptr, method->via.bin.size); if (is_get_mode && !input_blocking()) { - // Schedule on the main loop with special priority. #6247 - Event ev = event_create(kEvPriorityAsync, on_request_event, 1, evdata); - multiqueue_put_event(channel->events, ev); + // Defer the event to a special queue used by os/input.c. #6247 + multiqueue_put(ch_before_blocking_events, on_request_event, 1, evdata); } else { // Invoke immediately. on_request_event((void **)&evdata); diff --git a/src/nvim/msgpack_rpc/channel.h b/src/nvim/msgpack_rpc/channel.h index 0d92976d02..f8fe6f129b 100644 --- a/src/nvim/msgpack_rpc/channel.h +++ b/src/nvim/msgpack_rpc/channel.h @@ -11,6 +11,11 @@ #define METHOD_MAXLEN 512 +/// HACK: os/input.c drains this queue immediately before blocking for input. +/// Events on this queue are async-safe, but they need the resolved state +/// of os_inchar(), so they are processed "just-in-time". +MultiQueue *ch_before_blocking_events; + #ifdef INCLUDE_GENERATED_DECLARATIONS # include "msgpack_rpc/channel.h.generated.h" #endif -- cgit From 1483800cdff3bd00faabe66436a1618f567f6754 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 1 May 2017 17:04:56 +0200 Subject: coverity/161682: FP: Dereference after null check (#6630) --- src/nvim/msgpack_rpc/channel.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 911f2a6fa4..cd64e14976 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -442,8 +442,7 @@ static void handle_request(Channel *channel, msgpack_object *request) evdata->request_id = request_id; incref(channel); if (handler.async) { - bool is_get_mode = sizeof("nvim_get_mode") - 1 == method->via.bin.size - && !strncmp("nvim_get_mode", method->via.bin.ptr, method->via.bin.size); + bool is_get_mode = handler.fn == handle_nvim_get_mode; if (is_get_mode && !input_blocking()) { // Defer the event to a special queue used by os/input.c. #6247 -- cgit From aace622ca5ffe4deb72989452dc60877304d71aa Mon Sep 17 00:00:00 2001 From: Carlo Abelli Date: Mon, 8 May 2017 09:08:12 -0400 Subject: refactor/single-include (#6687) --- src/nvim/msgpack_rpc/server.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/server.h b/src/nvim/msgpack_rpc/server.h index f1a6703938..5446e40e0b 100644 --- a/src/nvim/msgpack_rpc/server.h +++ b/src/nvim/msgpack_rpc/server.h @@ -1,6 +1,8 @@ #ifndef NVIM_MSGPACK_RPC_SERVER_H #define NVIM_MSGPACK_RPC_SERVER_H +#include + #ifdef INCLUDE_GENERATED_DECLARATIONS # include "msgpack_rpc/server.h.generated.h" #endif -- cgit From 3efc82cbb28a1fd6f1c10a4fc91e8ce749ad384f Mon Sep 17 00:00:00 2001 From: Marco Hinz Date: Thu, 4 May 2017 17:59:51 +0200 Subject: Server: use uv_getaddrinfo() for $NVIM_LISTEN_ADDRESS This change implicitly adds IPv6 support. If the address contains ":", we try to use a TCP socket instead of a Unix domain socket. Everything in front of the last occurrence of ":" is the hostname and everything after it the port. If the hostname lookup fails, we fall back to using a Unix domain socket. If the port is empty ("localhost:"), a random port will be assigned. Examples: NVIM_LISTEN_ADDRESS=localhost:12345 -> TCP (IPv4 or IPv6), port: 12345 NVIM_LISTEN_ADDRESS=localhost: -> TCP (IPv4 or IPv6), port: random (> 1024) NVIM_LISTEN_ADDRESS=localhost:0 -> TCP (IPv4 or IPv6), port: random (> 1024) NVIM_LISTEN_ADDRESS=localhost -> Unix domain socket "localhost" in current dir --- src/nvim/msgpack_rpc/server.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index b6958088ca..ee96fa1a74 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -97,31 +97,36 @@ char *server_address_new(void) #endif } -/// Starts listening for API calls on the TCP address or pipe path `endpoint`. +/// Starts listening for API calls. +/// /// The socket type is determined by parsing `endpoint`: If it's a valid IPv4 -/// address in 'ip[:port]' format, then it will be TCP socket. The port is -/// optional and if omitted defaults to NVIM_DEFAULT_TCP_PORT. Otherwise it -/// will be a unix socket or named pipe. +/// or IPv6 address in 'ip:[port]' format, then it will be a TCP socket. +/// Otherwise it will be a Unix socket or named pipe (Windows). +/// +/// If no port is given, a random one will be assigned. /// -/// @param endpoint Address of the server. Either a 'ip[:port]' string or an -/// arbitrary identifier (trimmed to 256 bytes) for the unix socket or -/// named pipe. +/// @param endpoint Address of the server. Either a 'ip:[port]' string or an +/// arbitrary identifier (trimmed to 256 bytes) for the Unix +/// socket or named pipe. /// @returns 0 on success, 1 on a regular error, and negative errno -/// on failure to bind or connect. +/// on failure to bind or listen. int server_start(const char *endpoint) { - if (endpoint == NULL) { - ELOG("Attempting to start server on NULL endpoint"); + if (endpoint == NULL || endpoint[0] == '\0') { + ELOG("Empty or NULL endpoint"); return 1; } SocketWatcher *watcher = xmalloc(sizeof(SocketWatcher)); - socket_watcher_init(&main_loop, watcher, endpoint, NULL); + socket_watcher_init(&main_loop, watcher, endpoint); // Check if a watcher for the endpoint already exists for (int i = 0; i < watchers.ga_len; i++) { if (!strcmp(watcher->addr, ((SocketWatcher **)watchers.ga_data)[i]->addr)) { ELOG("Already listening on %s", watcher->addr); + if (watcher->stream->type == UV_TCP) { + uv_freeaddrinfo(watcher->uv.tcp.addrinfo); + } socket_watcher_close(watcher, free_server); return 1; } -- cgit From fd5e4e2e4c6b231980effc3055fb93300bd055b0 Mon Sep 17 00:00:00 2001 From: Marco Hinz Date: Tue, 16 May 2017 14:56:08 +0200 Subject: Server: don't fall back to Unix sockets --- src/nvim/msgpack_rpc/server.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index ee96fa1a74..bae5a32850 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -118,7 +118,12 @@ int server_start(const char *endpoint) } SocketWatcher *watcher = xmalloc(sizeof(SocketWatcher)); - socket_watcher_init(&main_loop, watcher, endpoint); + + int result = socket_watcher_init(&main_loop, watcher, endpoint); + if (result < 0) { + xfree(watcher); + return result; + } // Check if a watcher for the endpoint already exists for (int i = 0; i < watchers.ga_len; i++) { @@ -132,7 +137,7 @@ int server_start(const char *endpoint) } } - int result = socket_watcher_start(watcher, MAX_CONNECTIONS, connection_cb); + result = socket_watcher_start(watcher, MAX_CONNECTIONS, connection_cb); if (result < 0) { ELOG("Failed to start server: %s", uv_strerror(result)); socket_watcher_close(watcher, free_server); -- cgit From 6a7593875827374c15484dd4eecd31a88f8c6f77 Mon Sep 17 00:00:00 2001 From: Björn Linse Date: Wed, 26 Apr 2017 13:10:21 +0200 Subject: channels: implement sockopen() to connect to socket Helped-By: oni-link --- src/nvim/msgpack_rpc/channel.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index cd64e14976..0ff749649b 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -145,6 +145,25 @@ void channel_from_connection(SocketWatcher *watcher) rstream_start(&channel->data.stream, parse_msgpack, channel); } +uint64_t channel_connect(bool tcp, const char *address, + int timeout, const char **error) +{ + Channel *channel = register_channel(kChannelTypeSocket, 0, NULL); + if (!socket_connect(&main_loop, &channel->data.stream, + tcp, address, timeout, error)) { + decref(channel); + return 0; + } + + incref(channel); // close channel only after the stream is closed + channel->data.stream.internal_close_cb = close_cb; + channel->data.stream.internal_data = channel; + wstream_init(&channel->data.stream, 0); + rstream_init(&channel->data.stream, CHANNEL_BUFFER_SIZE); + rstream_start(&channel->data.stream, parse_msgpack, channel); + return channel->id; +} + /// Sends event/arguments to channel /// /// @param id The channel id. If 0, the event will be sent to all -- cgit From 5a151555c8dce70bbf235e7f6d5bd1ced5e7c46c Mon Sep 17 00:00:00 2001 From: Björn Linse Date: Sat, 6 May 2017 09:31:36 +0200 Subject: sockets: don't deadlock when connecting to own pipe address --- src/nvim/msgpack_rpc/channel.c | 78 ++++++++++++++++++++++++++++++++++-------- src/nvim/msgpack_rpc/server.c | 12 +++++++ 2 files changed, 75 insertions(+), 15 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 0ff749649b..e8ee0ede75 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -12,6 +12,7 @@ #include "nvim/api/vim.h" #include "nvim/api/ui.h" #include "nvim/msgpack_rpc/channel.h" +#include "nvim/msgpack_rpc/server.h" #include "nvim/event/loop.h" #include "nvim/event/libuv_process.h" #include "nvim/event/rstream.h" @@ -28,6 +29,7 @@ #include "nvim/map.h" #include "nvim/log.h" #include "nvim/misc1.h" +#include "nvim/path.h" #include "nvim/lib/kvec.h" #include "nvim/os/input.h" @@ -41,7 +43,8 @@ typedef enum { kChannelTypeSocket, kChannelTypeProc, - kChannelTypeStdio + kChannelTypeStdio, + kChannelTypeInternal } ChannelType; typedef struct { @@ -125,7 +128,7 @@ uint64_t channel_from_process(Process *proc, uint64_t id) wstream_init(proc->in, 0); rstream_init(proc->out, 0); - rstream_start(proc->out, parse_msgpack, channel); + rstream_start(proc->out, receive_msgpack, channel); return channel->id; } @@ -142,12 +145,22 @@ void channel_from_connection(SocketWatcher *watcher) channel->data.stream.internal_data = channel; wstream_init(&channel->data.stream, 0); rstream_init(&channel->data.stream, CHANNEL_BUFFER_SIZE); - rstream_start(&channel->data.stream, parse_msgpack, channel); + rstream_start(&channel->data.stream, receive_msgpack, channel); } uint64_t channel_connect(bool tcp, const char *address, int timeout, const char **error) { + if (!tcp) { + char *path = fix_fname(address); + if (server_owns_pipe_address(path)) { + // avoid deadlock + xfree(path); + return channel_create_internal(); + } + xfree(path); + } + Channel *channel = register_channel(kChannelTypeSocket, 0, NULL); if (!socket_connect(&main_loop, &channel->data.stream, tcp, address, timeout, error)) { @@ -160,7 +173,7 @@ uint64_t channel_connect(bool tcp, const char *address, channel->data.stream.internal_data = channel; wstream_init(&channel->data.stream, 0); rstream_init(&channel->data.stream, CHANNEL_BUFFER_SIZE); - rstream_start(&channel->data.stream, parse_msgpack, channel); + rstream_start(&channel->data.stream, receive_msgpack, channel); return channel->id; } @@ -324,11 +337,20 @@ void channel_from_stdio(void) incref(channel); // stdio channels are only closed on exit // read stream rstream_init_fd(&main_loop, &channel->data.std.in, 0, CHANNEL_BUFFER_SIZE); - rstream_start(&channel->data.std.in, parse_msgpack, channel); + rstream_start(&channel->data.std.in, receive_msgpack, channel); // write stream wstream_init_fd(&main_loop, &channel->data.std.out, 1, 0); } +/// Creates a loopback channel. This is used to avoid deadlock +/// when an instance connects to its own named pipe. +uint64_t channel_create_internal(void) +{ + Channel *channel = register_channel(kChannelTypeInternal, 0, NULL); + incref(channel); // internal channel lives until process exit + return channel->id; +} + void channel_process_exit(uint64_t id, int status) { Channel *channel = pmap_get(uint64_t)(channels, id); @@ -337,8 +359,8 @@ void channel_process_exit(uint64_t id, int status) decref(channel); } -static void parse_msgpack(Stream *stream, RBuffer *rbuf, size_t c, void *data, - bool eof) +static void receive_msgpack(Stream *stream, RBuffer *rbuf, size_t c, + void *data, bool eof) { Channel *channel = data; incref(channel); @@ -360,6 +382,14 @@ static void parse_msgpack(Stream *stream, RBuffer *rbuf, size_t c, void *data, rbuffer_read(rbuf, msgpack_unpacker_buffer(channel->unpacker), count); msgpack_unpacker_buffer_consumed(channel->unpacker, count); + parse_msgpack(channel); + +end: + decref(channel); +} + +static void parse_msgpack(Channel *channel) +{ msgpack_unpacked unpacked; msgpack_unpacked_init(&unpacked); msgpack_unpack_return result; @@ -383,7 +413,7 @@ static void parse_msgpack(Stream *stream, RBuffer *rbuf, size_t c, void *data, } msgpack_unpacked_destroy(&unpacked); // Bail out from this event loop iteration - goto end; + return; } handle_request(channel, &unpacked.data); @@ -407,11 +437,9 @@ static void parse_msgpack(Stream *stream, RBuffer *rbuf, size_t c, void *data, "This error can also happen when deserializing " "an object with high level of nesting"); } - -end: - decref(channel); } + static void handle_request(Channel *channel, msgpack_object *request) FUNC_ATTR_NONNULL_ALL { @@ -521,8 +549,11 @@ static bool channel_write(Channel *channel, WBuffer *buffer) case kChannelTypeStdio: success = wstream_write(&channel->data.std.out, buffer); break; - default: - abort(); + case kChannelTypeInternal: + incref(channel); + CREATE_EVENT(channel->events, internal_read_event, 2, channel, buffer); + success = true; + break; } if (!success) { @@ -539,6 +570,22 @@ static bool channel_write(Channel *channel, WBuffer *buffer) return success; } +static void internal_read_event(void **argv) +{ + Channel *channel = argv[0]; + WBuffer *buffer = argv[1]; + + msgpack_unpacker_reserve_buffer(channel->unpacker, buffer->size); + memcpy(msgpack_unpacker_buffer(channel->unpacker), + buffer->data, buffer->size); + msgpack_unpacker_buffer_consumed(channel->unpacker, buffer->size); + + parse_msgpack(channel); + + decref(channel); + wstream_release_wbuffer(buffer); +} + static void send_error(Channel *channel, uint64_t id, char *err) { Error e = ERROR_INIT; @@ -655,8 +702,9 @@ static void close_channel(Channel *channel) stream_close(&channel->data.std.out, NULL, NULL); multiqueue_put(main_loop.fast_events, exit_event, 1, channel); return; - default: - abort(); + case kChannelTypeInternal: + // nothing to free. + break; } decref(channel); diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index bae5a32850..c9edd05dc2 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -97,6 +97,18 @@ char *server_address_new(void) #endif } +/// Check if this instance owns a pipe address. +/// The argument must already be resolved to an absolute path! +bool server_owns_pipe_address(const char *path) +{ + for (int i = 0; i < watchers.ga_len; i++) { + if (!strcmp(path, ((SocketWatcher **)watchers.ga_data)[i]->addr)) { + return true; + } + } + return false; +} + /// Starts listening for API calls. /// /// The socket type is determined by parsing `endpoint`: If it's a valid IPv4 -- cgit From fcc9d999670c6fb29ac01e9c8d0a7e787ccfabea Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Wed, 31 May 2017 03:37:29 +0200 Subject: channel_write: fix compiler warning --- src/nvim/msgpack_rpc/channel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index e8ee0ede75..413b800af5 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -532,7 +532,7 @@ static void on_request_event(void **argv) static bool channel_write(Channel *channel, WBuffer *buffer) { - bool success; + bool success = false; if (channel->closed) { wstream_release_wbuffer(buffer); -- cgit From fe1af9c2bc020cdb9e1e86e1f16d6060246f957a Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Tue, 30 May 2017 02:03:08 +0200 Subject: log: Always enable; remove DISABLE_LOG - Establish ERROR log level as "critical". Such errors are rare and will be valuable when users encounter unusual circumstances. - Set -DMIN_LOG_LEVEL=3 for release-type builds --- src/nvim/msgpack_rpc/channel.c | 12 ++++++------ src/nvim/msgpack_rpc/server.c | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) (limited to 'src/nvim/msgpack_rpc') diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 413b800af5..68ac35bc4e 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -370,7 +370,7 @@ static void receive_msgpack(Stream *stream, RBuffer *rbuf, size_t c, char buf[256]; snprintf(buf, sizeof(buf), "ch %" PRIu64 " was closed by the client", channel->id); - call_set_error(channel, buf); + call_set_error(channel, buf, WARNING_LOG_LEVEL); goto end; } @@ -409,7 +409,7 @@ static void parse_msgpack(Channel *channel) "ch %" PRIu64 " returned a response with an unknown request " "id. Ensure the client is properly synchronized", channel->id); - call_set_error(channel, buf); + call_set_error(channel, buf, ERROR_LOG_LEVEL); } msgpack_unpacked_destroy(&unpacked); // Bail out from this event loop iteration @@ -459,7 +459,7 @@ static void handle_request(Channel *channel, msgpack_object *request) snprintf(buf, sizeof(buf), "ch %" PRIu64 " sent an invalid message, closed.", channel->id); - call_set_error(channel, buf); + call_set_error(channel, buf, ERROR_LOG_LEVEL); } api_clear_error(&error); return; @@ -564,7 +564,7 @@ static bool channel_write(Channel *channel, WBuffer *buffer) "Before returning from a RPC call, ch %" PRIu64 " was " "closed due to a failed write", channel->id); - call_set_error(channel, buf); + call_set_error(channel, buf, ERROR_LOG_LEVEL); } return success; @@ -795,9 +795,9 @@ static void complete_call(msgpack_object *obj, Channel *channel) } } -static void call_set_error(Channel *channel, char *msg) +static void call_set_error(Channel *channel, char *msg, int loglevel) { - ELOG("RPC: %s", msg); + LOG(loglevel, "RPC: %s", msg); for (size_t i = 0; i < kv_size(channel->call_stack); i++) { ChannelCallFrame *frame = kv_A(channel->call_stack, i); frame->returned = true; diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index c9edd05dc2..1e0cc27886 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -125,7 +125,7 @@ bool server_owns_pipe_address(const char *path) int server_start(const char *endpoint) { if (endpoint == NULL || endpoint[0] == '\0') { - ELOG("Empty or NULL endpoint"); + WLOG("Empty or NULL endpoint"); return 1; } @@ -151,7 +151,7 @@ int server_start(const char *endpoint) result = socket_watcher_start(watcher, MAX_CONNECTIONS, connection_cb); if (result < 0) { - ELOG("Failed to start server: %s", uv_strerror(result)); + WLOG("Failed to start server: %s", uv_strerror(result)); socket_watcher_close(watcher, free_server); return result; } -- cgit