From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id AB9546DE0941 for ; Sat, 10 Dec 2016 11:12:37 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: 0.519 X-Spam-Level: X-Spam-Status: No, score=0.519 tagged_above=-999 required=5 tests=[AWL=-0.133, SPF_NEUTRAL=0.652] autolearn=disabled Received: from arlo.cworth.org ([127.0.0.1]) by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id i6CKTgyqF0UU for ; Sat, 10 Dec 2016 11:12:36 -0800 (PST) Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) by arlo.cworth.org (Postfix) with ESMTP id 21A766DE024A for ; Sat, 10 Dec 2016 11:12:35 -0800 (PST) Received: from guru.guru-group.fi (localhost [IPv6:::1]) by guru.guru-group.fi (Postfix) with ESMTP id 94ECF10005A; Sat, 10 Dec 2016 21:13:10 +0200 (EET) From: Tomi Ollila To: Ioan-Adrian Ratiu , notmuch@notmuchmail.org Subject: Re: [PATCH v5 2/2] notmuch-config: replace config reading function In-Reply-To: <20161208122445.30918-3-adi@adirat.com> References: <20161208122445.30918-1-adi@adirat.com> <20161208122445.30918-3-adi@adirat.com> User-Agent: Notmuch/0.23.3+85~g2b85e66 (https://notmuchmail.org) Emacs/24.5.1 (x86_64-unknown-linux-gnu) X-Face: HhBM'cA~ MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Dec 2016 19:12:37 -0000 On Thu, Dec 08 2016, Ioan-Adrian Ratiu wrote: > Config files are currently read using glib's g_key_file_load_from_file > function which is very inconvenient because it's limited by design to read > only from "regular data files" in a filesystem. Because of this limitation > notmuch can't read configs from pipes, fifos, sockets, stdin, etc. Not even > "notmuch --config=/dev/stdin" works: > > Error reading configuration file /dev/stdin: Not a regular file > > So replace g_key_file_load_from_file with g_key_file_load_from_data which > gives us much more freedom to read configs from multiple sources. > > This also helps the more security sensitive users: If someone has private > information in the config file, it can be encrypted on disk, then decrypted > in RAM and passed through a pipe directly to notmuch without the use of > intermediate plain text files. > > Signed-off-by: Ioan-Adrian Ratiu > --- > notmuch-config.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 52 insertions(+), 10 deletions(-) > > diff --git a/notmuch-config.c b/notmuch-config.c > index bd52790..30823cb 100644 > --- a/notmuch-config.c > +++ b/notmuch-config.c > @@ -205,32 +205,74 @@ get_username_from_passwd_file (void *ctx) > static notmuch_bool_t > get_config_from_file (notmuch_config_t *config, notmuch_bool_t create_new) > { > + #define BUF_SIZE 4096 > + char *config_str = NULL; > + int config_len = 0; > + int config_bufsize = BUF_SIZE; > + size_t len; > GError *error = NULL; > notmuch_bool_t ret = FALSE; > > - if (g_key_file_load_from_file (config->key_file, config->filename, > - G_KEY_FILE_KEEP_COMMENTS, &error)) > - return TRUE; > - > - if (error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT) { > + FILE *fp = fopen(config->filename, "r"); > + if (fp == NULL) { > /* If create_new is true, then the caller is prepared for a > * default configuration file in the case of FILE NOT FOUND. > */ > if (create_new) { > config->is_new = TRUE; > ret = TRUE; > + goto out; > } else { > - fprintf (stderr, "Configuration file %s not found.\n" > + fprintf (stderr, "Error opening config file '%s': %s\n" Ok, the series now looked good so I ran tests -- and since the above line changed, T040-setup.1 failed. I started to do extra commit to fix that... ... but there is more on that. It used *not* be error if configuration file was not found, and now it is. I'd suggest that errno == ENOENT is checked separately: in that case those 2 original lines are printed: otherwise only the new line above (and not the one below). In that case there is also no need to modify that particular test. > "Try running 'notmuch setup' to create a configuration.\n", > - config->filename); > + config->filename, strerror(errno)); > + goto out; > + } > + } > + > + config_str = talloc_zero_array (config, char, config_bufsize); > + if (config_str == NULL) { > + fprintf (stderr, "Error reading '%s': Out of memory\n", config->filename); > + goto out; > + } > + > + while ((len = fread (config_str + config_len, 1, > + config_bufsize - config_len, fp)) > 0) { > + config_len += len; > + if (config_len == config_bufsize) { > + config_bufsize += BUF_SIZE; > + config_str = talloc_realloc (config, config_str, char, config_bufsize); > + if (config_str == NULL) { > + fprintf (stderr, "Error reading '%s': Failed to reallocate memory\n", > + config->filename); > + goto out; > + } > } > - } else { > - fprintf (stderr, "Error reading configuration file %s: %s\n", > - config->filename, error->message); > } > > + if (ferror (fp)) { > + fprintf (stderr, "Error reading '%s': I/O error\n", config->filename); > + goto out; > + } > + > + if (g_key_file_load_from_data (config->key_file, config_str, config_len, > + G_KEY_FILE_KEEP_COMMENTS, &error)) { > + ret = TRUE; > + goto out; > + } > + > + fprintf (stderr, "Error parsing config file '%s': %s\n", > + config->filename, error->message); > + > g_error_free (error); > > +out: > + if (fp) > + fclose(fp); > + > + if (config_str) > + talloc_free(config_str); > + > return ret; > } > > -- > 2.10.2