Merge pull request #4764 from elliefm/v39/supposedly-empty-directory

mailbox: no hierarchy walk when deleting uuid mailboxes

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;

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;
 

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;