Mailbox item iterator pattern

[Bron Gondwana]

Hi,

I'm investigating restoring from backups as part of our
new backup system, and one problem is that recovering
seen state is basically impossible once a message has
been "expunged", even with delayed deletion.  This is
because index_checkseen walks the UID list in the
mailbox and joins across the gaps.

There are also other places with problems:

sync_client/sync_server - doesn't copy messages which
have been expunged.

mailbox_rename_copy - doesn't move messages or the
cyrus.expunge file.

Basically it looks like you need to open both files at
once and step through them - reading whichever is lowest
next.

I really, _really_ think that hacking that directly in to
every place that needs it is a bad idea for maintainability,
so I'd like to propose refactoring all the places that
iterate over a mailbox.


I've CC'ed a few people who I know are active patchers/
maintainers of Cyrus and who I've spoken with before
about Cyrus changes - but I'd especially appreciate
input from Ken about how best to do this.  The current
pattern I'm leaning towards is:


struct index_iter *iter;
struct index_record *record;
int flags = INBOX_ITER_EXPUNGED;

iter = mailbox_iterator_create(mailbox, flags);

while (record = mailbox_iterator_next(&iter)) {
  /* process things the same way you would have with the pattern:
     for (msgno = 1; msgno <= mailbox->exists; msgno++) {
         r = mailbox_read_index_record(mailbox, msgno, &record);
   */
}

r = mailbox_iterator_haserror(&iter);

mailbox_iterator_free(&iter);

return(r);

Obviously, if there was an error then mailbox_iterator_next will
return NULL, breaking the loop early.  You'll know what the
error is because 'r' gets set.


An alternative is:

struct index_iter iter;
struct index_record record;

mailbox_iterator_init(&iter, mailbox);
// or mailbox_iterator_initexpunge(&iter, mailbox);

while (mailbox_iterator_has_more_records(&iter)) {
  r = mailbox_iterator_next_record(&iter, &record);
  if (r) {
    mailbox_iterator_free(&iter);
    return r;
  }
  /* process */
}
mailbox_iterator_free(&iter);


I've deliberately left mailbox_iter unformatted, but it would
probably contain at least two index_record buffers, a locked
mmaped fd for each of mailbox and expunge files and some sort
of state tracking mechanism to say which one is next!

What do you think?  Is there value in either of the above?
Can you see a better way?

Bron.