From b4507b56afcdcdcc443b5a838aaadde7e62ed520 Mon Sep 17 00:00:00 2001 From: Přemysl Janouch Date: Mon, 8 Jan 2018 21:53:42 +0100 Subject: degesch: thorough review, no functional changes --- degesch.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 61 insertions(+), 20 deletions(-) diff --git a/degesch.c b/degesch.c index ad4c33e..3ac042d 100644 --- a/degesch.c +++ b/degesch.c @@ -1,7 +1,7 @@ /* * degesch.c: the experimental IRC client * - * Copyright (c) 2015 - 2017, Přemysl Janouch + * Copyright (c) 2015 - 2018, Přemysl Janouch * * Permission to use, copy, modify, and/or distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -2042,7 +2042,6 @@ struct app_context struct buffer *current_buffer; ///< The current buffer struct buffer *last_buffer; ///< Last used buffer - // TODO: make buffer names fully unique like weechat does struct str_map buffers_by_name; ///< Buffers by name unsigned backlog_limit; ///< Limit for buffer lines @@ -2096,6 +2095,7 @@ filter_color_cube_for_acceptable_nick_colors (size_t *len) size_t len_counter = 0; for (int x = 0; x < 6 * 6 * 6; x++) { + // FIXME this isn't exactly right, the values aren't linear int r = x / 36; int g = (x / 6) % 6; int b = (x % 6); @@ -3122,6 +3122,8 @@ static const int g_mirc_to_terminal[] = [MIRC_L_GRAY] = COLOR_256 (WHITE, 252), }; +// TODO: support more colors, see https://modern.ircdocs.horse/formatting.html +// + http://anti.teamidiot.de/static/nei/*/extended_mirc_color_proposal.html static const char * formatter_parse_mirc_color (struct formatter *self, const char *s) { @@ -3778,7 +3780,7 @@ static void buffer_line_write_time (struct formatter *f, struct buffer_line *line, FILE *stream, int flush_opts) { - // Normal timestamps don't include the date, this way the user won't be + // Normal timestamps don't include the date, make sure the user won't be // confused as to when an event has happened buffer_update_time (f->ctx, line->when, stream, flush_opts); @@ -4003,7 +4005,6 @@ buffer_get_log_path (struct buffer *buffer) (void) mkdir_with_parents (path.str, NULL); - // TODO: make sure global and server buffers don't collide with filenames str_append_c (&path, '/'); make_log_filename (buffer->name, &path); str_append (&path, ".log"); @@ -4016,6 +4017,7 @@ buffer_open_log_file (struct app_context *ctx, struct buffer *buffer) if (!ctx->logging || buffer->log_file) return; + // TODO: should we try to reopen files wrt. case mapping? char *path = buffer_get_log_path (buffer); if (!(buffer->log_file = fopen (path, "ab"))) log_global_error (ctx, "Couldn't open log file `#s': #l", @@ -4064,7 +4066,8 @@ buffer_add (struct app_context *ctx, struct buffer *buffer) buffer_open_log_file (ctx, buffer); - // In theory this can't cause changes in the prompt + // Normally this doesn't cause changes in the prompt but a prompt hook + // could decide to show some information for all buffers nonetheless refresh_prompt (ctx); } @@ -4140,7 +4143,7 @@ buffer_print_backlog (struct app_context *ctx, struct buffer *buffer) free (buffer_name_localized); } - // That is, minus the readline prompt + // That is, minus the readline prompt (taking at least one line) int display_limit = MAX (10, g_terminal.lines - 1); int to_display = 0; @@ -4176,6 +4179,9 @@ buffer_activate (struct app_context *ctx, struct buffer *buffer) { if (ctx->current_buffer == buffer) return; + + // This is the only place where the unread messages marker + // and highlight indicator are reset if (ctx->current_buffer) { ctx->current_buffer->new_messages_count = 0; @@ -5025,6 +5031,7 @@ irc_process_hooks (struct server *s, char *input) return NULL; } + // The old input may get freed, so we compare against a hash of it uint64_t new_hash = siphash_wrapper (input, strlen (input)); if (new_hash != hash) log_server_debug (s, "#a>= \"#S\"#r", ATTR_JOIN, input); @@ -5067,7 +5074,9 @@ irc_try_read (struct server *s) enum socket_io_result result = s->transport->try_read (s); if (s->read_buffer.len >= (1 << 20)) { - // XXX: this is stupid; if anything, count it in dependence of time + // XXX: this is stupid; if anything, count it in dependence of time; + // we could make transport_tls_try_read() limit the immediate amount + // of data read like socket_io_try_read() does and remove this check log_server_error (s, s->buffer, "The IRC server seems to spew out data frantically"); return SOCKET_IO_ERROR; @@ -5302,7 +5311,7 @@ transport_tls_init_ctx (struct server *s, SSL_CTX *ssl_ctx, struct error **e) return false; } - // Only inform the user if we're not actually verifying + // Just inform the user if we're not actually verifying log_server_error (s, s->buffer, "#s", error->message); error_free (error); } @@ -5363,7 +5372,7 @@ transport_tls_init (struct server *s, const char *hostname, struct error **e) // Enable SNI, FWIW; literal IP addresses aren't allowed struct in6_addr dummy; - if (!inet_pton (AF_INET, hostname, &dummy) + if (!inet_pton (AF_INET, hostname, &dummy) && !inet_pton (AF_INET6, hostname, &dummy)) SSL_set_tlsext_host_name (ssl, hostname); @@ -6035,7 +6044,7 @@ irc_is_highlight (struct server *s, const char *message) // Special characters allowed in nicknames by RFC 2812: []\`_^{|} and - // Also excluded from the ASCII: common user channel prefixes: +%@&~ - // XXX: why did I exclude those? It won't match when newbies use them. + // XXX: why did I exclude those? It won't match when IRC newbies use them. const char *delimiters = ",.;:!?()<>/=#$* \t\r\n\v\f\"'"; bool result = false; @@ -6329,6 +6338,7 @@ irc_handle_sent_privmsg (struct server *s, const struct irc_message *msg) return; // This ignores empty messages which we should not normally send + // and the server is likely going to reject with an error reply anyway struct ctcp_chunk *chunks = ctcp_parse (msg->params.vector[1]); LIST_FOR_EACH (struct ctcp_chunk, iter, chunks) { @@ -6367,6 +6377,7 @@ static void irc_process_sent_message (const struct irc_message *msg, struct server *s) { // The server is free to reject even a matching prefix + // XXX: even though no prefix should normally be present, this is racy if (msg->prefix && !irc_is_this_us (s, msg->prefix)) return; @@ -7135,6 +7146,10 @@ irc_on_registered (struct server *s, const char *nickname) refresh_prompt (s->ctx); // XXX: we can also use WHOIS if it's not supported (optional by RFC 2812) + // TODO: maybe rather always use RPL_ISUPPORT NICKLEN & USERLEN & HOSTLEN + // since we don't seem to follow any subsequent changes in userhost; + // unrealircd sends RPL_HOSTHIDDEN (396), which has an optional user part, + // and there is also CAP CHGHOST which /may/ send it to ourselves irc_send (s, "USERHOST %s", s->irc_user->nickname); // A little hack that reinstates auto-away status when we get disconnected @@ -7225,7 +7240,7 @@ irc_handle_rpl_namreply (struct server *s, const struct irc_message *msg) struct channel_user_sort_entry { - struct server *s; ///< Server + struct server *s; ///< Server (because of the qsort API) struct channel_user *channel_user; ///< Channel user }; @@ -8182,7 +8197,10 @@ server_rename (struct app_context *ctx, struct server *s, const char *new_name) struct buffer *buffer; while ((buffer = str_map_iter_next (&iter))) { - // FIXME: creation of buffer names should be centralized + // TODO: creation of buffer names should be centralized -> replace + // calls to buffer_rename() and manual setting of buffer names + // with something like buffer_autorename() -- just mind the mess + // in irc_handle_nick(), which can hopefully be simplified char *x = NULL; switch (buffer->type) { @@ -8387,6 +8405,7 @@ lua_plugin_log_error error_free (error); } +/// Pop "n" values from the stack into a table, using their indexes as keys static void lua_plugin_pack (lua_State *L, int n) { @@ -8541,7 +8560,7 @@ LUA_WEAK_DECLARE (channel, XLUA_CHANNEL_METATABLE) LUA_WEAK_DECLARE (buffer, XLUA_BUFFER_METATABLE) LUA_WEAK_DECLARE (server, XLUA_SERVER_METATABLE) -// The global context is kind of fake and don't have any ref-counting, +// The global context is kind of fake and doesn't have any ref-counting, // however it's still very much an object static struct lua_weak_info lua_ctx_info = { @@ -9332,8 +9351,13 @@ lua_connection_close (lua_State *L) if (self->socket_fd != -1) { self->closing = true; + // NOTE: this seems to do nothing on Linux (void) shutdown (self->socket_fd, SHUT_RD); + // Right now we want to wait until all data is flushed to the socket + // and can't call close() here immediately -- a rewrite to use async + // would enable the user to await on either :send() or :flush(); + // a successful send() doesn't necessarily mean anything though if (!self->write_buffer.len) lua_connection_discard (self); } @@ -9413,6 +9437,8 @@ static bool lua_connection_invoke_on_error (struct lua_connection *self, const char *error, struct error **e) { + // XXX: not sure if ignoring errors after :close() is always desired; + // code might want to make sure that data are transferred successfully if (!self->closing && lua_connection_cb_lookup (self, "on_error", e) && !lua_connection_eat_nil (self)) @@ -9791,8 +9817,13 @@ lua_wait_dial_check (struct lua_wait_channel *wchannel) lua_State *L = self->super.task->thread; if (self->connection) { - // FIXME: this way the connection can leak, it shouldn't stay in cache - // automatically all the time but clean itself up on GC + // FIXME: this way the connection may leak -- we pass the value to the + // task manager on the stack and forget about it but still leave the + // connection in the cache. That is because right now, when Lua code + // sets up callbacks in the connection object and returns, it might + // get otherwise GC'd since nothing else keeps referencing it. + // By rewriting lua_connection using async, tasks and wait channels + // would hold a reference, allowing us to remove it from the cache. lua_cache_get (L, self->connection); lua_pushstring (L, self->hostname); self->connection = NULL; @@ -10040,7 +10071,7 @@ lua_plugin_push_struct (lua_State *L, struct lua_plugin *self, { if (type == ISPECT_STR) { - struct str *s = value; + const struct str *s = value; lua_pushlstring (L, s->str, s->len); return; } @@ -10194,7 +10225,8 @@ lua_plugin_reg_meta (lua_State *L, const char *name, luaL_Reg *fns) static void lua_plugin_reg_weak (lua_State *L, struct lua_weak_info *info, luaL_Reg *fns) { - // Create a mapping from the object type back to our metadata + // Create a mapping from the object type (info->ispect) back to metadata + // so that we can figure out what to create from ISPECT_REF fields lua_pushlightuserdata (L, info); lua_rawsetp (L, LUA_REGISTRYINDEX, info->ispect); @@ -10359,7 +10391,8 @@ plugin_load (struct app_context *ctx, const char *name) struct plugin *plugin = plugin_load_by_name (ctx, name, &e); if (plugin) { - // FIXME: this way the real name isn't available to the plugin on load + // FIXME: this way the real name isn't available to the plugin on load, + // which has effect on e.g. plugin_config_name() free (plugin->name); plugin->name = xstrdup (name); @@ -10464,6 +10497,7 @@ static char * maybe_cut_word_from_end (char **s, word_validator_fn validator, void *user_data) { // Find the start and end of the last word + // Contrary to maybe_cut_word(), we ignore all whitespace at the end char *start = *s, *end = start + strlen (start); while (end > start && strchr (WORD_BREAKING_CHARS, end [-1])) end--; @@ -11149,7 +11183,9 @@ static bool handle_command_topic (struct handler_args *a) { if (*a->arguments) - // FIXME: there's no way to unset the topic + // FIXME: there's no way to start the topic with whitespace + // FIXME: there's no way to unset the topic; + // we could adopt the Tcl style of "-switches" with "--" sentinels irc_send (a->s, "TOPIC %s :%s", a->channel_name, a->arguments); else irc_send (a->s, "TOPIC %s", a->channel_name); @@ -11228,6 +11264,7 @@ mass_channel_mode_mask_list for (size_t i = 0; i < v.len; i++) { char *target = v.vector[i]; + // TODO: support EXTBAN and leave those alone, too if (strpbrk (target, "!@*?")) continue; @@ -11768,6 +11805,7 @@ handle_command_help (struct handler_args *a) if (!*a->arguments) return show_command_list (ctx); + // TODO: we should probably also accept commands names with a leading slash char *command = cut_word (&a->arguments); for (size_t i = 0; i < N_ELEMENTS (g_command_handlers); i++) { @@ -12963,7 +13001,9 @@ on_readline_input (char *line) add_history (line); // readline always erases the input line after returning from here, - // but we don't want that in order to allow correct buffer switching + // but we don't want that to happen if the command to be executed + // would switch the buffer (we'd keep the already executed command in + // the old buffer and delete any input restored from the new buffer) strv_append_owned (&ctx->pending_input, line); poller_idle_set (&ctx->input_event); } @@ -13450,6 +13490,7 @@ process_mirc_escape (const struct pollfd *fd, struct app_context *ctx) goto error; buf->str[++buf->len] = '\0'; + // XXX: I think this should be global and shared with Readline/libedit mbstate_t state; memset (&state, 0, sizeof state); -- cgit v1.2.3-70-g09d2