Re: v2 of insert tempfail series
On Tue, Nov 29 2016, David Bremnerwrote: > This incorporates Tomi's patch of > > id:1480367228-22183-1-git-send-email-tomi.oll...@iki.fi > > verbatim, to sort out conflicts. > > It fixes the issues I alread sent mail about, and puts back the --keep > tests for various error codes. > > The interdiff follows; most of it is Tomi's fault :). This series looks good to me -- also my part which I had a slight afterthought when I looked what brew install gdb printed out -- it suggested setting 'startup-with-shell off' which would have borken my version (on macOS), but http://apple.stackexchange.com/questions/246245/macos-sierra-gdb-not-codesigned informed that setting startup-with-shell has so far not been necessary (and it would be major pain everywhere if it were required; in addition to everything else using `exec-wrapper` also requires shell...) I.e. if it makes gdb use more robust in general that is good reason to use it. Last (and least) (i.e. I could not resist the templation): the code block below would be simpler in this format -- but perhaps what happends there is less understandable -- although now the where the expansion is done is more visible ;) +test_expect_code 75 "EX_TEMPFAIL when add_message returns $code" \ + "gdb --batch-silent --return-child-result \ + -ex 'set args insert < $gen_msg_filename' \ + -x index-file-$code.gdb notmuch" (all variable expansions are done here (in context of the outermost "gdb...") and not in the `eval` that is done by test_expect_code -- unless any of the variables expanded to yet another variable syntax ;) Tomi > > diff --git a/notmuch-insert.c b/notmuch-insert.c > index a152f15..bc96af0 100644 > --- a/notmuch-insert.c > +++ b/notmuch-insert.c > @@ -541,7 +541,7 @@ notmuch_insert_command (notmuch_config_t *config, int > argc, char *argv[]) > status = notmuch_database_open (notmuch_config_get_database_path > (config), > NOTMUCH_DATABASE_MODE_READ_WRITE, ); > if (status) > - return status_to_exit(status); > + return keep ? NOTMUCH_STATUS_SUCCESS : status_to_exit (status); > > notmuch_exit_if_unmatched_db_uuid (notmuch); > > @@ -578,5 +578,5 @@ notmuch_insert_command (notmuch_config_t *config, int > argc, char *argv[]) > notmuch_run_hook (db_path, "post-insert"); > } > > -return status ? status_to_exit(status) : EXIT_SUCCESS; > +return status_to_exit (status); > } > diff --git a/test/T070-insert.sh b/test/T070-insert.sh > index fd620e5..3e7d582 100755 > --- a/test/T070-insert.sh > +++ b/test/T070-insert.sh > @@ -206,14 +206,24 @@ gen_insert_msg > > for code in FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; > do > test_expect_code 1 "EXIT_FAILURE when add_message returns $code" \ > - "gdb --batch-silent --return-child-result -x index-file-$code.gdb \ > - --args notmuch insert < $gen_msg_filename" > + "gdb --batch-silent --return-child-result \ > + -ex \"set args insert < $gen_msg_filename\" \ > + -x index-file-$code.gdb notmuch" > +test_expect_code 0 "success exit with --keep when add_message returns > $code" \ > + "gdb --batch-silent --return-child-result \ > + -ex \"set args insert --keep < $gen_msg_filename\" \ > + -x index-file-$code.gdb notmuch" > done > > for code in OUT_OF_MEMORY XAPIAN_EXCEPTION ; do > test_expect_code 75 "EX_TEMPFAIL when add_message returns $code" \ > - "gdb --batch-silent --return-child-result -x index-file-$code.gdb \ > - --args notmuch insert < $gen_msg_filename" > + "gdb --batch-silent --return-child-result \ > + -ex \"set args insert < $gen_msg_filename\" \ > + -x index-file-$code.gdb notmuch" > +test_expect_code 0 "success exit with --keep when add_message returns > $code" \ > + "gdb --batch-silent --return-child-result \ > + -ex \"set args insert --keep < $gen_msg_filename\" \ > + -x index-file-$code.gdb notmuch" > done > > test_done ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v3 2/2] notmuch-config: replace config reading function
On Sun, Dec 04 2016, Ioan-Adrian Ratiuwrote: > 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. That was QUICK! I try to do a bit more through review this time ;) > > Signed-off-by: Ioan-Adrian Ratiu > --- > notmuch-config.c | 60 > > 1 file changed, 47 insertions(+), 13 deletions(-) > > diff --git a/notmuch-config.c b/notmuch-config.c > index bd52790..1ea897a 100644 > --- a/notmuch-config.c > +++ b/notmuch-config.c > @@ -205,33 +205,67 @@ 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 200 // test line (use debug code or strace(1) to > verify) (actually I thought the above line to be used while testing the code before submission, and delete before that... ;) > +#define BUF_SIZE 4096 > +char *config_str; Hmm for reasons seen below, better do char *config_str = NULL (i.e. not like my previous suggestion) although not necessary it is a bit safer for future editors to mess with -- IMO we should use a bit more RAII way and declare the variable where it is first used (c99-supported feature), but that is not current status quo (or someone(tm) may know better why that would not be desired...) > +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, )) > - 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; > + return TRUE; > } else { > - fprintf (stderr, "Configuration file %s not found.\n" > + fprintf (stderr, "Error opening config file '%s': %s\n" >"Try running 'notmuch setup' to create a configuration.\n", > - config->filename); > + config->filename, strerror(errno)); > + return FALSE; > } > -} else { > - fprintf (stderr, "Error reading configuration file %s: %s\n", > - config->filename, error->message); > } > > +config_str = talloc_zero_array (config, char, config_bufsize); > +if (config_str == NULL) { > + fprintf (stderr, "Error reading '%s': Out of memory\n", > config->filename); close fp -- or better, set `ret` and goto out; (or if ret is (first declared, and) initialized to FALSE, then no need to set). > + return FALSE; > +} > + > +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); ditto > + return FALSE; > + } > + } > +} > + > +if (ferror (fp)) { > + fprintf (stderr, "Error reading '%s': I/O error\n", config->filename); ditto > + return FALSE; > +} > + > +fclose(fp); remove above... > + > +if (g_key_file_load_from_data (config->key_file, config_str, > strlen(config_str), here instead of strlen(), use config_len -- strlen() will stop at first embedded '\0' in data. > +G_KEY_FILE_KEEP_COMMENTS, )) > + return TRUE; set `ret` to TRUE, goto out; > + > +fprintf (stderr, "Error parsing config file '%s': %s\n", > + config->filename, error->message); > + > g_error_free (error); out: close(fp); if (config_str) talloc_free(config_str); return ret; >
[PATCH v3 1/2] cli: abstract config file reading to a separate function
From: Jani NikulaSimplify and fix the coding style while at it. --- notmuch-config.c | 65 ++-- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/notmuch-config.c b/notmuch-config.c index e5d42a0..bd52790 100644 --- a/notmuch-config.c +++ b/notmuch-config.c @@ -202,6 +202,38 @@ get_username_from_passwd_file (void *ctx) return name; } +static notmuch_bool_t +get_config_from_file (notmuch_config_t *config, notmuch_bool_t create_new) +{ +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, )) + return TRUE; + +if (error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT) { + /* 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; + } else { + fprintf (stderr, "Configuration file %s not found.\n" +"Try running 'notmuch setup' to create a configuration.\n", +config->filename); + } +} else { + fprintf (stderr, "Error reading configuration file %s: %s\n", +config->filename, error->message); +} + +g_error_free (error); + +return ret; +} + /* Open the named notmuch configuration file. If the filename is NULL, * the value of the environment variable $NOTMUCH_CONFIG will be used. * If $NOTMUCH_CONFIG is unset, the default configuration file @@ -289,36 +321,9 @@ notmuch_config_open (void *ctx, config->search_exclude_tags_length = 0; config->crypto_gpg_path = NULL; -if (! g_key_file_load_from_file (config->key_file, -config->filename, -G_KEY_FILE_KEEP_COMMENTS, -)) -{ - if (error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT) { - /* 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) { - g_error_free (error); - config->is_new = TRUE; - } else { - fprintf (stderr, "Configuration file %s not found.\n" -"Try running 'notmuch setup' to create a configuration.\n", -config->filename); - talloc_free (config); - g_error_free (error); - return NULL; - } - } - else - { - fprintf (stderr, "Error reading configuration file %s: %s\n", -config->filename, error->message); - talloc_free (config); - g_error_free (error); - return NULL; - } +if (! get_config_from_file (config, create_new)) { + talloc_free (config); + return NULL; } /* Whenever we know of configuration sections that don't appear in -- 2.10.2 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v3 2/2] notmuch-config: replace config reading function
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 | 60 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/notmuch-config.c b/notmuch-config.c index bd52790..1ea897a 100644 --- a/notmuch-config.c +++ b/notmuch-config.c @@ -205,33 +205,67 @@ 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 200 // test line (use debug code or strace(1) to verify) +#define BUF_SIZE 4096 +char *config_str; +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, )) - 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; + return TRUE; } else { - fprintf (stderr, "Configuration file %s not found.\n" + fprintf (stderr, "Error opening config file '%s': %s\n" "Try running 'notmuch setup' to create a configuration.\n", -config->filename); +config->filename, strerror(errno)); + return FALSE; } -} else { - fprintf (stderr, "Error reading configuration file %s: %s\n", -config->filename, error->message); } +config_str = talloc_zero_array (config, char, config_bufsize); +if (config_str == NULL) { + fprintf (stderr, "Error reading '%s': Out of memory\n", config->filename); + return FALSE; +} + +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); + return FALSE; + } + } +} + +if (ferror (fp)) { + fprintf (stderr, "Error reading '%s': I/O error\n", config->filename); + return FALSE; +} + +fclose(fp); + +if (g_key_file_load_from_data (config->key_file, config_str, strlen(config_str), + G_KEY_FILE_KEEP_COMMENTS, )) + return TRUE; + +fprintf (stderr, "Error parsing config file '%s': %s\n", +config->filename, error->message); + g_error_free (error); -return ret; +return FALSE; } /* Open the named notmuch configuration file. If the filename is NULL, -- 2.10.2 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v3 0/2] Refactor config reading to support non-regular files
Changes since v2 (based on Tomi's feedback): * Rewrote config reading loop to use fread instead of fgets (behaves the same but the code looks much better now). Ioan-Adrian Ratiu (1): notmuch-config: replace config reading function Jani Nikula (1): cli: abstract config file reading to a separate function notmuch-config.c | 99 +++- 1 file changed, 69 insertions(+), 30 deletions(-) -- 2.10.2 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 2/2] notmuch-config: replace config reading function
On Sun, Nov 06 2016, Ioan-Adrian Ratiuwrote: > 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 I'd suggest the reading loop be something like (error handling (mostly) missing, untested): // #define BUF_SIZE 200 // test line (use debug code or strace(1) to verify) #define BUF_SIZE 4096 char *config_str; int config_len = 0; int config_bufsize = BUF_SIZE; size_t len; ... FILE *fp = fopen (config->filename, "r"); ... config_str = talloc_zero_array (config, char, config_bufsize); ... 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 (ferror (fp)) { fprintf (stderr, "Error reading '%s': I/O error\n", config->filename); fclose (fp); return FALSE; } fclose (fp) if (g_key_file_load_from_data (config->key_file, config_str, config_len, G_KEY_FILE_KEEP_COMMENTS, )) return TRUE; > --- > notmuch-config.c | 53 + > 1 file changed, 41 insertions(+), 12 deletions(-) > > diff --git a/notmuch-config.c b/notmuch-config.c > index bd52790..569cf0b 100644 > --- a/notmuch-config.c > +++ b/notmuch-config.c > @@ -205,33 +205,62 @@ 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 buffer[BUF_SIZE]; > +size_t content_size = 1; // includes NULL > +char *config_str = NULL; > 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, )) > - 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; > + return TRUE; > } else { > - fprintf (stderr, "Configuration file %s not found.\n" > + fprintf (stderr, "Error opening config file '%s': %s\n" >"Try running 'notmuch setup' to create a configuration.\n", > + config->filename, strerror(errno)); > + return FALSE; > + } > +} > + > +config_str = talloc_zero_array (config, char, BUF_SIZE); > +if (config_str == NULL) { > + fprintf (stderr, "Error reading '%s': Out of memory\n", > config->filename); > + return FALSE; > +} > + > +while (fgets (buffer, BUF_SIZE, fp)) { > + content_size += strlen(buffer); > + config_str = talloc_realloc(config, config_str, char, content_size); > + if (config_str == NULL) { > + fprintf (stderr, "Error reading '%s': Failed to reallocate > memory\n", >config->filename); > + return FALSE; > } > -} else { > - fprintf (stderr, "Error reading configuration file %s: %s\n", > - config->filename, error->message); > + strcat (config_str, buffer); > +} > + > +if (ferror (fp)) { > + fprintf (stderr, "Error reading '%s': I/O error\n", config->filename); > + return FALSE; > } > > +fclose(fp); > + > +if (g_key_file_load_from_data (config->key_file, config_str, > strlen(config_str), > +G_KEY_FILE_KEEP_COMMENTS, )) > + return TRUE; > + > +fprintf (stderr, "Error parsing config file '%s': %s\n", > + config->filename, error->message); > + > g_error_free (error); > > -return ret; > +return FALSE; > } > > /* Open the named notmuch configuration file. If the filename is NULL, > -- >