unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* Library logging overhaul, round 6
@ 2015-03-27 22:11 David Bremner
  2015-03-27 22:11 ` [Patch v6 1/8] test: Add two tests for error output from notmuch_database_open David Bremner
                   ` (7 more replies)
  0 siblings, 8 replies; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:11 UTC (permalink / raw)
  To: notmuch

This obsoletes

     id:1427203451-1540-1-git-send-email-david@tethera.net

I think this addresses all of Tomi's comments, except the use of
status_cb to print error output from notmuch_database_compact.
I added some tests for notmuch_database_create error output.

diff --git a/lib/database.cc b/lib/database.cc
index 85054df..9f66b5f 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -625,7 +625,18 @@ parse_references (void *ctx,
 notmuch_status_t
 notmuch_database_create (const char *path, notmuch_database_t **database)
 {
-    return notmuch_database_create_verbose (path, database, NULL);
+    char *status_string = NULL;
+    notmuch_status_t status;
+
+    status = notmuch_database_create_verbose (path, database,
+					      &status_string);
+
+    if (status_string) {
+	fputs (status_string, stderr);
+	free (status_string);
+    }
+
+    return status;
 }
 
 notmuch_status_t
@@ -694,8 +705,9 @@ notmuch_database_create_verbose (const char *path,
     if (notmuch_path)
 	talloc_free (notmuch_path);
 
-    if (message)
+    if (status_string && message)
 	*status_string = message;
+
     if (database)
 	*database = notmuch;
     else
@@ -799,10 +811,13 @@ notmuch_database_open (const char *path,
     char *status_string = NULL;
     notmuch_status_t status;
 
-    status = notmuch_database_open_verbose(path, mode, database,
+    status = notmuch_database_open_verbose (path, mode, database,
 					   &status_string);
 
-    if (status_string) fputs(status_string, stderr);
+    if (status_string) {
+	fputs (status_string, stderr);
+	free (status_string);
+    }
 
     return status;
 }
diff --git a/notmuch-new.c b/notmuch-new.c
index 93b70bf..e6c283e 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -988,7 +988,10 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	char *status_string = NULL;
 	if (notmuch_database_open_verbose (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
 					   &notmuch, &status_string)) {
-	    if (status_string) fputs (status_string, stderr);
+	    if (status_string) {
+		fputs (status_string, stderr);
+		free (status_string);
+	    }
 
 	    return EXIT_FAILURE;
 	}
diff --git a/notmuch-search.c b/notmuch-search.c
index d012af3..b81ac01 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -574,7 +574,12 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar
     if (notmuch_database_open_verbose (
 	    notmuch_config_get_database_path (config),
 	    NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch, &status_string)) {
-	if (status_string) fputs (status_string, stderr);
+
+	if (status_string) {
+	    fputs (status_string, stderr);
+	    free (status_string);
+	}
+
 	return EXIT_FAILURE;
     }
 
diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index ec7552a..67a5e8d 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -3,13 +3,13 @@ test_description="error reporting for library"
 
 . ./test-lib.sh
 
-backup_database (){
+backup_database () {
     rm -rf notmuch-dir-backup
-    cp -a ${MAIL_DIR}/.notmuch notmuch-dir-backup
+    cp -pR ${MAIL_DIR}/.notmuch notmuch-dir-backup
 }
-restore_database (){
+restore_database () {
     rm -rf ${MAIL_DIR}/.notmuch
-    cp -a notmuch-dir-backup ${MAIL_DIR}/.notmuch
+    cp -pR notmuch-dir-backup ${MAIL_DIR}/.notmuch
 }
 
 
@@ -18,7 +18,7 @@ add_email_corpus
 test_expect_success "building database" "NOTMUCH_NEW"
 
 test_begin_subtest "Open null pointer"
-test_C <<EOF
+test_C <<'EOF'
 #include <stdio.h>
 #include <notmuch.h>
 int main (int argc, char** argv)
@@ -28,7 +28,7 @@ int main (int argc, char** argv)
     stat = notmuch_database_open (NULL, 0, 0);
 }
 EOF
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 Error: Cannot open a database for a NULL path.
@@ -36,7 +36,7 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "Open nonexistent database"
-test_C <<EOF
+test_C <<'EOF'
 #include <stdio.h>
 #include <notmuch.h>
 int main (int argc, char** argv)
@@ -46,15 +46,50 @@ int main (int argc, char** argv)
     stat = notmuch_database_open ("/nonexistent/foo", 0, 0);
 }
 EOF
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 Error opening database at /nonexistent/foo/.notmuch: No such file or directory
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "create NULL path"
+test_C <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+    notmuch_status_t stat;
+    stat = notmuch_database_create (NULL, NULL);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Error: Cannot create a database for a NULL path.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "Create database in non-existant directory"
+test_C <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+    notmuch_database_t *db;
+    notmuch_status_t stat;
+    stat = notmuch_database_create ("/nonexistent/foo", &db);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Error: Cannot create database at /nonexistent/foo: No such file or directory.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "Write to read-only database"
-test_C ${MAIL_DIR} <<EOF
+test_C ${MAIL_DIR} <<'EOF'
 #include <stdio.h>
 #include <notmuch.h>
 int main (int argc, char** argv)
@@ -71,7 +106,7 @@ int main (int argc, char** argv)
 
 }
 EOF
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 Cannot write to a read-only database.
@@ -79,7 +114,7 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "Add non-existent file"
-test_C ${MAIL_DIR} <<EOF
+test_C ${MAIL_DIR} <<'EOF'
 #include <stdio.h>
 #include <notmuch.h>
 int main (int argc, char** argv)
@@ -96,7 +131,7 @@ int main (int argc, char** argv)
 
 }
 EOF
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 Error opening /nonexistent: No such file or directory
@@ -104,7 +139,7 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "compact, overwriting existing backup"
-test_C ${MAIL_DIR} <<EOF
+test_C ${MAIL_DIR} <<'EOF'
 #include <stdio.h>
 #include <notmuch.h>
 static void
@@ -119,7 +154,7 @@ int main (int argc, char** argv)
    stat = notmuch_database_compact (argv[1], argv[1], status_cb, NULL);
 }
 EOF
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 Path already exists: CWD/mail
 
@@ -127,7 +162,7 @@ Path already exists: CWD/mail
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
-cat <<EOF > head.c
+cat <<'EOF' > c_head
 #include <stdio.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -151,7 +186,7 @@ int main (int argc, char** argv)
    if (fd < 0)
        fprintf (stderr, "error opening %s\n");
 EOF
