Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mboxlist root mailboxid #3571

Closed
wants to merge 3 commits into from
Closed

Mboxlist root mailboxid #3571

wants to merge 3 commits into from

Conversation

ksmurchison
Copy link
Contributor

@ksmurchison ksmurchison commented Jul 22, 2021

Add a 'R' (root mailboxid) field to mbentry records.

Also add 's' option to ctl_mboxlist to ass 'R' field to existing records

Copy link
Member

@brong brong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of little bits of edge-case feedback here, but it's looking like it'll be useful soon :)

The big question is whether we want to version the mailboxes.db with a single key rather than using the presence of AS as the upgraded marker and needing yet another upgraded marker later. This would allow us to rev the database with more improvements without needing this upgrade magic each time - just have ctl_cyrusdb -r check if it's current and upgrade it if not.

@@ -96,6 +96,7 @@ enum mboxop { DUMP,
CHECKPOINT,
UNDUMP,
VERIFY,
ADD_ROOTS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really does feel like what we need is a $V key or something in mailboxes.db, rather than using the AS key as the upgrade indicator, and then we can confirm that roots have been added.

int is_root = 0, is_child = 0;

/* Skip DELETED.* mailboxes */
if (mbname_isdeleted(mbname)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe we do want to add the INBOX root_mailboxid to DELETED.user.foo mailboxes as well, so we know to consider them in the same userid, since they're findable with mboxlist_usermboxtree and flags.

/* Toplevel mailbox */
is_root = 1;
}
else if (rrock->mbname &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this also need && !mbname_userid(rock->mbname) for maximum safety?

rrock->mbname = mbname;
rrock->mboxid = xstrdup(mbentry->uniqueid);
}
else if (is_child) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should also be setting root_mailboxid on the INBOX or top-level folder, so we can just use that field rather than having to interpret the lack of one as "this is the root".

Particularly since that's what we do further down when we maintain this field in the mailbox creation code.

struct root_rock rrock = { NULL, NULL };
int flags = MBOXTREE_TOMBSTONES;

if (intermediary) flags |= MBOXTREE_INTERMEDIATES;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be always doing INTERMEDIATES here, while they still exist. We don't want to leave them rootless.

Copy link
Member

@brong brong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I clicked the wrong button - I meant to request changes! I think we do need to fix these few things before deploying

@ksmurchison
Copy link
Contributor Author

Superseded by #4838

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants