Both the library and the CLI use a fair amount of static variables. These are an obvious blocker for any kind of thread-safety, even the limited goal of concurrently opening two databases for reading in the same process. This series cleans up some of the low-hanging fruit.
In principle this could be done without depending on C++11 features, but these features should be available since gcc 4.8.1, and this localized usage is easy to replace if it turns out to be problematic for portability. --- lib/Makefile.local | 3 ++- lib/index.cc | 43 ++++++++++++++++++++++--------------------- lib/init.cc | 21 +++++++++++++++++++++ lib/message-file.c | 6 +----- lib/notmuch-private.h | 7 +++++++ lib/open.cc | 24 +++--------------------- 6 files changed, 56 insertions(+), 48 deletions(-) create mode 100644 lib/init.cc diff --git a/lib/Makefile.local b/lib/Makefile.local index 01cbb3f2..e2d4b91d 100644 --- a/lib/Makefile.local +++ b/lib/Makefile.local @@ -62,7 +62,8 @@ libnotmuch_cxx_srcs = \ $(dir)/thread-fp.cc \ $(dir)/features.cc \ $(dir)/prefix.cc \ - $(dir)/open.cc + $(dir)/open.cc \ + $(dir)/init.cc libnotmuch_modules := $(libnotmuch_c_srcs:.c=.o) $(libnotmuch_cxx_srcs:.cc=.o) diff --git a/lib/index.cc b/lib/index.cc index 55c8372e..728bfb22 100644 --- a/lib/index.cc +++ b/lib/index.cc @@ -148,8 +148,6 @@ notmuch_filter_discard_non_term_class_init (NotmuchFilterDiscardNonTermClass *kl GObjectClass *object_class = G_OBJECT_CLASS (klass); GMimeFilterClass *filter_class = GMIME_FILTER_CLASS (klass); - parent_class = (GMimeFilterClass *) g_type_class_ref (GMIME_TYPE_FILTER); - object_class->finalize = notmuch_filter_discard_non_term_finalize; filter_class->copy = filter_copy; @@ -240,30 +238,33 @@ filter_reset (GMimeFilter *gmime_filter) * * Returns: a new #NotmuchFilterDiscardNonTerm filter. **/ +static GType type = 0; + +static const GTypeInfo info = { + .class_size = sizeof (NotmuchFilterDiscardNonTermClass), + .base_init = NULL, + .base_finalize = NULL, + .class_init = (GClassInitFunc) notmuch_filter_discard_non_term_class_init, + .class_finalize = NULL, + .class_data = NULL, + .instance_size = sizeof (NotmuchFilterDiscardNonTerm), + .n_preallocs = 0, + .instance_init = NULL, + .value_table = NULL, +}; + +void +_notmuch_filter_init () { + type = g_type_register_static (GMIME_TYPE_FILTER, "NotmuchFilterDiscardNonTerm", &info, + (GTypeFlags) 0); + parent_class = (GMimeFilterClass *) g_type_class_ref (GMIME_TYPE_FILTER); +} + static GMimeFilter * notmuch_filter_discard_non_term_new (GMimeContentType *content_type) { - static GType type = 0; NotmuchFilterDiscardNonTerm *filter; - if (! type) { - static const GTypeInfo info = { - .class_size = sizeof (NotmuchFilterDiscardNonTermClass), - .base_init = NULL, - .base_finalize = NULL, - .class_init = (GClassInitFunc) notmuch_filter_discard_non_term_class_init, - .class_finalize = NULL, - .class_data = NULL, - .instance_size = sizeof (NotmuchFilterDiscardNonTerm), - .n_preallocs = 0, - .instance_init = NULL, - .value_table = NULL, - }; - - type = g_type_register_static (GMIME_TYPE_FILTER, "NotmuchFilterDiscardNonTerm", &info, - (GTypeFlags) 0); - } - filter = (NotmuchFilterDiscardNonTerm *) g_object_new (type, NULL); filter->content_type = content_type; filter->state = 0; diff --git a/lib/init.cc b/lib/init.cc new file mode 100644 index 00000000..cf29200f --- /dev/null +++ b/lib/init.cc @@ -0,0 +1,21 @@ +#include "notmuch-private.h" + +#include <mutex> + +static void do_init () +{ + /* Initialize the GLib type system and threads */ +#if ! GLIB_CHECK_VERSION (2, 35, 1) + g_type_init (); +#endif + + g_mime_init (); + _notmuch_filter_init (); +} + +void +_notmuch_init () +{ + static std::once_flag initialized; + std::call_once (initialized, do_init); +} diff --git a/lib/message-file.c b/lib/message-file.c index 9e9b387f..647ccf3a 100644 --- a/lib/message-file.c +++ b/lib/message-file.c @@ -141,7 +141,6 @@ _notmuch_message_file_parse (notmuch_message_file_t *message) { GMimeParser *parser; notmuch_status_t status = NOTMUCH_STATUS_SUCCESS; - static int initialized = 0; bool is_mbox; if (message->message) @@ -149,10 +148,7 @@ _notmuch_message_file_parse (notmuch_message_file_t *message) is_mbox = _is_mbox (message->stream); - if (! initialized) { - g_mime_init (); - initialized = 1; - } + _notmuch_init (); message->headers = g_hash_table_new_full (strcase_hash, strcase_equal, free, g_free); diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h index 10b1b024..093c29b1 100644 --- a/lib/notmuch-private.h +++ b/lib/notmuch-private.h @@ -469,11 +469,18 @@ _notmuch_database_link_message_to_parents (notmuch_database_t *notmuch, const char **thread_id); /* index.cc */ +void +_notmuch_filter_init (); + notmuch_status_t _notmuch_message_index_file (notmuch_message_t *message, notmuch_indexopts_t *indexopts, notmuch_message_file_t *message_file); +/* init.cc */ +void +_notmuch_init (); + /* messages.c */ typedef struct _notmuch_message_node { diff --git a/lib/open.cc b/lib/open.cc index 1e9c86fe..84b2d6b1 100644 --- a/lib/open.cc +++ b/lib/open.cc @@ -307,24 +307,6 @@ _set_database_path (notmuch_database_t *notmuch, _notmuch_config_cache (notmuch, NOTMUCH_CONFIG_DATABASE_PATH, path); } -static void -_init_libs () -{ - - static int initialized = 0; - - /* Initialize the GLib type system and threads */ -#if ! GLIB_CHECK_VERSION (2, 35, 1) - g_type_init (); -#endif - - /* Initialize gmime */ - if (! initialized) { - g_mime_init (); - initialized = 1; - } -} - static void _load_database_state (notmuch_database_t *notmuch) { @@ -498,7 +480,7 @@ notmuch_database_open_with_config (const char *database_path, GKeyFile *key_file = NULL; bool split = false; - _init_libs (); + _notmuch_init (); notmuch = _alloc_notmuch (); if (! notmuch) { @@ -595,7 +577,7 @@ notmuch_database_create_with_config (const char *database_path, int err; bool split = false; - _init_libs (); + _notmuch_init (); notmuch = _alloc_notmuch (); if (! notmuch) { @@ -791,7 +773,7 @@ notmuch_database_load_config (const char *database_path, GKeyFile *key_file = NULL; bool split = false; - _init_libs (); + _notmuch_init (); notmuch = _alloc_notmuch (); if (! notmuch) { -- 2.30.2
Work towards the goal of concurrent access to different Xapian databases from the same process. --- lib/add-message.cc | 9 +++------ lib/database-private.h | 4 ++++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/add-message.cc b/lib/add-message.cc index 0c34d318..d4a00b17 100644 --- a/lib/add-message.cc +++ b/lib/add-message.cc @@ -40,17 +40,14 @@ parse_references (void *ctx, static const char * _notmuch_database_generate_thread_id (notmuch_database_t *notmuch) { - /* 16 bytes (+ terminator) for hexadecimal representation of - * a 64-bit integer. */ - static char thread_id[17]; notmuch->last_thread_id++; - sprintf (thread_id, "%016" PRIx64, notmuch->last_thread_id); + sprintf (notmuch->thread_id_str, "%016" PRIx64, notmuch->last_thread_id); - notmuch->writable_xapian_db->set_metadata ("last_thread_id", thread_id); + notmuch->writable_xapian_db->set_metadata ("last_thread_id", notmuch->thread_id_str); - return thread_id; + return notmuch->thread_id_str; } static char * diff --git a/lib/database-private.h b/lib/database-private.h index 0d12ec1e..1a73dacc 100644 --- a/lib/database-private.h +++ b/lib/database-private.h @@ -206,6 +206,10 @@ struct _notmuch_database { enum _notmuch_features features; unsigned int last_doc_id; + + /* 16 bytes (+ terminator) for hexadecimal representation of + * a 64-bit integer. */ + char thread_id_str[17]; uint64_t last_thread_id; /* error reporting; this value persists only until the -- 2.30.2
This table is intended to be immutable --- lib/message.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/message.cc b/lib/message.cc index 42d56acb..7af6ab82 100644 --- a/lib/message.cc +++ b/lib/message.cc @@ -68,7 +68,7 @@ struct maildir_flag_tag { }; /* ASCII ordered table of Maildir flags and associated tags */ -static struct maildir_flag_tag flag2tag[] = { +static const struct maildir_flag_tag flag2tag[] = { { 'D', "draft", false }, { 'F', "flagged", false }, { 'P', "passed", false }, -- 2.30.2
Initially make sure gmime_filter_reply initialization is thread-safe (assuming notmuch_client_init is only called once). For tidyness, also put talloc initialization in the new function. --- Makefile.local | 1 + gmime-filter-reply.c | 42 +++++++++++++++++++++--------------------- gmime-filter-reply.h | 2 ++ notmuch-client.h | 4 ++++ notmuch.c | 7 +------ 5 files changed, 29 insertions(+), 27 deletions(-) diff --git a/Makefile.local b/Makefile.local index bbb8f0b6..e12b94cd 100644 --- a/Makefile.local +++ b/Makefile.local @@ -231,6 +231,7 @@ notmuch_client_srcs = \ gmime-filter-reply.c \ hooks.c \ notmuch.c \ + notmuch-client-init.c \ notmuch-compact.c \ notmuch-config.c \ notmuch-count.c \ diff --git a/gmime-filter-reply.c b/gmime-filter-reply.c index 2b067669..35349cc8 100644 --- a/gmime-filter-reply.c +++ b/gmime-filter-reply.c @@ -43,29 +43,31 @@ static void filter_reset (GMimeFilter *filter); static GMimeFilterClass *parent_class = NULL; +static GType type = 0; +static const GTypeInfo info = { + .class_size = sizeof (GMimeFilterReplyClass), + .base_init = NULL, + .base_finalize = NULL, + .class_init = (GClassInitFunc) g_mime_filter_reply_class_init, + .class_finalize = NULL, + .class_data = NULL, + .instance_size = sizeof (GMimeFilterReply), + .n_preallocs = 0, + .instance_init = (GInstanceInitFunc) g_mime_filter_reply_init, + .value_table = NULL, +}; + + +void +g_mime_filter_reply_module_init (void) +{ + type = g_type_register_static (GMIME_TYPE_FILTER, "GMimeFilterReply", &info, (GTypeFlags) 0); + parent_class = (GMimeFilterClass *) g_type_class_ref (GMIME_TYPE_FILTER); +} GType g_mime_filter_reply_get_type (void) { - static GType type = 0; - - if (! type) { - static const GTypeInfo info = { - .class_size = sizeof (GMimeFilterReplyClass), - .base_init = NULL, - .base_finalize = NULL, - .class_init = (GClassInitFunc) g_mime_filter_reply_class_init, - .class_finalize = NULL, - .class_data = NULL, - .instance_size = sizeof (GMimeFilterReply), - .n_preallocs = 0, - .instance_init = (GInstanceInitFunc) g_mime_filter_reply_init, - .value_table = NULL, - }; - - type = g_type_register_static (GMIME_TYPE_FILTER, "GMimeFilterReply", &info, (GTypeFlags) 0); - } - return type; } @@ -76,8 +78,6 @@ g_mime_filter_reply_class_init (GMimeFilterReplyClass *klass, unused (void *clas GObjectClass *object_class = G_OBJECT_CLASS (klass); GMimeFilterClass *filter_class = GMIME_FILTER_CLASS (klass); - parent_class = (GMimeFilterClass *) g_type_class_ref (GMIME_TYPE_FILTER); - object_class->finalize = g_mime_filter_reply_finalize; filter_class->copy = filter_copy; diff --git a/gmime-filter-reply.h b/gmime-filter-reply.h index 7cdefcd1..988fe2d6 100644 --- a/gmime-filter-reply.h +++ b/gmime-filter-reply.h @@ -21,6 +21,8 @@ #include <gmime/gmime-filter.h> +void g_mime_filter_reply_module_init (void); + G_BEGIN_DECLS #define GMIME_TYPE_FILTER_REPLY (g_mime_filter_reply_get_type ()) diff --git a/notmuch-client.h b/notmuch-client.h index 270553ad..8227fea4 100644 --- a/notmuch-client.h +++ b/notmuch-client.h @@ -250,6 +250,10 @@ json_quote_chararray (const void *ctx, const char *str, const size_t len); char * json_quote_str (const void *ctx, const char *str); +/* notmuch-client-init.c */ + +void notmuch_client_init (void); + /* notmuch-config.c */ typedef enum { diff --git a/notmuch.c b/notmuch.c index 2429999c..9ca3a4be 100644 --- a/notmuch.c +++ b/notmuch.c @@ -464,15 +464,10 @@ main (int argc, char *argv[]) { } }; - talloc_enable_null_tracking (); + notmuch_client_init (); local = talloc_new (NULL); - g_mime_init (); -#if ! GLIB_CHECK_VERSION (2, 35, 1) - g_type_init (); -#endif - /* Globally default to the current output format version. */ notmuch_format_version = NOTMUCH_FORMAT_CUR; -- 2.30.2
Let the compiler help us catch bugs. --- notmuch-config.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/notmuch-config.c b/notmuch-config.c index d9390c4d..3430a3d3 100644 --- a/notmuch-config.c +++ b/notmuch-config.c @@ -32,7 +32,7 @@ static const char toplevel_config_comment[] = "\n" " For more information about notmuch, see https://notmuchmail.org"; -struct config_group { +static const struct config_group { const char *group_name; const char *comment; } group_comment_table [] = { @@ -512,14 +512,14 @@ typedef struct config_key { bool (*validate)(const char *); } config_key_info_t; -static struct config_key +static const struct config_key config_key_table[] = { { "index.decrypt", false, NULL }, { "index.header.", true, validate_field_name }, { "query.", true, NULL }, }; -static config_key_info_t * +static const config_key_info_t * _config_key_info (const char *item) { for (size_t i = 0; i < ARRAY_SIZE (config_key_table); i++) { @@ -583,7 +583,7 @@ notmuch_config_command_set (notmuch_database_t *notmuch, int argc, char *argv[]) { char *group, *key; - config_key_info_t *key_info; + const config_key_info_t *key_info; notmuch_conffile_t *config; bool update_database = false; int opt_index, ret; -- 2.30.2
This is both a bit clearer and avoids the possibility of modification. --- notmuch-insert.c | 2 +- notmuch-tag.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/notmuch-insert.c b/notmuch-insert.c index 00c00468..e3d87e4a 100644 --- a/notmuch-insert.c +++ b/notmuch-insert.c @@ -34,7 +34,7 @@ static volatile sig_atomic_t interrupted; static void handle_sigint (unused (int sig)) { - static char msg[] = "Stopping... \n"; + static const char msg[] = "Stopping... \n"; /* This write is "opportunistic", so it's okay to ignore the * result. It is not required for correctness, and if it does diff --git a/notmuch-tag.c b/notmuch-tag.c index 667a75d6..de428c8e 100644 --- a/notmuch-tag.c +++ b/notmuch-tag.c @@ -27,7 +27,7 @@ static volatile sig_atomic_t interrupted; static void handle_sigint (unused (int sig)) { - static char msg[] = "Stopping... \n"; + static const char msg[] = "Stopping... \n"; /* This write is "opportunistic", so it's okay to ignore the * result. It is not required for correctness, and if it does -- 2.30.2
Let the compiler enforce the immutability. --- notmuch.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/notmuch.c b/notmuch.c index 9ca3a4be..d0a94fc2 100644 --- a/notmuch.c +++ b/notmuch.c @@ -139,7 +139,7 @@ notmuch_process_shared_indexing_options (notmuch_database_t *notmuch) } -static command_t commands[] = { +static const command_t commands[] = { { NULL, notmuch_command, NOTMUCH_COMMAND_CONFIG_CREATE | NOTMUCH_COMMAND_CONFIG_LOAD, "Notmuch main command." }, { "setup", notmuch_setup_command, NOTMUCH_COMMAND_CONFIG_CREATE | NOTMUCH_COMMAND_CONFIG_LOAD, @@ -189,7 +189,7 @@ typedef struct help_topic { const char *summary; } help_topic_t; -static help_topic_t help_topics[] = { +static const help_topic_t help_topics[] = { { "search-terms", "Common search term syntax." }, { "hooks", @@ -198,7 +198,7 @@ static help_topic_t help_topics[] = { "Message property conventions and documentation." }, }; -static command_t * +static const command_t * find_command (const char *name) { size_t i; @@ -216,8 +216,8 @@ int notmuch_format_version; static void usage (FILE *out) { - command_t *command; - help_topic_t *topic; + const command_t *command; + const help_topic_t *topic; unsigned int i; fprintf (out, @@ -308,8 +308,8 @@ exec_man (const char *page) static int _help_for (const char *topic_name) { - command_t *command; - help_topic_t *topic; + const command_t *command; + const help_topic_t *topic; unsigned int i; if (! topic_name) { @@ -452,7 +452,7 @@ main (int argc, char *argv[]) void *local; char *talloc_report; const char *command_name = NULL; - command_t *command; + const command_t *command; const char *config_file_name = NULL; notmuch_database_t *notmuch = NULL; int opt_index; -- 2.30.2
On Thu, May 13 2021, David Bremner wrote: > This is both a bit clearer and avoids the possibility of modification. > --- > notmuch-insert.c | 2 +- > notmuch-tag.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/notmuch-insert.c b/notmuch-insert.c > index 00c00468..e3d87e4a 100644 > --- a/notmuch-insert.c > +++ b/notmuch-insert.c > @@ -34,7 +34,7 @@ static volatile sig_atomic_t interrupted; > static void > handle_sigint (unused (int sig)) > { > - static char msg[] = "Stopping... \n"; > + static const char msg[] = "Stopping... \n"; If compiler is smart enough, it can even save copy from read-only area of the binary to read-write runtime memory area. If not that smart it does the copy. In my projects I've been using the following macro: #define WriteCS(fd, str) write((fd), (str), sizeof(str) - 1) but I don't know if that behaved any better (if we cared)... But, in addition to these two identical copies of handle_sigint() also notmuch-new.c and notmuch-reindex.c defines anoter 2 identical copies of handle_sigint()... I did not see that those two were also modified in this series... Other that these, the series looked ok... Tomi > > /* This write is "opportunistic", so it's okay to ignore the > * result. It is not required for correctness, and if it does > diff --git a/notmuch-tag.c b/notmuch-tag.c > index 667a75d6..de428c8e 100644 > --- a/notmuch-tag.c > +++ b/notmuch-tag.c > @@ -27,7 +27,7 @@ static volatile sig_atomic_t interrupted; > static void > handle_sigint (unused (int sig)) > { > - static char msg[] = "Stopping... \n"; > + static const char msg[] = "Stopping... \n"; > > /* This write is "opportunistic", so it's okay to ignore the > * result. It is not required for correctness, and if it does > -- > 2.30.2 > _______________________________________________ > notmuch mailing list -- notmuch@notmuchmail.org > To unsubscribe send an email to notmuch-leave@notmuchmail.org
Tomi Ollila <tomi.ollila@iki.fi> writes: > > In my projects I've been using the following macro: > #define WriteCS(fd, str) write((fd), (str), sizeof(str) - 1) > but I don't know if that behaved any better (if we cared)... I'm not sure. The sizeof here is actually slightly treacherous (iiuc, making things const char * will blow up the world) so I prefer it out in the open. > But, in addition to these two identical copies of handle_sigint() > also notmuch-new.c and notmuch-reindex.c defines anoter 2 identical > copies of handle_sigint()... > > I did not see that those two were also modified in this series... Good catch! I did the same for those two and pushed the modified series.
On Fri, May 14 2021, David Bremner wrote: > Tomi Ollila <tomi.ollila@iki.fi> writes: > >> >> In my projects I've been using the following macro: >> #define WriteCS(fd, str) write((fd), (str), sizeof(str) - 1) >> but I don't know if that behaved any better (if we cared)... > > I'm not sure. The sizeof here is actually slightly treacherous (iiuc, > making things const char * will blow up the world) so I prefer it out in > the open. Right, const char * would make sizeof 8 (or 4). (I just fixed bug somewhere where first 4 chars of strings were compared for equality ;/). The WriteCS meant (Const String), perhaps LS (Literal String) would be better. (and write((fd), "" str "", sizeof(str) - 1)). ... have to check if this works, and start using if it works as expected! Tomi > >> But, in addition to these two identical copies of handle_sigint() >> also notmuch-new.c and notmuch-reindex.c defines anoter 2 identical >> copies of handle_sigint()... >> >> I did not see that those two were also modified in this series... > > Good catch! I did the same for those two and pushed the modified series.