diff --git a/cassandane/Cassandane/Cyrus/Delete.pm b/cassandane/Cassandane/Cyrus/Delete.pm index 6c34dea750..10f9fb6030 100644 --- a/cassandane/Cassandane/Cyrus/Delete.pm +++ b/cassandane/Cassandane/Cyrus/Delete.pm @@ -152,6 +152,15 @@ sub check_folder_not_ondisk } } +sub check_syslog +{ + my ($self, $instance) = @_; + + my $remove_empty_pat = qr/Remove of supposedly empty directory/; + + $self->assert_null($instance->_check_syslog($remove_empty_pat)); +} + sub test_self_inbox_imm :ImmediateDelete :SemidelayedExpunge :NoAltNameSpace { @@ -218,6 +227,8 @@ sub test_self_inbox_imm $self->check_folder_not_ondisk($subfolder); $self->check_folder_not_ondisk($inbox, deleted => 1); $self->check_folder_not_ondisk($subfolder, deleted => 1); + + $self->check_syslog($self->{instance}); } sub test_self_inbox_del @@ -293,6 +304,8 @@ sub test_self_inbox_del $self->check_folder_not_ondisk($subfolder); $self->check_folder_not_ondisk($inbox, deleted => 1); $self->check_folder_not_ondisk($subfolder, deleted => 1); + + $self->check_syslog($self->{instance}); } # old version of this test for builds without newer httpd features @@ -370,6 +383,8 @@ sub test_admin_inbox_imm_legacy $self->check_folder_not_ondisk($subfolder); $self->check_folder_not_ondisk($inbox, deleted => 1); $self->check_folder_not_ondisk($subfolder, deleted => 1); + + $self->check_syslog($self->{instance}); } sub test_admin_inbox_imm @@ -509,6 +524,8 @@ EOF } $self->assert_not_file_test($data->{user}{sieve}, '-e'); $self->assert_not_file_test($data->{xapian}{t1}, '-e'); + + $self->check_syslog($self->{instance}); } sub test_admin_inbox_del @@ -590,6 +607,8 @@ sub test_admin_inbox_del $self->check_folder_not_ondisk($subfolder); $self->check_folder_not_ondisk($inbox, deleted => 1); $self->check_folder_not_ondisk($subfolder, deleted => 1); + + $self->check_syslog($self->{instance}); } sub test_bz3781 @@ -699,6 +718,8 @@ sub test_cyr_expire_delete # and not exist from mbpath either... $self->assert_null($self->{instance}->folder_to_deleted_directories("user.cassandane.$subfoldername")); + + $self->check_syslog($self->{instance}); } sub test_allowdeleted @@ -741,6 +762,8 @@ sub test_allowdeleted $talk->select($result->[1][2]); $self->assert_str_equals('ok', $talk->get_last_completion_response()); $self->assert_num_equals(1, $talk->get_response_code('exists')); + + $self->check_syslog($self->{instance}); } sub test_cyr_expire_delete_with_annotation @@ -811,6 +834,8 @@ sub test_cyr_expire_delete_with_annotation xlog $self, "Run cyr_expire -D now, with -a, skipping annotation."; $self->{instance}->run_command({ cyrus => 1 }, 'cyr_expire', '-D' => '0', '-a' ); $self->assert_not_file_test($path, '-d'); + + $self->check_syslog($self->{instance}); } # https://github.com/cyrusimap/cyrus-imapd/issues/2413 @@ -883,6 +908,8 @@ sub test_cyr_expire_dont_resurrect_convdb # expect user does not have a conversations database $self->assert_not_file_test($convdbfile, '-f'); + + $self->check_syslog($self->{instance}); } sub test_no_delete_with_children @@ -906,6 +933,8 @@ sub test_no_delete_with_children $talk->delete($subfolder); $self->assert_str_equals('no', $talk->get_last_completion_response()); + + $self->check_syslog($self->{instance}); } sub test_cyr_expire_inherit_annot @@ -940,6 +969,8 @@ sub test_cyr_expire_inherit_annot $talk->unselect(); $talk->select($subfolder); $self->assert_num_equals(0, $talk->get_response_code('exists')); + + $self->check_syslog($self->{instance}); } sub test_cyr_expire_noexpire @@ -991,6 +1022,8 @@ sub test_cyr_expire_noexpire $talk->unselect(); $talk->select($subfolder); $self->assert_num_equals(0, $talk->get_response_code('exists')); + + $self->check_syslog($self->{instance}); } sub test_cyr_expire_delete_noexpire @@ -1048,6 +1081,8 @@ sub test_cyr_expire_delete_noexpire xlog $self, "Run cyr_expire"; $self->{instance}->run_command({ cyrus => 1 }, 'cyr_expire', '-D' => '1s' ); $self->assert(!-d "$path"); + + $self->check_syslog($self->{instance}); } 1; diff --git a/cassandane/Cassandane/Instance.pm b/cassandane/Cassandane/Instance.pm index 4f132a8015..352eaf40e5 100644 --- a/cassandane/Cassandane/Instance.pm +++ b/cassandane/Cassandane/Instance.pm @@ -1501,9 +1501,18 @@ sub _check_sanity sub _check_syslog { - my ($self) = @_; + my ($self, $pattern) = @_; + + if (defined $pattern) { + # pattern is optional but must be a regex if present + die "getsyslog: pattern is not a regular expression" + if lc ref($pattern) ne 'regexp'; + } - my @errors = $self->getsyslog(qr/ERROR|TRACELOG|Unknown code ____/); + my @lines = $self->getsyslog(); + my @errors = grep { + m/ERROR|TRACELOG|Unknown code ____/ || ($pattern && m/$pattern/) + } @lines; @errors = grep { not m/DBERROR.*skipstamp/ } @errors; diff --git a/imap/mailbox.c b/imap/mailbox.c index c8063826bf..fb4d420b6d 100644 --- a/imap/mailbox.c +++ b/imap/mailbox.c @@ -6085,7 +6085,7 @@ static void mailbox_delete_files(const char *path) } } -/* Callback for use by cmd_delete */ +/* Callback for use by mailbox_delete_cleanup */ static int chkchildren(const mbentry_t *mbentry, void *rock) { @@ -6412,7 +6412,11 @@ static struct meta_file meta_files[] = { * try to create a mailbox while the delete is underway. * VERY tight race condition exists right now... */ /* we need an exclusive namelock for this */ -HIDDEN int mailbox_delete_cleanup(struct mailbox *mailbox, const char *part, const char *name, const char *uniqueid) +/* XXX uniqueid here must be NULL if this is a legacy mailbox */ +HIDDEN int mailbox_delete_cleanup(struct mailbox *mailbox, + const char *part, + const char *name, + const char *uniqueid) { strarray_t paths = STRARRAY_INITIALIZER; int i; @@ -6432,7 +6436,7 @@ HIDDEN int mailbox_delete_cleanup(struct mailbox *mailbox, const char *part, con ntail = nbuf + strlen(nbuf); if (mailbox && object_storage_enabled){ - mailbox_remove_files_from_object_storage (mailbox, 0) ; + mailbox_remove_files_from_object_storage(mailbox, 0); } /* XXX - double XXX - this is a really ugly function. It should be @@ -6465,14 +6469,36 @@ HIDDEN int mailbox_delete_cleanup(struct mailbox *mailbox, const char *part, con for (i = 0; i < paths.count; i++) { char *path = paths.data[i]; /* need direct reference, because we're fiddling */ r = rmdir(path); - if (r && errno != ENOENT) + if (r && errno != ENOENT) { syslog(LOG_NOTICE, "Remove of supposedly empty directory %s failed: %m", path); - p = strrchr(path, '/'); - if (p) *p = '\0'; + } + + if (!uniqueid) { + p = strrchr(path, '/'); + if (p) *p = '\0'; + } } + if (uniqueid) { + /* n.b. careful here -- legacy mailboxes will also have uniqueids, + * but we don't pass the uniqueid to this function for those, so + * uniqueid being non-NULL here means we're dealing with a UUID + * mailbox. + * UUID mailboxes have a flat on disk structure, so once we've + * removed the listed paths we're done, no hierarchy walk needed. + */ + goto done; + } + + /* XXX For legacy mailboxes, this hierarchy walk might have been + * XXX intended to remove intermediate mailboxes after their + * XXX last child was removed. In which case, the hierarchy walk + * XXX might be obsolete for legacy mailboxes too, since we no + * XXX longer create intermediate mailboxes. + */ + /* Check if parent mailbox exists */ ntail = strrchr(nbuf, '.'); if (!ntail || strchr(ntail, '!')) { @@ -6496,6 +6522,7 @@ HIDDEN int mailbox_delete_cleanup(struct mailbox *mailbox, const char *part, con } } while (r == IMAP_MAILBOX_NONEXISTENT); +done: strarray_fini(&paths); return 0;