* compactor adjustments v2
@ 2013-11-13 17:02 Tomi Ollila
2013-11-13 17:02 ` [PATCH v2 1/5] compact: tidy formatting Tomi Ollila
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Tomi Ollila @ 2013-11-13 17:02 UTC (permalink / raw)
To: notmuch; +Cc: tomi.ollila
This is v2 of id:1384192538-15291-1-git-send-email-tomi.ollila@iki.fi
Changes include:
tidy formatting (i.e. "uncrustifying") on related code
catching Xapian::Error (always)
The tests done in id:m2fvr1tpkf.fsf@guru.guru-group.fi were redone
with one extra (*) -- results same. Automatic tests pass.
(*)
$ mkdir xapian.compact
$ touch xapian.compact/file
$ chmod 000 xapian.compact
$ notmuch compact
Compacting database...
compacting table postlist
Error while compacting: Couldn't open base /path/to/.notmuch/xapian.compact/postlist.baseA to write: Permission denied
Compaction failed: A Xapian exception occurred
zsh: exit 1 ./notmuch compact
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 1/5] compact: tidy formatting 2013-11-13 17:02 compactor adjustments v2 Tomi Ollila @ 2013-11-13 17:02 ` Tomi Ollila 2013-11-13 17:02 ` [PATCH v2 2/5] compact: catch Xapian::Error consistently Tomi Ollila ` (3 subsequent siblings) 4 siblings, 0 replies; 16+ messages in thread From: Tomi Ollila @ 2013-11-13 17:02 UTC (permalink / raw) To: notmuch; +Cc: tomi.ollila Notmuch compact code whitespace changes to match devel/STYLE. --- lib/database.cc | 62 ++++++++++++++++++++++++++++--------------------------- notmuch-compact.c | 4 ++-- 2 files changed, 34 insertions(+), 32 deletions(-) diff --git a/lib/database.cc b/lib/database.cc index a021bf1..3c008d6 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -805,17 +805,19 @@ notmuch_database_close (notmuch_database_t *notmuch) } #if HAVE_XAPIAN_COMPACT -static int unlink_cb (const char *path, - unused (const struct stat *sb), - unused (int type), - unused (struct FTW *ftw)) +static int +unlink_cb (const char *path, + unused (const struct stat *sb), + unused (int type), + unused (struct FTW *ftw)) { - return remove(path); + return remove (path); } -static int rmtree (const char *path) +static int +rmtree (const char *path) { - return nftw(path, unlink_cb, 64, FTW_DEPTH | FTW_PHYS); + return nftw (path, unlink_cb, 64, FTW_DEPTH | FTW_PHYS); } class NotmuchCompactor : public Xapian::Compactor @@ -825,17 +827,17 @@ class NotmuchCompactor : public Xapian::Compactor public: NotmuchCompactor(notmuch_compact_status_cb_t cb, void *closure) : - status_cb(cb), status_closure(closure) { } + status_cb (cb), status_closure (closure) { } virtual void set_status (const std::string &table, const std::string &status) { - char* msg; + char *msg; if (status_cb == NULL) return; - if (status.length() == 0) + if (status.length () == 0) msg = talloc_asprintf (NULL, "compacting table %s", table.c_str()); else msg = talloc_asprintf (NULL, " %s", status.c_str()); @@ -844,8 +846,8 @@ public: return; } - status_cb(msg, status_closure); - talloc_free(msg); + status_cb (msg, status_closure); + talloc_free (msg); } }; @@ -861,8 +863,8 @@ public: * compaction process to protect data integrity. */ notmuch_status_t -notmuch_database_compact (const char* path, - const char* backup_path, +notmuch_database_compact (const char *path, + const char *backup_path, notmuch_compact_status_cb_t status_cb, void *closure) { @@ -876,7 +878,7 @@ notmuch_database_compact (const char* path, if (! local) return NOTMUCH_STATUS_OUT_OF_MEMORY; - ret = notmuch_database_open(path, NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much); + ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much); if (ret) { goto DONE; } @@ -897,25 +899,25 @@ notmuch_database_compact (const char* path, } if (backup_path != NULL) { - if (stat(backup_path, &statbuf) != -1) { + if (stat (backup_path, &statbuf) != -1) { fprintf (stderr, "Backup path already exists: %s\n", backup_path); ret = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } if (errno != ENOENT) { fprintf (stderr, "Unknown error while stat()ing backup path: %s\n", - strerror(errno)); + strerror (errno)); goto DONE; } } try { - NotmuchCompactor compactor(status_cb, closure); + NotmuchCompactor compactor (status_cb, closure); - compactor.set_renumber(false); - compactor.add_source(xapian_path); - compactor.set_destdir(compact_xapian_path); - compactor.compact(); + compactor.set_renumber (false); + compactor.add_source (xapian_path); + compactor.set_destdir (compact_xapian_path); + compactor.compact (); } catch (Xapian::InvalidArgumentError e) { fprintf (stderr, "Error while compacting: %s\n", e.get_msg().c_str()); ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION; @@ -923,33 +925,33 @@ notmuch_database_compact (const char* path, } if (backup_path) { - if (rename(xapian_path, backup_path)) { + if (rename (xapian_path, backup_path)) { fprintf (stderr, "Error moving old database out of the way\n"); ret = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } } else { - rmtree(xapian_path); + rmtree (xapian_path); } - if (rename(compact_xapian_path, xapian_path)) { + if (rename (compact_xapian_path, xapian_path)) { fprintf (stderr, "Error moving compacted database\n"); ret = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } -DONE: + DONE: if (notmuch) notmuch_database_destroy (notmuch); - talloc_free(local); + talloc_free (local); return ret; } #else notmuch_status_t -notmuch_database_compact (unused (const char* path), - unused (const char* backup_path), +notmuch_database_compact (unused (const char *path), + unused (const char *backup_path), unused (notmuch_compact_status_cb_t status_cb), unused (void *closure)) { @@ -1538,7 +1540,7 @@ _notmuch_database_generate_doc_id (notmuch_database_t *notmuch) notmuch->last_doc_id++; if (notmuch->last_doc_id == 0) - INTERNAL_ERROR ("Xapian document IDs are exhausted.\n"); + INTERNAL_ERROR ("Xapian document IDs are exhausted.\n"); return notmuch->last_doc_id; } diff --git a/notmuch-compact.c b/notmuch-compact.c index 8022dfe..8b820c0 100644 --- a/notmuch-compact.c +++ b/notmuch-compact.c @@ -23,7 +23,7 @@ static void status_update_cb (const char *msg, unused (void *closure)) { - printf("%s\n", msg); + printf ("%s\n", msg); } int @@ -49,7 +49,7 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[]) ret = notmuch_database_compact (path, backup_path, quiet ? NULL : status_update_cb, NULL); if (ret) { - fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string(ret)); + fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string (ret)); return 1; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/5] compact: catch Xapian::Error consistently 2013-11-13 17:02 compactor adjustments v2 Tomi Ollila 2013-11-13 17:02 ` [PATCH v2 1/5] compact: tidy formatting Tomi Ollila @ 2013-11-13 17:02 ` Tomi Ollila 2013-11-14 14:13 ` Jani Nikula 2013-11-18 0:33 ` David Bremner 2013-11-13 17:02 ` [PATCH v2 3/5] compact: preserve backup database until compacted database is in place Tomi Ollila ` (2 subsequent siblings) 4 siblings, 2 replies; 16+ messages in thread From: Tomi Ollila @ 2013-11-13 17:02 UTC (permalink / raw) To: notmuch; +Cc: tomi.ollila catch Xapian::Error in compact code in lib/database.cc to be consistent with other code in addition to not making software crash on uncaught other Xapian error. --- lib/database.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/database.cc b/lib/database.cc index 3c008d6..3530cb6 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -918,8 +918,8 @@ notmuch_database_compact (const char *path, compactor.add_source (xapian_path); compactor.set_destdir (compact_xapian_path); compactor.compact (); - } catch (Xapian::InvalidArgumentError e) { - fprintf (stderr, "Error while compacting: %s\n", e.get_msg().c_str()); + } catch (const Xapian::Error &error) { + fprintf (stderr, "Error while compacting: %s\n", error.get_msg().c_str()); ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION; goto DONE; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] compact: catch Xapian::Error consistently 2013-11-13 17:02 ` [PATCH v2 2/5] compact: catch Xapian::Error consistently Tomi Ollila @ 2013-11-14 14:13 ` Jani Nikula 2013-11-18 0:33 ` David Bremner 1 sibling, 0 replies; 16+ messages in thread From: Jani Nikula @ 2013-11-14 14:13 UTC (permalink / raw) To: Tomi Ollila, notmuch; +Cc: tomi.ollila The first two patches LGTM. On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote: > catch Xapian::Error in compact code in lib/database.cc to be consistent > with other code in addition to not making software crash on uncaught > other Xapian error. > --- > lib/database.cc | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/database.cc b/lib/database.cc > index 3c008d6..3530cb6 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -918,8 +918,8 @@ notmuch_database_compact (const char *path, > compactor.add_source (xapian_path); > compactor.set_destdir (compact_xapian_path); > compactor.compact (); > - } catch (Xapian::InvalidArgumentError e) { > - fprintf (stderr, "Error while compacting: %s\n", e.get_msg().c_str()); > + } catch (const Xapian::Error &error) { > + fprintf (stderr, "Error while compacting: %s\n", error.get_msg().c_str()); > ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION; > goto DONE; > } > -- > 1.8.3.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] compact: catch Xapian::Error consistently 2013-11-13 17:02 ` [PATCH v2 2/5] compact: catch Xapian::Error consistently Tomi Ollila 2013-11-14 14:13 ` Jani Nikula @ 2013-11-18 0:33 ` David Bremner 1 sibling, 0 replies; 16+ messages in thread From: David Bremner @ 2013-11-18 0:33 UTC (permalink / raw) To: Tomi Ollila, notmuch; +Cc: tomi.ollila Tomi Ollila <tomi.ollila@iki.fi> writes: > catch Xapian::Error in compact code in lib/database.cc to be consistent > with other code in addition to not making software crash on uncaught > other Xapian error. pushed the first two patches d ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/5] compact: preserve backup database until compacted database is in place 2013-11-13 17:02 compactor adjustments v2 Tomi Ollila 2013-11-13 17:02 ` [PATCH v2 1/5] compact: tidy formatting Tomi Ollila 2013-11-13 17:02 ` [PATCH v2 2/5] compact: catch Xapian::Error consistently Tomi Ollila @ 2013-11-13 17:02 ` Tomi Ollila 2013-11-14 14:08 ` Jani Nikula 2013-11-13 17:02 ` [PATCH v2 4/5] compact: unconditionally attempt to remove old wip database compact directory Tomi Ollila 2013-11-13 17:02 ` [PATCH v2 5/5] compact: provide user more information on after-compaction failures Tomi Ollila 4 siblings, 1 reply; 16+ messages in thread From: Tomi Ollila @ 2013-11-13 17:02 UTC (permalink / raw) To: notmuch; +Cc: tomi.ollila It is less error prone and window of failure opportunity is smaller if the old (backup) database is always renamed (instead of sometimes rmtree'd) before new (compacted) database is put into its place. Finally rmtree() old database in case old database backup is not kept. --- lib/database.cc | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/lib/database.cc b/lib/database.cc index 3530cb6..ee09c5e 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -873,6 +873,7 @@ notmuch_database_compact (const char *path, notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS; notmuch_database_t *notmuch = NULL; struct stat statbuf; + notmuch_bool_t keep_backup; local = talloc_new (NULL); if (! local) @@ -898,17 +899,25 @@ notmuch_database_compact (const char *path, goto DONE; } - if (backup_path != NULL) { - if (stat (backup_path, &statbuf) != -1) { - fprintf (stderr, "Backup path already exists: %s\n", backup_path); - ret = NOTMUCH_STATUS_FILE_ERROR; - goto DONE; - } - if (errno != ENOENT) { - fprintf (stderr, "Unknown error while stat()ing backup path: %s\n", - strerror (errno)); + if (backup_path == NULL) { + if (! (backup_path = talloc_asprintf (local, "%s.old", xapian_path))) { + ret = NOTMUCH_STATUS_OUT_OF_MEMORY; goto DONE; } + keep_backup = FALSE; + } + else + keep_backup = TRUE; + + if (stat (backup_path, &statbuf) != -1) { + fprintf (stderr, "Backup path already exists: %s\n", backup_path); + ret = NOTMUCH_STATUS_FILE_ERROR; + goto DONE; + } + if (errno != ENOENT) { + fprintf (stderr, "Unknown error while stat()ing backup path: %s\n", + strerror (errno)); + goto DONE; } try { @@ -924,14 +933,10 @@ notmuch_database_compact (const char *path, goto DONE; } - if (backup_path) { - if (rename (xapian_path, backup_path)) { - fprintf (stderr, "Error moving old database out of the way\n"); - ret = NOTMUCH_STATUS_FILE_ERROR; - goto DONE; - } - } else { - rmtree (xapian_path); + if (rename (xapian_path, backup_path)) { + fprintf (stderr, "Error moving old database out of the way\n"); + ret = NOTMUCH_STATUS_FILE_ERROR; + goto DONE; } if (rename (compact_xapian_path, xapian_path)) { @@ -940,6 +945,9 @@ notmuch_database_compact (const char *path, goto DONE; } + if (! keep_backup) + rmtree (backup_path); + DONE: if (notmuch) notmuch_database_destroy (notmuch); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/5] compact: preserve backup database until compacted database is in place 2013-11-13 17:02 ` [PATCH v2 3/5] compact: preserve backup database until compacted database is in place Tomi Ollila @ 2013-11-14 14:08 ` Jani Nikula 0 siblings, 0 replies; 16+ messages in thread From: Jani Nikula @ 2013-11-14 14:08 UTC (permalink / raw) To: Tomi Ollila, notmuch; +Cc: tomi.ollila On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote: > It is less error prone and window of failure opportunity is smaller > if the old (backup) database is always renamed (instead of sometimes > rmtree'd) before new (compacted) database is put into its place. > Finally rmtree() old database in case old database backup is not kept. > --- > lib/database.cc | 42 +++++++++++++++++++++++++----------------- > 1 file changed, 25 insertions(+), 17 deletions(-) > > diff --git a/lib/database.cc b/lib/database.cc > index 3530cb6..ee09c5e 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -873,6 +873,7 @@ notmuch_database_compact (const char *path, > notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS; > notmuch_database_t *notmuch = NULL; > struct stat statbuf; > + notmuch_bool_t keep_backup; > > local = talloc_new (NULL); > if (! local) > @@ -898,17 +899,25 @@ notmuch_database_compact (const char *path, > goto DONE; > } > > - if (backup_path != NULL) { > - if (stat (backup_path, &statbuf) != -1) { > - fprintf (stderr, "Backup path already exists: %s\n", backup_path); > - ret = NOTMUCH_STATUS_FILE_ERROR; > - goto DONE; > - } > - if (errno != ENOENT) { > - fprintf (stderr, "Unknown error while stat()ing backup path: %s\n", > - strerror (errno)); > + if (backup_path == NULL) { > + if (! (backup_path = talloc_asprintf (local, "%s.old", xapian_path))) { > + ret = NOTMUCH_STATUS_OUT_OF_MEMORY; > goto DONE; > } > + keep_backup = FALSE; > + } > + else > + keep_backup = TRUE; *cough* I thought you were the style police around here *cough* > + > + if (stat (backup_path, &statbuf) != -1) { > + fprintf (stderr, "Backup path already exists: %s\n", backup_path); The user will be confused if he specifically didn't add --backup and happens to get this. But maybe it's a corner case. *shrug*. > + ret = NOTMUCH_STATUS_FILE_ERROR; > + goto DONE; > + } > + if (errno != ENOENT) { > + fprintf (stderr, "Unknown error while stat()ing backup path: %s\n", > + strerror (errno)); ret = ? > + goto DONE; > } > > try { > @@ -924,14 +933,10 @@ notmuch_database_compact (const char *path, > goto DONE; > } > > - if (backup_path) { > - if (rename (xapian_path, backup_path)) { > - fprintf (stderr, "Error moving old database out of the way\n"); > - ret = NOTMUCH_STATUS_FILE_ERROR; > - goto DONE; > - } > - } else { > - rmtree (xapian_path); > + if (rename (xapian_path, backup_path)) { > + fprintf (stderr, "Error moving old database out of the way\n"); > + ret = NOTMUCH_STATUS_FILE_ERROR; > + goto DONE; > } > > if (rename (compact_xapian_path, xapian_path)) { > @@ -940,6 +945,9 @@ notmuch_database_compact (const char *path, > goto DONE; > } > > + if (! keep_backup) > + rmtree (backup_path); > + > DONE: > if (notmuch) > notmuch_database_destroy (notmuch); > -- > 1.8.3.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 4/5] compact: unconditionally attempt to remove old wip database compact directory 2013-11-13 17:02 compactor adjustments v2 Tomi Ollila ` (2 preceding siblings ...) 2013-11-13 17:02 ` [PATCH v2 3/5] compact: preserve backup database until compacted database is in place Tomi Ollila @ 2013-11-13 17:02 ` Tomi Ollila 2013-11-14 14:02 ` Jani Nikula 2013-11-13 17:02 ` [PATCH v2 5/5] compact: provide user more information on after-compaction failures Tomi Ollila 4 siblings, 1 reply; 16+ messages in thread From: Tomi Ollila @ 2013-11-13 17:02 UTC (permalink / raw) To: notmuch; +Cc: tomi.ollila In case previous notmuch compact has been interrupted there is old work-in-progress database compact directory partially filled. Remove it just before starting to fill the directory with new files. --- lib/database.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/database.cc b/lib/database.cc index ee09c5e..4b5ac64 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -920,6 +920,11 @@ notmuch_database_compact (const char *path, goto DONE; } + // Unconditionally attempt to remove old work-in-progress database (if any). + // This is "protected" by database lock. If this fails due to write errors + // (etc), the following code will fail and provide error message. + (void) rmtree (compact_xapian_path); + try { NotmuchCompactor compactor (status_cb, closure); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/5] compact: unconditionally attempt to remove old wip database compact directory 2013-11-13 17:02 ` [PATCH v2 4/5] compact: unconditionally attempt to remove old wip database compact directory Tomi Ollila @ 2013-11-14 14:02 ` Jani Nikula 0 siblings, 0 replies; 16+ messages in thread From: Jani Nikula @ 2013-11-14 14:02 UTC (permalink / raw) To: Tomi Ollila, notmuch; +Cc: tomi.ollila On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote: > In case previous notmuch compact has been interrupted there is old > work-in-progress database compact directory partially filled. Remove > it just before starting to fill the directory with new files. > --- > lib/database.cc | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/lib/database.cc b/lib/database.cc > index ee09c5e..4b5ac64 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -920,6 +920,11 @@ notmuch_database_compact (const char *path, > goto DONE; > } > > + // Unconditionally attempt to remove old work-in-progress database (if any). > + // This is "protected" by database lock. If this fails due to write errors > + // (etc), the following code will fail and provide error message. > + (void) rmtree (compact_xapian_path); I thought we avoid using // comments. Otherwise LGTM. > + > try { > NotmuchCompactor compactor (status_cb, closure); > > -- > 1.8.3.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 5/5] compact: provide user more information on after-compaction failures 2013-11-13 17:02 compactor adjustments v2 Tomi Ollila ` (3 preceding siblings ...) 2013-11-13 17:02 ` [PATCH v2 4/5] compact: unconditionally attempt to remove old wip database compact directory Tomi Ollila @ 2013-11-13 17:02 ` Tomi Ollila 2013-11-14 14:13 ` Jani Nikula 4 siblings, 1 reply; 16+ messages in thread From: Tomi Ollila @ 2013-11-13 17:02 UTC (permalink / raw) To: notmuch; +Cc: tomi.ollila After database has been compacted, there are steps to put the new database into place -- and these steps may fail. In case such failure happens, provide better information how to resolve it. Thanks to Ben Gamari for most of the information content. --- lib/database.cc | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/lib/database.cc b/lib/database.cc index 4b5ac64..a6daac6 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -939,19 +939,50 @@ notmuch_database_compact (const char *path, } if (rename (xapian_path, backup_path)) { - fprintf (stderr, "Error moving old database out of the way\n"); + fprintf (stderr, "Error moving old database out of the way:\n" + "Old database: %s\n" + "Backup database: %s\n" + "Error: %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 compacted database\n"); + fprintf (stderr, "Error moving compacted database into place: %s\n", + strerror (errno)); + fprintf (stderr, "\n" + "Encountered error while moving the compacted database\n" + "\n" + " %s\n" + "\n" + "to\n" + "\n" + " %s\n" + "\n" + "Please identify the reason for this and move the compacted database\n" + "into place manually.\n" + "\n" + "Alternatively you can revert to the uncompacted database with\n" + "\n" + " mv '%s' '%s'\n" + "\n", compact_xapian_path, xapian_path, + backup_path, xapian_path); ret = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } - if (! keep_backup) - rmtree (backup_path); + if (! keep_backup) { + if (rmtree (backup_path)) { + fprintf (stderr, "Error removing backup database: %s\n", + strerror (errno)); + fprintf (stderr, "\n" + "Please remove the backup database with\n" + "\n" + " rm -rf '%s'\n" "\n", backup_path); + ret = NOTMUCH_STATUS_FILE_ERROR; + goto DONE; + } + } DONE: if (notmuch) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures 2013-11-13 17:02 ` [PATCH v2 5/5] compact: provide user more information on after-compaction failures Tomi Ollila @ 2013-11-14 14:13 ` Jani Nikula 2013-11-14 16:11 ` David Bremner 0 siblings, 1 reply; 16+ messages in thread From: Jani Nikula @ 2013-11-14 14:13 UTC (permalink / raw) To: Tomi Ollila, notmuch; +Cc: tomi.ollila On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote: > After database has been compacted, there are steps to put the new > database into place -- and these steps may fail. In case such > failure happens, provide better information how to resolve it. I disagree with having a library spew all this information out. For each case, I think it should be sufficient to just say what happened (e.g. "rename a -> b failed" + strerror). I don't think a library's error messages should be a tutorial on how to fix things. We may need to amend notmuch compact man page though. BR, Jani. > > Thanks to Ben Gamari for most of the information content. > --- > lib/database.cc | 39 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 35 insertions(+), 4 deletions(-) > > diff --git a/lib/database.cc b/lib/database.cc > index 4b5ac64..a6daac6 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -939,19 +939,50 @@ notmuch_database_compact (const char *path, > } > > if (rename (xapian_path, backup_path)) { > - fprintf (stderr, "Error moving old database out of the way\n"); > + fprintf (stderr, "Error moving old database out of the way:\n" > + "Old database: %s\n" > + "Backup database: %s\n" > + "Error: %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 compacted database\n"); > + fprintf (stderr, "Error moving compacted database into place: %s\n", > + strerror (errno)); > + fprintf (stderr, "\n" > + "Encountered error while moving the compacted database\n" > + "\n" > + " %s\n" > + "\n" > + "to\n" > + "\n" > + " %s\n" > + "\n" > + "Please identify the reason for this and move the compacted database\n" > + "into place manually.\n" > + "\n" > + "Alternatively you can revert to the uncompacted database with\n" > + "\n" > + " mv '%s' '%s'\n" > + "\n", compact_xapian_path, xapian_path, > + backup_path, xapian_path); > ret = NOTMUCH_STATUS_FILE_ERROR; > goto DONE; > } > > - if (! keep_backup) > - rmtree (backup_path); > + if (! keep_backup) { > + if (rmtree (backup_path)) { > + fprintf (stderr, "Error removing backup database: %s\n", > + strerror (errno)); > + fprintf (stderr, "\n" > + "Please remove the backup database with\n" > + "\n" > + " rm -rf '%s'\n" "\n", backup_path); > + ret = NOTMUCH_STATUS_FILE_ERROR; > + goto DONE; > + } > + } > > DONE: > if (notmuch) > -- > 1.8.3.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures 2013-11-14 14:13 ` Jani Nikula @ 2013-11-14 16:11 ` David Bremner 2013-11-14 17:23 ` Tomi Ollila 0 siblings, 1 reply; 16+ messages in thread From: David Bremner @ 2013-11-14 16:11 UTC (permalink / raw) To: Jani Nikula, Tomi Ollila, notmuch; +Cc: tomi.ollila Jani Nikula <jani@nikula.org> writes: > On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote: >> After database has been compacted, there are steps to put the new >> database into place -- and these steps may fail. In case such >> failure happens, provide better information how to resolve it. > > I disagree with having a library spew all this information out. For each > case, I think it should be sufficient to just say what happened > (e.g. "rename a -> b failed" + strerror). I don't think a library's > error messages should be a tutorial on how to fix things. > I can live with whatever the concensus level of verbosity, but not the fprintf's; as I mentioned, for once we have a log hook. That might also somewhat comfort Jani, since the library client has the option of ignoring the spewing. d ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures 2013-11-14 16:11 ` David Bremner @ 2013-11-14 17:23 ` Tomi Ollila 2013-11-17 11:34 ` David Bremner 0 siblings, 1 reply; 16+ messages in thread From: Tomi Ollila @ 2013-11-14 17:23 UTC (permalink / raw) To: David Bremner, Jani Nikula, notmuch On Thu, Nov 14 2013, David Bremner <david@tethera.net> wrote: > Jani Nikula <jani@nikula.org> writes: > >> On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote: >>> After database has been compacted, there are steps to put the new >>> database into place -- and these steps may fail. In case such >>> failure happens, provide better information how to resolve it. >> >> I disagree with having a library spew all this information out. For each >> case, I think it should be sufficient to just say what happened >> (e.g. "rename a -> b failed" + strerror). I don't think a library's >> error messages should be a tutorial on how to fix things. >> > > I can live with whatever the concensus level of verbosity, but not the > fprintf's; as I mentioned, for once we have a log hook. Jani is right there that *library* should not dump all that info to ... stderr. If so much info is printed, the client code should do that -- but doing that cleanly is not a trivial job. The idea of amending the namual page to inform user is a good one. The log hook in it's current form is problematic as it doesn't provide way to distinguish progress reporting from error reporting. Currently lib/database.cc writes error messages with fprintf(stderr, ...) everywhere. I suggest that this problem is fixed in one big sweep during 0.18 development -- the suggestion Jani pastebin'd a few days ago is a good one and I'm willing to take part of that development... And now take this approach of fprintf()ing (basically I would also ask developers using the library wait for 0.18 before starting to use the compact functionality (if ever), as the we have yet another soname bump with changing interface coming... Just that the log interface needs to be in format void (*log_cb)(void * closure, int level, const char * message) And we need to decide between the (non-NIH) syslog levels vs. our own levels... This way we can distinguish debug (DEBUG), progress (NOTICE, INFO) and error (WARN, EER, CRIT, ALERT, EMERG) conditions. For someone who wished logf(closure, level, fmt, ...) we can provide helper functions _log_printf(log_cb, closure, level, fmt, ...) and _log_vprintf(log_cb, closure, level, fmt, ap) > > That might also somewhat comfort Jani, since the library client has the > option of ignoring the spewing. String maybe! > > d Tomi ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures 2013-11-14 17:23 ` Tomi Ollila @ 2013-11-17 11:34 ` David Bremner 2013-11-17 15:28 ` Tomi Ollila 0 siblings, 1 reply; 16+ messages in thread From: David Bremner @ 2013-11-17 11:34 UTC (permalink / raw) To: Tomi Ollila, Jani Nikula, notmuch Tomi Ollila <tomi.ollila@iki.fi> writes: > The log hook in it's current form is problematic as it doesn't provide > way to distinguish progress reporting from error reporting. Is this _more_ problematic than more output to stderr? > Currently > lib/database.cc writes error messages with fprintf(stderr, ...) everywhere. Sure. But I'm trying to understand why a partial fix isn't better than nothing. Is the argument just that the effort is wasted, or that the result is somehow less satisfactory than the status quo. > I suggest that this problem is fixed in one big sweep during 0.18 > development -- the suggestion Jani pastebin'd a few days ago is > a good one and I'm willing to take part of that development... > And now take this approach of fprintf()ing (basically I would > also ask developers using the library wait for 0.18 before starting > to use the compact functionality (if ever), as the we have yet > another soname bump with changing interface coming... I guess we can mark this interface as unstable for the moment? "Asking developers not to use it" sounds pretty bad. d ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures 2013-11-17 11:34 ` David Bremner @ 2013-11-17 15:28 ` Tomi Ollila 2013-11-18 0:53 ` David Bremner 0 siblings, 1 reply; 16+ messages in thread From: Tomi Ollila @ 2013-11-17 15:28 UTC (permalink / raw) To: David Bremner, Jani Nikula, notmuch On Sun, Nov 17 2013, David Bremner <david@tethera.net> wrote: > Tomi Ollila <tomi.ollila@iki.fi> writes: > >> The log hook in it's current form is problematic as it doesn't provide >> way to distinguish progress reporting from error reporting. > > Is this _more_ problematic than more output to stderr? > >> Currently >> lib/database.cc writes error messages with fprintf(stderr, ...) everywhere. > > Sure. But I'm trying to understand why a partial fix isn't better than > nothing. Is the argument just that the effort is wasted, or that the > result is somehow less satisfactory than the status quo. The partial fix (using current log hook) would mean we either write all messages to stdout or to stderr. To the end user this would mean inconsistent behaviour in compact compared to other commands (like 'notmuch new' which prints progress to stdout and errors to stderr). I personally think doing things this way for 0.17 is tolerable (considering current schedule and risks involved) so that user experience is stable. >> I suggest that this problem is fixed in one big sweep during 0.18 >> development -- the suggestion Jani pastebin'd a few days ago is >> a good one and I'm willing to take part of that development... >> And now take this approach of fprintf()ing (basically I would >> also ask developers using the library wait for 0.18 before starting >> to use the compact functionality (if ever), as the we have yet >> another soname bump with changing interface coming... > > I guess we can mark this interface as unstable for the moment? > "Asking developers not to use it" sounds pretty bad. I was thinking naming the function notmuch_database_compact_internal () as one option (I also though of notmuch_database_compact_unstable () -- but that sounds so "unstable" (at least outside Debian people ;D)) -- could be one option. Then developers should understand the API is not fixed there... Although "Informing developers to prepare for upcoming changes to the API" (which is goind to happen early on 0.18 development cycle) should suffice. > d Tomi ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures 2013-11-17 15:28 ` Tomi Ollila @ 2013-11-18 0:53 ` David Bremner 0 siblings, 0 replies; 16+ messages in thread From: David Bremner @ 2013-11-18 0:53 UTC (permalink / raw) To: Tomi Ollila, notmuch Tomi Ollila <tomi.ollila@iki.fi> writes: > I was thinking naming the function notmuch_database_compact_internal () > as one option (I also though of notmuch_database_compact_unstable () -- but > that sounds so "unstable" (at least outside Debian people ;D)) -- > could be one option. Then developers should understand the API is not > fixed there... I think 0.18 will just be an API breaking release in general; compact is probably the least of people's worries. Yes, it's a bit more irritating because the API will be short lived, but since we plan to change notmuch_database_open, pretty much every client of the library will have to update anyway. Luckily there probably aren't enough such clients to get a really good lynch mob together. I'd suggest just mentioning it NEWS for 0.17 that the library interface for compact is expected to change in 0.18. d ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-11-18 0:53 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-13 17:02 compactor adjustments v2 Tomi Ollila 2013-11-13 17:02 ` [PATCH v2 1/5] compact: tidy formatting Tomi Ollila 2013-11-13 17:02 ` [PATCH v2 2/5] compact: catch Xapian::Error consistently Tomi Ollila 2013-11-14 14:13 ` Jani Nikula 2013-11-18 0:33 ` David Bremner 2013-11-13 17:02 ` [PATCH v2 3/5] compact: preserve backup database until compacted database is in place Tomi Ollila 2013-11-14 14:08 ` Jani Nikula 2013-11-13 17:02 ` [PATCH v2 4/5] compact: unconditionally attempt to remove old wip database compact directory Tomi Ollila 2013-11-14 14:02 ` Jani Nikula 2013-11-13 17:02 ` [PATCH v2 5/5] compact: provide user more information on after-compaction failures Tomi Ollila 2013-11-14 14:13 ` Jani Nikula 2013-11-14 16:11 ` David Bremner 2013-11-14 17:23 ` Tomi Ollila 2013-11-17 11:34 ` David Bremner 2013-11-17 15:28 ` Tomi Ollila 2013-11-18 0:53 ` 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).