-cat <<EOF > tail.c
+cat <<'EOF' > c_tail
    if (stat) {
        const char *stat_str = notmuch_database_status_string (db);
        if (stat_str)
@@ -163,14 +198,14 @@ EOF
 
 backup_database
 test_begin_subtest "Xapian exception finding message"
-cat head.c - tail.c <<EOF | test_C ${MAIL_DIR}
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
    {
        notmuch_message_t *message = NULL;
        stat = notmuch_database_find_message (db, "id:nonexistant", &message);
    }
 EOF
 sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 A Xapian exception occurred finding message
@@ -180,7 +215,7 @@ restore_database
 
 backup_database
 test_begin_subtest "Xapian exception getting tags"
-cat head.c - tail.c <<EOF | test_C ${MAIL_DIR}
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
    {
        notmuch_tags_t *tags = NULL;
        tags = notmuch_database_get_all_tags (db);
@@ -188,7 +223,7 @@ cat head.c - tail.c <<EOF | test_C ${MAIL_DIR}
    }
 EOF
 sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 A Xapian exception occurred getting tags
@@ -198,14 +233,14 @@ restore_database
 
 backup_database
 test_begin_subtest "Xapian exception creating directory"
-cat head.c - tail.c <<EOF | test_C ${MAIL_DIR}
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
    {
        notmuch_directory_t *directory = NULL;
        stat = notmuch_database_get_directory (db, "none/existing", &directory);
    }
 EOF
 sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 A Xapian exception occurred creating a directory
@@ -215,7 +250,7 @@ restore_database
 
 backup_database
 test_begin_subtest "Xapian exception searching messages"
-cat head.c - tail.c <<EOF | test_C ${MAIL_DIR}
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
    {
        notmuch_messages_t *messages = NULL;
        notmuch_query_t *query=notmuch_query_create (db, "*");
@@ -223,7 +258,7 @@ cat head.c - tail.c <<EOF | test_C ${MAIL_DIR}
    }
 EOF
 sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 A Xapian exception occurred performing query
@@ -234,7 +269,7 @@ restore_database
 
 backup_database
 test_begin_subtest "Xapian exception counting messages"
-cat head.c - tail.c <<EOF | test_C ${MAIL_DIR}
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
    {
        notmuch_query_t *query=notmuch_query_create (db, "id:87ocn0qh6d.fsf@yoom.home.cworth.org");
        int count = notmuch_query_count_messages (query);
@@ -242,7 +277,7 @@ cat head.c - tail.c <<EOF | test_C ${MAIL_DIR}
    }
 EOF
 sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
-cat <<EOF >EXPECTED
+cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
 A Xapian exception occurred performing query
diff --git a/test/symbol-test.cc b/test/symbol-test.cc
index 9f8eea7..d979f83 100644
--- a/test/symbol-test.cc
+++ b/test/symbol-test.cc
@@ -1,4 +1,5 @@
 #include <stdio.h>
+#include <stdlib.h>
 #include <xapian.h>
 #include <notmuch.h>
 
@@ -8,8 +9,10 @@ int main() {
   char *message = NULL;
 
   if (notmuch_database_open_verbose  ("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch, &message))
-      if (message) fputs (message, stderr);
-
+      if (message) {
+	  fputs (message, stderr);
+	  free (message);
+      }
 
   try {
     (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);
diff --git a/test/test-lib.sh b/test/test-lib.sh
index fdb84ea..486d1c4 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1172,7 +1172,7 @@ test_C () {
     echo "== stdout ==" > OUTPUT.stdout
     echo "== stderr ==" > OUTPUT.stderr
     ./${exec_file} "$@" 1>>OUTPUT.stdout 2>>OUTPUT.stderr
-    sed "s,$(pwd),CWD,"  OUTPUT.stdout OUTPUT.stderr > OUTPUT
+    sed "s,${PWD},CWD,g"  OUTPUT.stdout OUTPUT.stderr > OUTPUT
 }
 
 

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch v6 1/8] test: Add two tests for error output from notmuch_database_open
  2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
@ 2015-03-27 22:11 ` David Bremner
  2015-03-27 22:11 ` [Patch v6 2/8] test: add support for compiling and running C snippets David Bremner
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:11 UTC (permalink / raw)
  To: notmuch

This is arguably testing the same thing twice, but in the brave new
future where we don't use printf anymore, each subcommand will be
responsible for handling the output on it's own.
---
 test/T050-new.sh     | 7 +++++++
 test/T150-tagging.sh | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/test/T050-new.sh b/test/T050-new.sh
index 7119356..e6c3291 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -276,4 +276,11 @@ test_expect_code 1 "Invalid tags set exit code" \
 
 notmuch config set new.tags $OLDCONFIG
 
+
+test_begin_subtest "Xapian exception: read only files"
+chmod u-w  ${MAIL_DIR}/.notmuch/xapian/*.DB
+output=$(NOTMUCH_NEW 2>&1 | sed 's/: .*$//' )
+chmod u+w  ${MAIL_DIR}/.notmuch/xapian/*.DB
+test_expect_equal "$output" "A Xapian exception occurred opening database"
+
 test_done
diff --git a/test/T150-tagging.sh b/test/T150-tagging.sh
index 45471ac..4a2673d 100755
--- a/test/T150-tagging.sh
+++ b/test/T150-tagging.sh
@@ -261,4 +261,10 @@ test_expect_code 1 "Empty tag names" 'notmuch tag + One'
 
 test_expect_code 1 "Tag name beginning with -" 'notmuch tag +- One'
 
+test_begin_subtest "Xapian exception: read only files"
+chmod u-w  ${MAIL_DIR}/.notmuch/xapian/*.DB
+output=$(notmuch tag +something '*' 2>&1 | sed 's/: .*$//' )
+chmod u+w  ${MAIL_DIR}/.notmuch/xapian/*.DB
+test_expect_equal "$output" "A Xapian exception occurred opening database"
+
 test_done
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch v6 2/8] test: add support for compiling and running C snippets
  2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
  2015-03-27 22:11 ` [Patch v6 1/8] test: Add two tests for error output from notmuch_database_open David Bremner
@ 2015-03-27 22:11 ` David Bremner
  2015-03-27 22:11 ` [Patch v6 3/8] test: add error reporting tests David Bremner
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:11 UTC (permalink / raw)
  To: notmuch

This is to limit the copy-pasta involved in running C tests. I decided
to keep things simple and not try to provide an actual C skeleton.

The setting of LD_LIBRARY_PATH is to force using the built libnotmuch
rather than any potential system one.
---
 test/README      |  5 +++++
 test/test-lib.sh | 15 +++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/test/README b/test/README
index 81a1c82..5b40474 100644
--- a/test/README
+++ b/test/README
@@ -84,6 +84,11 @@ the tests in one of the following ways.
 	TEST_EMACS=my-special-emacs TEST_EMACSCLIENT=my-emacsclient ./emacs
 	make test TEST_EMACS=my-special-emacs TEST_EMACSCLIENT=my-emacsclient
 
+Some tests may require a c compiler. You can choose the name and flags similarly 
+to with emacs, e.g.
+
+     make test TEST_CC=gcc TEST_CFLAGS="-g -O2"
+     
 Quiet Execution
 ---------------
 
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 133fbe4..486d1c4 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -73,6 +73,8 @@ if [[ ( -n "$TEST_EMACS" && -z "$TEST_EMACSCLIENT" ) || \
 fi
 TEST_EMACS=${TEST_EMACS:-${EMACS:-emacs}}
 TEST_EMACSCLIENT=${TEST_EMACSCLIENT:-emacsclient}
+TEST_CC=${TEST_CC:-cc}
+TEST_CFLAGS=${TEST_CFLAGS:-"-g -O0"}
 
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
@@ -1161,6 +1163,19 @@ test_python() {
 		| $cmd -
 }
 
+test_C () {
+    exec_file="test${test_count}"
+    test_file="${exec_file}.c"
+    cat > ${test_file}
+    export LD_LIBRARY_PATH=${TEST_DIRECTORY}/../lib
+    ${TEST_CC} ${TEST_CFLAGS} -I${TEST_DIRECTORY}/../lib -o ${exec_file} ${test_file} -L${TEST_DIRECTORY}/../lib/ -lnotmuch -ltalloc
+    echo "== stdout ==" > OUTPUT.stdout
+    echo "== stderr ==" > OUTPUT.stderr
+    ./${exec_file} "$@" 1>>OUTPUT.stdout 2>>OUTPUT.stderr
+    sed "s,${PWD},CWD,g"  OUTPUT.stdout OUTPUT.stderr > OUTPUT
+}
+
+
 # Creates a script that counts how much time it is executed and calls
 # notmuch.  $notmuch_counter_command is set to the path to the
 # generated script.  Use notmuch_counter_value() function to get the
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch v6 3/8] test: add error reporting tests
  2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
  2015-03-27 22:11 ` [Patch v6 1/8] test: Add two tests for error output from notmuch_database_open David Bremner
  2015-03-27 22:11 ` [Patch v6 2/8] test: add support for compiling and running C snippets David Bremner
@ 2015-03-27 22:11 ` David Bremner
  2015-03-27 22:11 ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open,create} David Bremner
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:11 UTC (permalink / raw)
  To: notmuch

This includes tests for all of the error fprintfs in the library I
could figure out how to test without using gdb scripts. It boils down
to errors opening files and exceptions caused by corrupted Xapian
databases.
---
 test/T560-lib-error.sh | 285 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 285 insertions(+)
 create mode 100755 test/T560-lib-error.sh

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
new file mode 100755
index 0000000..f1ca543
--- /dev/null
+++ b/test/T560-lib-error.sh
@@ -0,0 +1,285 @@
+#!/usr/bin/env bash
+test_description="error reporting for library"
+
+. ./test-lib.sh
+
+backup_database () {
+    rm -rf notmuch-dir-backup
+    cp -pR ${MAIL_DIR}/.notmuch notmuch-dir-backup
+}
+restore_database () {
+    rm -rf ${MAIL_DIR}/.notmuch
+    cp -pR notmuch-dir-backup ${MAIL_DIR}/.notmuch
+}
+
+
+add_email_corpus
+
+test_expect_success "building database" "NOTMUCH_NEW"
+
+test_begin_subtest "Open null pointer"
+test_C <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+    notmuch_database_t *db;
+    notmuch_status_t stat;
+    stat = notmuch_database_open (NULL, 0, 0);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Error: Cannot open a database for a NULL path.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "Open nonexistent database"
+test_C <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+    notmuch_database_t *db;
+    notmuch_status_t stat;
+    stat = notmuch_database_open ("/nonexistent/foo", 0, 0);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Error opening database at /nonexistent/foo/.notmuch: No such file or directory
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "create NULL path"
+test_C <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+    notmuch_status_t stat;
+    stat = notmuch_database_create (NULL, NULL);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Error: Cannot create a database for a NULL path.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "Create database in non-existant directory"
+test_C <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+    notmuch_database_t *db;
+    notmuch_status_t stat;
+    stat = notmuch_database_create ("/nonexistent/foo", &db);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Error: Cannot create database at /nonexistent/foo: No such file or directory.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "Write to read-only database"
+test_C ${MAIL_DIR} <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_status_t stat;
+   stat = notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_ONLY, &db);
+   if (stat != NOTMUCH_STATUS_SUCCESS) {
+     fprintf (stderr, "error opening database: %d\n", stat);
+   }
+   stat = notmuch_database_add_message (db, "/dev/null", NULL);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Cannot write to a read-only database.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "Add non-existent file"
+test_C ${MAIL_DIR} <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_status_t stat;
+   stat = notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db);
+   if (stat != NOTMUCH_STATUS_SUCCESS) {
+     fprintf (stderr, "error opening database: %d\n", stat);
+   }
+   stat = notmuch_database_add_message (db, "/nonexistent", NULL);
+   if (stat)
+       fputs (notmuch_database_status_string (db), stderr);
+
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Error opening /nonexistent: No such file or directory
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "compact, overwriting existing backup"
+test_C ${MAIL_DIR} <<'EOF'
+#include <stdio.h>
+#include <notmuch.h>
+static void
+status_cb (const char *msg, void *closure)
+{
+    printf ("%s\n", msg);
+}
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_status_t stat;
+   stat = notmuch_database_compact (argv[1], argv[1], status_cb, NULL);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+Path already exists: CWD/mail
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+cat <<'EOF' > c_head
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <talloc.h>
+#include <notmuch.h>
+
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_status_t stat;
+   char *path;
+   int fd;
+
+   stat = notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db);
+   if (stat != NOTMUCH_STATUS_SUCCESS) {
+     fprintf (stderr, "error opening database: %d\n", stat);
+   }
+   path = talloc_asprintf (db, "%s/.notmuch/xapian/postlist.DB", argv[1]);
+   fd = open(path,O_WRONLY|O_TRUNC);
+   if (fd < 0)
+       fprintf (stderr, "error opening %s\n");
+EOF
+cat <<'EOF' > c_tail
+   if (stat) {
+       const char *stat_str = notmuch_database_status_string (db);
+       if (stat_str)
+           fputs (stat_str, stderr);
+    }
+
+}
+EOF
+
+backup_database
+test_begin_subtest "Xapian exception finding message"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+   {
+       notmuch_message_t *message = NULL;
+       stat = notmuch_database_find_message (db, "id:nonexistant", &message);
+   }
+EOF
+sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+A Xapian exception occurred finding message
+EOF
+test_expect_equal_file EXPECTED OUTPUT.clean
+restore_database
+
+backup_database
+test_begin_subtest "Xapian exception getting tags"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+   {
+       notmuch_tags_t *tags = NULL;
+       tags = notmuch_database_get_all_tags (db);
+       stat = (tags == NULL);
+   }
+EOF
+sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+A Xapian exception occurred getting tags
+EOF
+test_expect_equal_file EXPECTED OUTPUT.clean
+restore_database
+
+backup_database
+test_begin_subtest "Xapian exception creating directory"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+   {
+       notmuch_directory_t *directory = NULL;
+       stat = notmuch_database_get_directory (db, "none/existing", &directory);
+   }
+EOF
+sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+A Xapian exception occurred creating a directory
+EOF
+test_expect_equal_file EXPECTED OUTPUT.clean
+restore_database
+
+backup_database
+test_begin_subtest "Xapian exception searching messages"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+   {
+       notmuch_messages_t *messages = NULL;
+       notmuch_query_t *query=notmuch_query_create (db, "*");
+       stat = notmuch_query_search_messages_st (query, &messages);
+   }
+EOF
+sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+A Xapian exception occurred performing query
+Query string was: *
+EOF
+test_expect_equal_file EXPECTED OUTPUT.clean
+restore_database
+
+backup_database
+test_begin_subtest "Xapian exception counting messages"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+   {
+       notmuch_query_t *query=notmuch_query_create (db, "id:87ocn0qh6d.fsf@yoom.home.cworth.org");
+       int count = notmuch_query_count_messages (query);
+       stat = (count == 0);
+   }
+EOF
+sed 's/^\(A Xapian exception [^:]*\):.*$/\1/' < OUTPUT > OUTPUT.clean
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+A Xapian exception occurred performing query
+Query string was: id:87ocn0qh6d.fsf@yoom.home.cworth.org
+EOF
+test_expect_equal_file EXPECTED OUTPUT.clean
+restore_database
+
+test_done
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open,create}
  2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
                   ` (2 preceding siblings ...)
  2015-03-27 22:11 ` [Patch v6 3/8] test: add error reporting tests David Bremner
@ 2015-03-27 22:11 ` David Bremner
  2015-03-28 10:04   ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open, create} Tomi Ollila
  2015-03-27 22:11 ` [Patch v6 5/8] lib: add a log function with output to a string in notmuch_database_t David Bremner
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:11 UTC (permalink / raw)
  To: notmuch

The compatibility wrapper ensures that clients calling
notmuch_database_open will receive consistent output for now.

The changes to notmuch-{new,search} and test/symbol-test are just to
make the test suite pass.

The use of IGNORE_RESULT is justified by two things. 1) I don't know
what else to do.  2) asprintf guarantees the output string is NULL if
an error occurs, so at least we are not passing garbage back.
---
 lib/database.cc     | 109 ++++++++++++++++++++++++++++++++++++++--------------
 lib/notmuch.h       |  21 ++++++++++
 notmuch-new.c       |  11 +++++-
 notmuch-search.c    |  13 ++++++-
 test/symbol-test.cc |   9 ++++-
 5 files changed, 130 insertions(+), 33 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 3974e2e..5d973b9 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -608,29 +608,50 @@ parse_references (void *ctx,
 notmuch_status_t
 notmuch_database_create (const char *path, notmuch_database_t **database)
 {
+    char *status_string = NULL;
+    notmuch_status_t status;
+
+    status = notmuch_database_create_verbose (path, database,
+					      &status_string);
+
+    if (status_string) {
+	fputs (status_string, stderr);
+	free (status_string);
+    }
+
+    return status;
+}
+
+notmuch_status_t
+notmuch_database_create_verbose (const char *path,
+				 notmuch_database_t **database,
+				 char **status_string)
+{
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     notmuch_database_t *notmuch = NULL;
     char *notmuch_path = NULL;
+    char *message = NULL;
     struct stat st;
     int err;
 
     if (path == NULL) {
-	fprintf (stderr, "Error: Cannot create a database for a NULL path.\n");
+	message = strdup ("Error: Cannot create a database for a NULL path.\n");
 	status = NOTMUCH_STATUS_NULL_POINTER;
 	goto DONE;
     }
 
     err = stat (path, &st);
     if (err) {
-	fprintf (stderr, "Error: Cannot create database at %s: %s.\n",
-		 path, strerror (errno));
+	IGNORE_RESULT (asprintf (&message, "Error: Cannot create database at %s: %s.\n",
+				path, strerror (errno)));
 	status = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
 
     if (! S_ISDIR (st.st_mode)) {
-	fprintf (stderr, "Error: Cannot create database at %s: Not a directory.\n",
-		 path);
+	IGNORE_RESULT (asprintf (&message, "Error: Cannot create database at %s: "
+				 "Not a directory.\n",
+				 path));
 	status = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
@@ -640,15 +661,15 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
     err = mkdir (notmuch_path, 0755);
 
     if (err) {
-	fprintf (stderr, "Error: Cannot create directory %s: %s.\n",
-		 notmuch_path, strerror (errno));
+	IGNORE_RESULT (asprintf (&message, "Error: Cannot create directory %s: %s.\n",
+				 notmuch_path, strerror (errno)));
 	status = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
 
-    status = notmuch_database_open (path,
-				    NOTMUCH_DATABASE_MODE_READ_WRITE,
-				    &notmuch);
+    status = notmuch_database_open_verbose (path,
+					    NOTMUCH_DATABASE_MODE_READ_WRITE,
+					    &notmuch, &message);
     if (status)
 	goto DONE;
 
@@ -667,6 +688,9 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
     if (notmuch_path)
 	talloc_free (notmuch_path);
 
+    if (status_string && message)
+	*status_string = message;
+
     if (database)
 	*database = notmuch;
     else
@@ -767,37 +791,58 @@ notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode,
 		       notmuch_database_t **database)
 {
+    char *status_string = NULL;
+    notmuch_status_t status;
+
+    status = notmuch_database_open_verbose (path, mode, database,
+					   &status_string);
+
+    if (status_string) {
+	fputs (status_string, stderr);
+	free (status_string);
+    }
+
+    return status;
+}
+
+notmuch_status_t
+notmuch_database_open_verbose (const char *path,
+			       notmuch_database_mode_t mode,
+			       notmuch_database_t **database,
+			       char **status_string)
+{
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     void *local = talloc_new (NULL);
     notmuch_database_t *notmuch = NULL;
     char *notmuch_path, *xapian_path, *incompat_features;
+    char *message = NULL;
     struct stat st;
     int err;
     unsigned int i, version;
     static int initialized = 0;
 
     if (path == NULL) {
-	fprintf (stderr, "Error: Cannot open a database for a NULL path.\n");
+	message = strdup ("Error: Cannot open a database for a NULL path.\n");
 	status = NOTMUCH_STATUS_NULL_POINTER;
 	goto DONE;
     }
 
     if (! (notmuch_path = talloc_asprintf (local, "%s/%s", path, ".notmuch"))) {
-	fprintf (stderr, "Out of memory\n");
+	message = strdup ("Out of memory\n");
 	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
 	goto DONE;
     }
 
     err = stat (notmuch_path, &st);
     if (err) {
-	fprintf (stderr, "Error opening database at %s: %s\n",
-		 notmuch_path, strerror (errno));
+	IGNORE_RESULT (asprintf (&message, "Error opening database at %s: %s\n",
+				 notmuch_path, strerror (errno)));
 	status = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
 
     if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
-	fprintf (stderr, "Out of memory\n");
+	message = strdup ("Out of memory\n");
 	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
 	goto DONE;
     }
@@ -837,11 +882,11 @@ notmuch_database_open (const char *path,
 	 * means a dramatically incompatible change. */
 	version = notmuch_database_get_version (notmuch);
 	if (version > NOTMUCH_DATABASE_VERSION) {
-	    fprintf (stderr,
-		     "Error: Notmuch database at %s\n"
-		     "       has a newer database format version (%u) than supported by this\n"
-		     "       version of notmuch (%u).\n",
-		     notmuch_path, version, NOTMUCH_DATABASE_VERSION);
+	    IGNORE_RESULT (asprintf (&message,
+		      "Error: Notmuch database at %s\n"
+		      "       has a newer database format version (%u) than supported by this\n"
+		      "       version of notmuch (%u).\n",
+				     notmuch_path, version, NOTMUCH_DATABASE_VERSION));
 	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
 	    notmuch_database_destroy (notmuch);
 	    notmuch = NULL;
@@ -856,11 +901,11 @@ notmuch_database_open (const char *path,
 	    version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',
 	    &incompat_features);
 	if (incompat_features) {
-	    fprintf (stderr,
-		     "Error: Notmuch database at %s\n"
-		     "       requires features (%s)\n"
-		     "       not supported by this version of notmuch.\n",
-		     notmuch_path, incompat_features);
+	    IGNORE_RESULT (asprintf (&message,
+		"Error: Notmuch database at %s\n"
+		"       requires features (%s)\n"
+		"       not supported by this version of notmuch.\n",
+				     notmuch_path, incompat_features));
 	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
 	    notmuch_database_destroy (notmuch);
 	    notmuch = NULL;
@@ -906,8 +951,8 @@ notmuch_database_open (const char *path,
 	    notmuch->query_parser->add_prefix (prefix->name, prefix->prefix);
 	}
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
-		 error.get_msg().c_str());
+	IGNORE_RESULT (asprintf (&message, "A Xapian exception occurred opening database: %s\n",
+				 error.get_msg().c_str()));
 	notmuch_database_destroy (notmuch);
 	notmuch = NULL;
 	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
@@ -916,6 +961,9 @@ notmuch_database_open (const char *path,
   DONE:
     talloc_free (local);
 
+    if (status_string && message)
+	*status_string = message;
+
     if (database)
 	*database = notmuch;
     else
@@ -1039,13 +1087,18 @@ notmuch_database_compact (const char *path,
     notmuch_database_t *notmuch = NULL;
     struct stat statbuf;
     notmuch_bool_t keep_backup;
+    char *message = NULL;
 
     local = talloc_new (NULL);
     if (! local)
 	return NOTMUCH_STATUS_OUT_OF_MEMORY;
 
-    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
+    ret = notmuch_database_open_verbose (path,
+					 NOTMUCH_DATABASE_MODE_READ_WRITE,
+					 &notmuch,
+					 &message);
     if (ret) {
+	if (status_cb) status_cb (message, closure);
 	goto DONE;
     }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index f066245..c671d82 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -230,6 +230,16 @@ notmuch_status_t
 notmuch_database_create (const char *path, notmuch_database_t **database);
 
 /**
+ * Like notmuch_database_create, except optionally return an error
+ * message. This message is allocated by malloc and should be freed by
+ * the caller.
+ */
+notmuch_status_t
+notmuch_database_create_verbose (const char *path,
+				 notmuch_database_t **database,
+				 char **error_message);
+
+/**
  * Database open mode for notmuch_database_open.
  */
 typedef enum {
@@ -279,6 +289,17 @@ notmuch_status_t
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode,
 		       notmuch_database_t **database);
+/**
+ * Like notmuch_database_open, except optionally return an error
+ * message. This message is allocated by malloc and should be freed by
+ * the caller.
+ */
+
+notmuch_status_t
+notmuch_database_open_verbose (const char *path,
+			       notmuch_database_mode_t mode,
+			       notmuch_database_t **database,
+			       char **error_message);
 
 /**
  * Commit changes and close the given notmuch database.
diff --git a/notmuch-new.c b/notmuch-new.c
index ddf42c1..e6c283e 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -985,9 +985,16 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	    return EXIT_FAILURE;
 	add_files_state.total_files = count;
     } else {
-	if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
-				   &notmuch))
+	char *status_string = NULL;
+	if (notmuch_database_open_verbose (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
+					   &notmuch, &status_string)) {
+	    if (status_string) {
+		fputs (status_string, stderr);
+		free (status_string);
+	    }
+
 	    return EXIT_FAILURE;
+	}
 
 	if (notmuch_database_needs_upgrade (notmuch)) {
 	    time_t now = time (NULL);
diff --git a/notmuch-search.c b/notmuch-search.c
index a591d45..b81ac01 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -545,6 +545,7 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar
 {
     char *query_str;
     unsigned int i;
+    char *status_string = NULL;
 
     switch (ctx->format_sel) {
     case NOTMUCH_FORMAT_TEXT:
@@ -570,9 +571,17 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar
 
     notmuch_exit_if_unsupported_format ();
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch))
+    if (notmuch_database_open_verbose (
+	    notmuch_config_get_database_path (config),
+	    NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch, &status_string)) {
+
+	if (status_string) {
+	    fputs (status_string, stderr);
+	    free (status_string);
+	}
+
 	return EXIT_FAILURE;
+    }
 
     query_str = query_string_from_args (ctx->notmuch, argc, argv);
     if (query_str == NULL) {
diff --git a/test/symbol-test.cc b/test/symbol-test.cc
index 3e96c03..d979f83 100644
--- a/test/symbol-test.cc
+++ b/test/symbol-test.cc
@@ -1,11 +1,18 @@
 #include <stdio.h>
+#include <stdlib.h>
 #include <xapian.h>
 #include <notmuch.h>
 
 
 int main() {
   notmuch_database_t *notmuch;
-  notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch);
+  char *message = NULL;
+
+  if (notmuch_database_open_verbose  ("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch, &message))
+      if (message) {
+	  fputs (message, stderr);
+	  free (message);
+      }
 
   try {
     (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch v6 5/8] lib: add a log function with output to a string in notmuch_database_t
  2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
                   ` (3 preceding siblings ...)
  2015-03-27 22:11 ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open,create} David Bremner
@ 2015-03-27 22:11 ` David Bremner
  2015-03-27 22:11 ` [Patch v6 6/8] lib: add private function to extract the database for a message David Bremner
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:11 UTC (permalink / raw)
  To: notmuch

In principle in the future this could do something fancier than
asprintf.
---
 lib/database-private.h |  4 ++++
 lib/database.cc        | 24 ++++++++++++++++++++++++
 lib/notmuch-private.h  |  4 ++++
 lib/notmuch.h          |  7 +++++++
 4 files changed, 39 insertions(+)

diff --git a/lib/database-private.h b/lib/database-private.h
index 6d6fa2c..24243db 100644
--- a/lib/database-private.h
+++ b/lib/database-private.h
@@ -154,6 +154,10 @@ struct _notmuch_database {
     unsigned int last_doc_id;
     uint64_t last_thread_id;
 
+    /* error reporting; this value persists only until the
+     * next library call. May be NULL */
+    char *status_string;
+
     Xapian::QueryParser *query_parser;
     Xapian::TermGenerator *term_gen;
     Xapian::ValueRangeProcessor *value_range_processor;
diff --git a/lib/database.cc b/lib/database.cc
index 5d973b9..c114765 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -348,6 +348,23 @@ notmuch_status_to_string (notmuch_status_t status)
     }
 }
 
+void
+_notmuch_database_log (notmuch_database_t *notmuch,
+		      const char *format,
+		      ...)
+{
+    va_list va_args;
+
+    va_start (va_args, format);
+
+    if (notmuch->status_string)
+	talloc_free (notmuch->status_string);
+
+    notmuch->status_string = talloc_vasprintf (notmuch, format, va_args);
+
+    va_end (va_args);
+}
+
 static void
 find_doc_ids_for_term (notmuch_database_t *notmuch,
 		       const char *term,
@@ -860,6 +877,7 @@ notmuch_database_open_verbose (const char *path,
 
     notmuch = talloc_zero (NULL, notmuch_database_t);
     notmuch->exception_reported = FALSE;
+    notmuch->status_string = NULL;
     notmuch->path = talloc_strdup (notmuch, path);
 
     if (notmuch->path[strlen (notmuch->path) - 1] == '/')
@@ -2545,3 +2563,9 @@ notmuch_database_get_all_tags (notmuch_database_t *db)
 	return NULL;
     }
 }
+
+const char *
+notmuch_database_status_string (notmuch_database_t *notmuch)
+{
+    return notmuch->status_string;
+}
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 8a1f2fa..7cb6fd4 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -190,6 +190,10 @@ _notmuch_message_id_compressed (void *ctx, const char *message_id);
 notmuch_status_t
 _notmuch_database_ensure_writable (notmuch_database_t *notmuch);
 
+void
+_notmuch_database_log (notmuch_database_t *notmuch,
+		       const char *format, ...);
+
 const char *
 _notmuch_database_relative_path (notmuch_database_t *notmuch,
 				 const char *path);
diff --git a/lib/notmuch.h b/lib/notmuch.h
index c671d82..20c4e01 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -302,6 +302,13 @@ notmuch_database_open_verbose (const char *path,
 			       char **error_message);
 
 /**
+ * Retrieve last status string for given database.
+ *
+ */
+const char *
+notmuch_database_status_string (notmuch_database_t *notmuch);
+
+/**
  * Commit changes and close the given notmuch database.
  *
  * After notmuch_database_close has been called, calls to other
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch v6 6/8] lib: add private function to extract the database for a message.
  2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
                   ` (4 preceding siblings ...)
  2015-03-27 22:11 ` [Patch v6 5/8] lib: add a log function with output to a string in notmuch_database_t David Bremner
@ 2015-03-27 22:11 ` David Bremner
  2015-03-27 22:11 ` [Patch v6 7/8] lib: replace almost all fprintfs in library with _n_d_log David Bremner
  2015-03-27 22:12 ` [Patch v6 8/8] lib: eliminate fprintf from _notmuch_message_file_open David Bremner
  7 siblings, 0 replies; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:11 UTC (permalink / raw)
  To: notmuch

This is needed by logging in functions outside message.cc that take
only a notmuch_message_t object.
---
 lib/message.cc        | 6 ++++++
 lib/notmuch-private.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/lib/message.cc b/lib/message.cc
index 956a70a..b84e5e1 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1626,3 +1626,9 @@ notmuch_message_destroy (notmuch_message_t *message)
 {
     talloc_free (message);
 }
+
+notmuch_database_t *
+_notmuch_message_database (notmuch_message_t *message)
+{
+    return message->notmuch;
+}
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 7cb6fd4..dc58a2f 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -472,6 +472,8 @@ _notmuch_doc_id_set_remove (notmuch_doc_id_set_t *doc_ids,
 void
 _notmuch_message_add_reply (notmuch_message_t *message,
 			    notmuch_message_t *reply);
+notmuch_database_t *
+_notmuch_message_database (notmuch_message_t *message);
 
 /* sha1.c */
 
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch v6 7/8] lib: replace almost all fprintfs in library with _n_d_log
  2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
                   ` (5 preceding siblings ...)
  2015-03-27 22:11 ` [Patch v6 6/8] lib: add private function to extract the database for a message David Bremner
@ 2015-03-27 22:11 ` David Bremner
  2015-03-27 22:12 ` [Patch v6 8/8] lib: eliminate fprintf from _notmuch_message_file_open David Bremner
  7 siblings, 0 replies; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:11 UTC (permalink / raw)
  To: notmuch

This is not supposed to change any functionality from an end user
point of view. Note that it will eliminate some output to stderr. The
query debugging output is left as is; it doesn't really fit with the
current primitive logging model. The remaining "bad" fprintf will need
an internal API change.
---
 lib/database.cc        | 38 +++++++++++++++++++++-----------------
 lib/directory.cc       |  4 ++--
 lib/index.cc           | 11 +++++++----
 lib/message.cc         |  6 +++---
 lib/query.cc           | 18 ++++++++++++------
 test/T560-lib-error.sh |  6 +++++-
 6 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index c114765..e40674a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -473,7 +473,7 @@ notmuch_database_find_message (notmuch_database_t *notmuch,
 
 	return NOTMUCH_STATUS_SUCCESS;
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred finding message: %s.\n",
+	_notmuch_database_log (notmuch, "A Xapian exception occurred finding message: %s.\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	*message_ret = NULL;
@@ -719,7 +719,7 @@ notmuch_status_t
 _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
 {
     if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY) {
-	fprintf (stderr, "Cannot write to a read-only database.\n");
+	_notmuch_database_log (notmuch, "Cannot write to a read-only database.\n");
 	return NOTMUCH_STATUS_READ_ONLY_DATABASE;
     }
 
@@ -1015,7 +1015,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
 	} catch (const Xapian::Error &error) {
 	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	    if (! notmuch->exception_reported) {
-		fprintf (stderr, "Error: A Xapian exception occurred closing database: %s\n",
+		_notmuch_database_log (notmuch, "Error: A Xapian exception occurred closing database: %s\n",
 			 error.get_msg().c_str());
 	    }
 	}
@@ -1147,12 +1147,12 @@ notmuch_database_compact (const char *path,
     }
 
     if (stat (backup_path, &statbuf) != -1) {
-	fprintf (stderr, "Path already exists: %s\n", backup_path);
+	_notmuch_database_log (notmuch, "Path already exists: %s\n", backup_path);
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
     if (errno != ENOENT) {
-	fprintf (stderr, "Unknown error while stat()ing path: %s\n",
+	_notmuch_database_log (notmuch, "Unknown error while stat()ing path: %s\n",
 		 strerror (errno));
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
@@ -1172,20 +1172,20 @@ notmuch_database_compact (const char *path,
 	compactor.set_destdir (compact_xapian_path);
 	compactor.compact ();
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "Error while compacting: %s\n", error.get_msg().c_str());
+	_notmuch_database_log (notmuch, "Error while compacting: %s\n", error.get_msg().c_str());
 	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	goto DONE;
     }
 
     if (rename (xapian_path, backup_path)) {
-	fprintf (stderr, "Error moving %s to %s: %s\n",
+	_notmuch_database_log (notmuch, "Error moving %s to %s: %s\n",
 		 xapian_path, backup_path, strerror (errno));
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
 
     if (rename (compact_xapian_path, xapian_path)) {
-	fprintf (stderr, "Error moving %s to %s: %s\n",
+	_notmuch_database_log (notmuch, "Error moving %s to %s: %s\n",
 		 compact_xapian_path, xapian_path, strerror (errno));
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
@@ -1193,7 +1193,7 @@ notmuch_database_compact (const char *path,
 
     if (! keep_backup) {
 	if (rmtree (backup_path)) {
-	    fprintf (stderr, "Error removing old database %s: %s\n",
+	    _notmuch_database_log (notmuch, "Error removing old database %s: %s\n",
 		     backup_path, strerror (errno));
 	    ret = NOTMUCH_STATUS_FILE_ERROR;
 	    goto DONE;
@@ -1204,6 +1204,10 @@ notmuch_database_compact (const char *path,
     if (notmuch) {
 	notmuch_status_t ret2;
 
+	const char *str = notmuch_database_status_string (notmuch);
+	if (status_cb && str)
+	    status_cb (str, closure);
+
 	ret2 = notmuch_database_destroy (notmuch);
 
 	/* don't clobber previous error status */
@@ -1222,7 +1226,7 @@ notmuch_database_compact (unused (const char *path),
 			  unused (notmuch_compact_status_cb_t status_cb),
 			  unused (void *closure))
 {
-    fprintf (stderr, "notmuch was compiled against a xapian version lacking compaction support.\n");
+    _notmuch_database_log (notmuch, "notmuch was compiled against a xapian version lacking compaction support.\n");
     return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;
 }
 #endif
@@ -1500,7 +1504,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	    }
 
 	    if (private_status) {
-		fprintf (stderr,
+		_notmuch_database_log (notmuch,
 			 "Upgrade failed while creating ghost messages.\n");
 		status = COERCE_STATUS (private_status, "Unexpected status from _notmuch_message_initialize_ghost");
 		goto DONE;
@@ -1550,7 +1554,7 @@ notmuch_database_begin_atomic (notmuch_database_t *notmuch)
     try {
 	(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->begin_transaction (false);
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred beginning transaction: %s.\n",
+	_notmuch_database_log (notmuch, "A Xapian exception occurred beginning transaction: %s.\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
@@ -1584,7 +1588,7 @@ notmuch_database_end_atomic (notmuch_database_t *notmuch)
 	if (thresh && atoi (thresh) == 1)
 	    db->flush ();
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred committing transaction: %s.\n",
+	_notmuch_database_log (notmuch, "A Xapian exception occurred committing transaction: %s.\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
@@ -1830,7 +1834,7 @@ notmuch_database_get_directory (notmuch_database_t *notmuch,
 	*directory = _notmuch_directory_create (notmuch, path,
 						NOTMUCH_FIND_LOOKUP, &status);
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred getting directory: %s.\n",
+	_notmuch_database_log (notmuch, "A Xapian exception occurred getting directory: %s.\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
@@ -2412,7 +2416,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 
 	_notmuch_message_sync (message);
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred adding message: %s.\n",
+	_notmuch_database_log (notmuch, "A Xapian exception occurred adding message: %s.\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
@@ -2504,7 +2508,7 @@ notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
 		status = NOTMUCH_STATUS_OUT_OF_MEMORY;
 	}
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "Error: A Xapian exception occurred finding message by filename: %s\n",
+	_notmuch_database_log (notmuch, "Error: A Xapian exception occurred finding message by filename: %s\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
@@ -2557,7 +2561,7 @@ notmuch_database_get_all_tags (notmuch_database_t *db)
 	_notmuch_string_list_sort (tags);
 	return _notmuch_tags_create (db, tags);
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred getting tags: %s.\n",
+	_notmuch_database_log (db, "A Xapian exception occurred getting tags: %s.\n",
 		 error.get_msg().c_str());
 	db->exception_reported = TRUE;
 	return NULL;
diff --git a/lib/directory.cc b/lib/directory.cc
index 8daaec8..b836ea2 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -186,7 +186,7 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
 	directory->mtime = Xapian::sortable_unserialise (
 	    directory->doc.get_value (NOTMUCH_VALUE_TIMESTAMP));
     } catch (const Xapian::Error &error) {
-	fprintf (stderr,
+	_notmuch_database_log (notmuch,
 		 "A Xapian exception occurred creating a directory: %s.\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
@@ -228,7 +228,7 @@ notmuch_directory_set_mtime (notmuch_directory_t *directory,
 
 	db->replace_document (directory->document_id, directory->doc);
     } catch (const Xapian::Error &error) {
-	fprintf (stderr,
+	_notmuch_database_log (notmuch,
 		 "A Xapian exception occurred setting directory mtime: %s.\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
diff --git a/lib/index.cc b/lib/index.cc
index c88ed8d..e81aa81 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -314,7 +314,8 @@ _index_mime_part (notmuch_message_t *message,
     const char *charset;
 
     if (! part) {
-	fprintf (stderr, "Warning: Not indexing empty mime part.\n");
+	_notmuch_database_log (_notmuch_message_database (message),
+			      "Warning: Not indexing empty mime part.\n");
 	return;
     }
 
@@ -344,7 +345,8 @@ _index_mime_part (notmuch_message_t *message,
 		if (i == 1)
 		    continue;
 		if (i > 1)
-		    fprintf (stderr, "Warning: Unexpected extra parts of multipart/signed. Indexing anyway.\n");
+		    _notmuch_database_log (_notmuch_message_database (message),
+					  "Warning: Unexpected extra parts of multipart/signed. Indexing anyway.\n");
 	    }
 	    if (GMIME_IS_MULTIPART_ENCRYPTED (multipart)) {
 		/* Don't index encrypted parts. */
@@ -367,8 +369,9 @@ _index_mime_part (notmuch_message_t *message,
     }
 
     if (! (GMIME_IS_PART (part))) {
-	fprintf (stderr, "Warning: Not indexing unknown mime part: %s.\n",
-		 g_type_name (G_OBJECT_TYPE (part)));
+	_notmuch_database_log (_notmuch_message_database (message),
+			      "Warning: Not indexing unknown mime part: %s.\n",
+			      g_type_name (G_OBJECT_TYPE (part)));
 	return;
     }
 
diff --git a/lib/message.cc b/lib/message.cc
index b84e5e1..a8ca988 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -252,7 +252,7 @@ _notmuch_message_create_for_message_id (notmuch_database_t *notmuch,
 
 	doc_id = _notmuch_database_generate_doc_id (notmuch);
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred creating message: %s\n",
+	_notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred creating message: %s\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	*status_ret = NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
@@ -467,7 +467,7 @@ notmuch_message_get_header (notmuch_message_t *message, const char *header)
 		return talloc_strdup (message, value.c_str ());
 
 	} catch (Xapian::Error &error) {
-	    fprintf (stderr, "A Xapian exception occurred when reading header: %s\n",
+	    _notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred when reading header: %s\n",
 		     error.get_msg().c_str());
 	    message->notmuch->exception_reported = TRUE;
 	    return NULL;
@@ -921,7 +921,7 @@ notmuch_message_get_date (notmuch_message_t *message)
     try {
 	value = message->doc.get_value (NOTMUCH_VALUE_TIMESTAMP);
     } catch (Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred when reading date: %s\n",
+	_notmuch_database_log(_notmuch_message_database (message), "A Xapian exception occurred when reading date: %s\n",
 		 error.get_msg().c_str());
 	message->notmuch->exception_reported = TRUE;
 	return 0;
diff --git a/lib/query.cc b/lib/query.cc
index 57aa6d2..9cedb6a 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -296,9 +296,12 @@ notmuch_query_search_messages_st (notmuch_query_t *query,
 	return NOTMUCH_STATUS_SUCCESS;
 
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred performing query: %s\n",
-		 error.get_msg().c_str());
-	fprintf (stderr, "Query string was: %s\n", query->query_string);
+	_notmuch_database_log (notmuch,
+			       "A Xapian exception occurred performing query: %s\n"
+			       "Query string was: %s\n",
+			       error.get_msg().c_str(),
+			       query->query_string);
+
 	notmuch->exception_reported = TRUE;
 	talloc_free (messages);
 	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
@@ -597,9 +600,12 @@ notmuch_query_count_messages (notmuch_query_t *query)
 	count = mset.get_matches_estimated();
 
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred: %s\n",
-		 error.get_msg().c_str());
-	fprintf (stderr, "Query string was: %s\n", query->query_string);
+	_notmuch_database_log (notmuch,
+			       "A Xapian exception occurred performing query: %s\n"
+			       "Query string was: %s\n",
+			       error.get_msg().c_str(),
+			       query->query_string);
+
     }
 
     return count;
diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index f1ca543..67a5e8d 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -101,6 +101,9 @@ int main (int argc, char** argv)
      fprintf (stderr, "error opening database: %d\n", stat);
    }
    stat = notmuch_database_add_message (db, "/dev/null", NULL);
+   if (stat)
+       fputs (notmuch_database_status_string (db), stderr);
+
 }
 EOF
 cat <<'EOF' >EXPECTED
@@ -153,8 +156,9 @@ int main (int argc, char** argv)
 EOF
 cat <<'EOF' >EXPECTED
 == stdout ==
-== stderr ==
 Path already exists: CWD/mail
+
+== stderr ==
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch v6 8/8] lib: eliminate fprintf from _notmuch_message_file_open
  2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
                   ` (6 preceding siblings ...)
  2015-03-27 22:11 ` [Patch v6 7/8] lib: replace almost all fprintfs in library with _n_d_log David Bremner
@ 2015-03-27 22:12 ` David Bremner
  7 siblings, 0 replies; 11+ messages in thread
From: David Bremner @ 2015-03-27 22:12 UTC (permalink / raw)
  To: notmuch

You may wonder why _notmuch_message_file_open_ctx has two parameters.
This is because we need sometime to use a ctx which is a
notmuch_message_t. While we could get the database from this, there is
no easy way in C to tell type we are getting.
---
 lib/database.cc       |  2 +-
 lib/message-file.c    | 11 +++++++----
 lib/message.cc        |  3 ++-
 lib/notmuch-private.h |  5 +++--
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index e40674a..9f66b5f 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -2307,7 +2307,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
     if (ret)
 	return ret;
 
-    message_file = _notmuch_message_file_open (filename);
+    message_file = _notmuch_message_file_open (notmuch, filename);
     if (message_file == NULL)
 	return NOTMUCH_STATUS_FILE_ERROR;
 
diff --git a/lib/message-file.c b/lib/message-file.c
index a41d9ad..8ac96e8 100644
--- a/lib/message-file.c
+++ b/lib/message-file.c
@@ -76,7 +76,8 @@ _notmuch_message_file_destructor (notmuch_message_file_t *message)
 /* Create a new notmuch_message_file_t for 'filename' with 'ctx' as
  * the talloc owner. */
 notmuch_message_file_t *
-_notmuch_message_file_open_ctx (void *ctx, const char *filename)
+_notmuch_message_file_open_ctx (notmuch_database_t *notmuch,
+				void *ctx, const char *filename)
 {
     notmuch_message_file_t *message;
 
@@ -98,16 +99,18 @@ _notmuch_message_file_open_ctx (void *ctx, const char *filename)
     return message;
 
   FAIL:
-    fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+    _notmuch_database_log (notmuch, "Error opening %s: %s\n",
+			  filename, strerror (errno));
     _notmuch_message_file_close (message);
 
     return NULL;
 }
 
 notmuch_message_file_t *
-_notmuch_message_file_open (const char *filename)
+_notmuch_message_file_open (notmuch_database_t *notmuch,
+			    const char *filename)
 {
-    return _notmuch_message_file_open_ctx (NULL, filename);
+    return _notmuch_message_file_open_ctx (notmuch, NULL, filename);
 }
 
 void
diff --git a/lib/message.cc b/lib/message.cc
index a8ca988..5bc7aff 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -437,7 +437,8 @@ _notmuch_message_ensure_message_file (notmuch_message_t *message)
     if (unlikely (filename == NULL))
 	return;
 
-    message->message_file = _notmuch_message_file_open_ctx (message, filename);
+    message->message_file = _notmuch_message_file_open_ctx (
+	_notmuch_message_database (message), message, filename);
 }
 
 const char *
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index dc58a2f..cc9ce12 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -358,11 +358,12 @@ typedef struct _notmuch_message_file notmuch_message_file_t;
  * Returns NULL if any error occurs.
  */
 notmuch_message_file_t *
-_notmuch_message_file_open (const char *filename);
+_notmuch_message_file_open (notmuch_database_t *notmuch, const char *filename);
 
 /* Like notmuch_message_file_open but with 'ctx' as the talloc owner. */
 notmuch_message_file_t *
-_notmuch_message_file_open_ctx (void *ctx, const char *filename);
+_notmuch_message_file_open_ctx (notmuch_database_t *notmuch,
+				void *ctx, const char *filename);
 
 /* Close a notmuch message previously opened with notmuch_message_open. */
 void
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open, create}
  2015-03-27 22:11 ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open,create} David Bremner
@ 2015-03-28 10:04   ` Tomi Ollila
  2015-03-28 23:46     ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open,create} David Bremner
  0 siblings, 1 reply; 11+ messages in thread
From: Tomi Ollila @ 2015-03-28 10:04 UTC (permalink / raw)
  To: David Bremner, notmuch

On Sat, Mar 28 2015, David Bremner <david@tethera.net> wrote:

> The compatibility wrapper ensures that clients calling
> notmuch_database_open will receive consistent output for now.
>
> The changes to notmuch-{new,search} and test/symbol-test are just to
> make the test suite pass.
>
> The use of IGNORE_RESULT is justified by two things. 1) I don't know
> what else to do.  2) asprintf guarantees the output string is NULL if
> an error occurs, so at least we are not passing garbage back.
> ---
>  lib/database.cc     | 109 ++++++++++++++++++++++++++++++++++++++--------------
>  lib/notmuch.h       |  21 ++++++++++
>  notmuch-new.c       |  11 +++++-
>  notmuch-search.c    |  13 ++++++-
>  test/symbol-test.cc |   9 ++++-
>  5 files changed, 130 insertions(+), 33 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 3974e2e..5d973b9 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -608,29 +608,50 @@ parse_references (void *ctx,
>  notmuch_status_t
>  notmuch_database_create (const char *path, notmuch_database_t **database)
>  {
> +    char *status_string = NULL;
> +    notmuch_status_t status;
> +
> +    status = notmuch_database_create_verbose (path, database,
> +					      &status_string);
> +
> +    if (status_string) {
> +	fputs (status_string, stderr);
> +	free (status_string);
> +    }
> +
> +    return status;
> +}
> +
> +notmuch_status_t
> +notmuch_database_create_verbose (const char *path,
> +				 notmuch_database_t **database,
> +				 char **status_string)
> +{
>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>      notmuch_database_t *notmuch = NULL;
>      char *notmuch_path = NULL;
> +    char *message = NULL;
>      struct stat st;
>      int err;
>  
>      if (path == NULL) {
> -	fprintf (stderr, "Error: Cannot create a database for a NULL path.\n");
> +	message = strdup ("Error: Cannot create a database for a NULL path.\n");
>  	status = NOTMUCH_STATUS_NULL_POINTER;
>  	goto DONE;
>      }
>  
>      err = stat (path, &st);
>      if (err) {
> -	fprintf (stderr, "Error: Cannot create database at %s: %s.\n",
> -		 path, strerror (errno));
> +	IGNORE_RESULT (asprintf (&message, "Error: Cannot create database at %s: %s.\n",
> +				path, strerror (errno)));
>  	status = NOTMUCH_STATUS_FILE_ERROR;
>  	goto DONE;
>      }
>  
>      if (! S_ISDIR (st.st_mode)) {
> -	fprintf (stderr, "Error: Cannot create database at %s: Not a directory.\n",
> -		 path);
> +	IGNORE_RESULT (asprintf (&message, "Error: Cannot create database at %s: "
> +				 "Not a directory.\n",
> +				 path));
>  	status = NOTMUCH_STATUS_FILE_ERROR;
>  	goto DONE;
>      }
> @@ -640,15 +661,15 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
>      err = mkdir (notmuch_path, 0755);
>  
>      if (err) {
> -	fprintf (stderr, "Error: Cannot create directory %s: %s.\n",
> -		 notmuch_path, strerror (errno));
> +	IGNORE_RESULT (asprintf (&message, "Error: Cannot create directory %s: %s.\n",
> +				 notmuch_path, strerror (errno)));
>  	status = NOTMUCH_STATUS_FILE_ERROR;
>  	goto DONE;
>      }
>  
> -    status = notmuch_database_open (path,
> -				    NOTMUCH_DATABASE_MODE_READ_WRITE,
> -				    &notmuch);
> +    status = notmuch_database_open_verbose (path,
> +					    NOTMUCH_DATABASE_MODE_READ_WRITE,
> +					    &notmuch, &message);
>      if (status)
>  	goto DONE;
>  
> @@ -667,6 +688,9 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
>      if (notmuch_path)
>  	talloc_free (notmuch_path);
>  
> +    if (status_string && message)
> +	*status_string = message;

This series looks good and tests pass (I had confusion there before seeing
one of the last patch moving one output from stderr to stdout of where
the test status_cb prints it output)

Just that this small piece of code above git pass my sparse sieve, perhaps
this could be amended to:

    if (message) {
        if (status_string)
            *status_string = message;
        else 
            free(message);
    }


IMO either amend or leave to followup patch; just for that I don't
want to go through full patch series ;D


> +
>      if (database)
>  	*database = notmuch;
>      else
> @@ -767,37 +791,58 @@ notmuch_database_open (const char *path,
>  		       notmuch_database_mode_t mode,
>  		       notmuch_database_t **database)
>  {
> +    char *status_string = NULL;
> +    notmuch_status_t status;
> +
> +    status = notmuch_database_open_verbose (path, mode, database,
> +					   &status_string);
> +
> +    if (status_string) {
> +	fputs (status_string, stderr);
> +	free (status_string);
> +    }
> +
> +    return status;
> +}
> +
> +notmuch_status_t
> +notmuch_database_open_verbose (const char *path,
> +			       notmuch_database_mode_t mode,
> +			       notmuch_database_t **database,
> +			       char **status_string)
> +{
>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>      void *local = talloc_new (NULL);
>      notmuch_database_t *notmuch = NULL;
>      char *notmuch_path, *xapian_path, *incompat_features;
> +    char *message = NULL;
>      struct stat st;
>      int err;
>      unsigned int i, version;
>      static int initialized = 0;
>  
>      if (path == NULL) {
> -	fprintf (stderr, "Error: Cannot open a database for a NULL path.\n");
> +	message = strdup ("Error: Cannot open a database for a NULL path.\n");
>  	status = NOTMUCH_STATUS_NULL_POINTER;
>  	goto DONE;
>      }
>  
>      if (! (notmuch_path = talloc_asprintf (local, "%s/%s", path, ".notmuch"))) {
> -	fprintf (stderr, "Out of memory\n");
> +	message = strdup ("Out of memory\n");
>  	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
>  	goto DONE;
>      }
>  
>      err = stat (notmuch_path, &st);
>      if (err) {
> -	fprintf (stderr, "Error opening database at %s: %s\n",
> -		 notmuch_path, strerror (errno));
> +	IGNORE_RESULT (asprintf (&message, "Error opening database at %s: %s\n",
> +				 notmuch_path, strerror (errno)));
>  	status = NOTMUCH_STATUS_FILE_ERROR;
>  	goto DONE;
>      }
>  
>      if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
> -	fprintf (stderr, "Out of memory\n");
> +	message = strdup ("Out of memory\n");
>  	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
>  	goto DONE;
>      }
> @@ -837,11 +882,11 @@ notmuch_database_open (const char *path,
>  	 * means a dramatically incompatible change. */
>  	version = notmuch_database_get_version (notmuch);
>  	if (version > NOTMUCH_DATABASE_VERSION) {
> -	    fprintf (stderr,
> -		     "Error: Notmuch database at %s\n"
> -		     "       has a newer database format version (%u) than supported by this\n"
> -		     "       version of notmuch (%u).\n",
> -		     notmuch_path, version, NOTMUCH_DATABASE_VERSION);
> +	    IGNORE_RESULT (asprintf (&message,
> +		      "Error: Notmuch database at %s\n"
> +		      "       has a newer database format version (%u) than supported by this\n"
> +		      "       version of notmuch (%u).\n",
> +				     notmuch_path, version, NOTMUCH_DATABASE_VERSION));
>  	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
>  	    notmuch_database_destroy (notmuch);
>  	    notmuch = NULL;
> @@ -856,11 +901,11 @@ notmuch_database_open (const char *path,
>  	    version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',
>  	    &incompat_features);
>  	if (incompat_features) {
> -	    fprintf (stderr,
> -		     "Error: Notmuch database at %s\n"
> -		     "       requires features (%s)\n"
> -		     "       not supported by this version of notmuch.\n",
> -		     notmuch_path, incompat_features);
> +	    IGNORE_RESULT (asprintf (&message,
> +		"Error: Notmuch database at %s\n"
> +		"       requires features (%s)\n"
> +		"       not supported by this version of notmuch.\n",
> +				     notmuch_path, incompat_features));
>  	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
>  	    notmuch_database_destroy (notmuch);
>  	    notmuch = NULL;
> @@ -906,8 +951,8 @@ notmuch_database_open (const char *path,
>  	    notmuch->query_parser->add_prefix (prefix->name, prefix->prefix);
>  	}
>      } catch (const Xapian::Error &error) {
> -	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
> -		 error.get_msg().c_str());
> +	IGNORE_RESULT (asprintf (&message, "A Xapian exception occurred opening database: %s\n",
> +				 error.get_msg().c_str()));
>  	notmuch_database_destroy (notmuch);
>  	notmuch = NULL;
>  	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
> @@ -916,6 +961,9 @@ notmuch_database_open (const char *path,
>    DONE:
>      talloc_free (local);
>  
> +    if (status_string && message)
> +	*status_string = message;

Ditto.

> +
>      if (database)
>  	*database = notmuch;
>      else
> @@ -1039,13 +1087,18 @@ notmuch_database_compact (const char *path,
>      notmuch_database_t *notmuch = NULL;
>      struct stat statbuf;
>      notmuch_bool_t keep_backup;
> +    char *message = NULL;
>  
>      local = talloc_new (NULL);
>      if (! local)
>  	return NOTMUCH_STATUS_OUT_OF_MEMORY;
>  
> -    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
> +    ret = notmuch_database_open_verbose (path,
> +					 NOTMUCH_DATABASE_MODE_READ_WRITE,
> +					 &notmuch,
> +					 &message);
>      if (ret) {
> +	if (status_cb) status_cb (message, closure);
>  	goto DONE;
>      }
>  
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index f066245..c671d82 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -230,6 +230,16 @@ notmuch_status_t
>  notmuch_database_create (const char *path, notmuch_database_t **database);
>  
>  /**
> + * Like notmuch_database_create, except optionally return an error
> + * message. This message is allocated by malloc and should be freed by
> + * the caller.
> + */
> +notmuch_status_t
> +notmuch_database_create_verbose (const char *path,
> +				 notmuch_database_t **database,
> +				 char **error_message);
> +
> +/**
>   * Database open mode for notmuch_database_open.
>   */
>  typedef enum {
> @@ -279,6 +289,17 @@ notmuch_status_t
>  notmuch_database_open (const char *path,
>  		       notmuch_database_mode_t mode,
>  		       notmuch_database_t **database);
> +/**
> + * Like notmuch_database_open, except optionally return an error
> + * message. This message is allocated by malloc and should be freed by
> + * the caller.
> + */
> +
> +notmuch_status_t
> +notmuch_database_open_verbose (const char *path,
> +			       notmuch_database_mode_t mode,
> +			       notmuch_database_t **database,
> +			       char **error_message);
>  
>  /**
>   * Commit changes and close the given notmuch database.
> diff --git a/notmuch-new.c b/notmuch-new.c
> index ddf42c1..e6c283e 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -985,9 +985,16 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  	    return EXIT_FAILURE;
>  	add_files_state.total_files = count;
>      } else {
> -	if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
> -				   &notmuch))
> +	char *status_string = NULL;
> +	if (notmuch_database_open_verbose (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
> +					   &notmuch, &status_string)) {
> +	    if (status_string) {
> +		fputs (status_string, stderr);
> +		free (status_string);
> +	    }
> +
>  	    return EXIT_FAILURE;
> +	}
>  
>  	if (notmuch_database_needs_upgrade (notmuch)) {
>  	    time_t now = time (NULL);
> diff --git a/notmuch-search.c b/notmuch-search.c
> index a591d45..b81ac01 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -545,6 +545,7 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar
>  {
>      char *query_str;
>      unsigned int i;
> +    char *status_string = NULL;
>  
>      switch (ctx->format_sel) {
>      case NOTMUCH_FORMAT_TEXT:
> @@ -570,9 +571,17 @@ _notmuch_search_prepare (search_context_t *ctx, notmuch_config_t *config, int ar
>  
>      notmuch_exit_if_unsupported_format ();
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch))
> +    if (notmuch_database_open_verbose (
> +	    notmuch_config_get_database_path (config),
> +	    NOTMUCH_DATABASE_MODE_READ_ONLY, &ctx->notmuch, &status_string)) {
> +
> +	if (status_string) {
> +	    fputs (status_string, stderr);
> +	    free (status_string);
> +	}
> +
>  	return EXIT_FAILURE;
> +    }
>  
>      query_str = query_string_from_args (ctx->notmuch, argc, argv);
>      if (query_str == NULL) {
> diff --git a/test/symbol-test.cc b/test/symbol-test.cc
> index 3e96c03..d979f83 100644
> --- a/test/symbol-test.cc
> +++ b/test/symbol-test.cc
> @@ -1,11 +1,18 @@
>  #include <stdio.h>
> +#include <stdlib.h>
>  #include <xapian.h>
>  #include <notmuch.h>
>  
>  
>  int main() {
>    notmuch_database_t *notmuch;
> -  notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch);
> +  char *message = NULL;
> +
> +  if (notmuch_database_open_verbose  ("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch, &message))
> +      if (message) {
> +	  fputs (message, stderr);
> +	  free (message);
> +      }
>  
>    try {
>      (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open,create}
  2015-03-28 10:04   ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open, create} Tomi Ollila
@ 2015-03-28 23:46     ` David Bremner
  0 siblings, 0 replies; 11+ messages in thread
From: David Bremner @ 2015-03-28 23:46 UTC (permalink / raw)
  To: Tomi Ollila, notmuch

Tomi Ollila <tomi.ollila@iki.fi> writes:

>
> Just that this small piece of code above git pass my sparse sieve, perhaps
> this could be amended to:
>
>     if (message) {
>         if (status_string)
>             *status_string = message;
>         else 
>             free(message);
>     }
>

I amended and pushed. Thanks for all the reviewing.

d

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-03-28 23:47 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-27 22:11 Library logging overhaul, round 6 David Bremner
2015-03-27 22:11 ` [Patch v6 1/8] test: Add two tests for error output from notmuch_database_open David Bremner
2015-03-27 22:11 ` [Patch v6 2/8] test: add support for compiling and running C snippets David Bremner
2015-03-27 22:11 ` [Patch v6 3/8] test: add error reporting tests David Bremner
2015-03-27 22:11 ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open,create} David Bremner
2015-03-28 10:04   ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open, create} Tomi Ollila
2015-03-28 23:46     ` [Patch v6 4/8] lib: add "verbose" versions of notmuch_database_{open,create} David Bremner
2015-03-27 22:11 ` [Patch v6 5/8] lib: add a log function with output to a string in notmuch_database_t David Bremner
2015-03-27 22:11 ` [Patch v6 6/8] lib: add private function to extract the database for a message David Bremner
2015-03-27 22:11 ` [Patch v6 7/8] lib: replace almost all fprintfs in library with _n_d_log David Bremner
2015-03-27 22:12 ` [Patch v6 8/8] lib: eliminate fprintf from _notmuch_message_file_open David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).