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

ctl_mboxlist (un)dump - deal with all fields #3384

Closed

Conversation

dilyanpalauzov
Copy link
Contributor

In order to have the improved_mboxlist_sort I altered ctl_mboxlist to dump/undump all fields. The parsing (undumping) is done now by scanf(), which significantly simplifies the code.

It works for me in the environment I have.

@dilyanpalauzov
Copy link
Contributor Author

I just changed the wording in thoughts/improved_mboxlist_sort.rst to make renaming the mailboxes.db as intermediate step not recommended, but imperative. First it does not work without renaming it, second there shall be a backup, in case something goes wrong.

Copy link
Contributor

@elliefm elliefm left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks! I'm requesting one tiny change, but overall it's great.

It looks like it preserves the original set of fields, and then uses a '>' to indicate the start of the new set. I think this means that if you had a dump file created with an old version of ctl_mboxlist, a new version of ctl_mboxlist would still be able to import it successfully (albeit missing the data that wasn't in the file). I'm not sure it makes sense to undump a file created from an older version, but I'm pleased to see that it looks like it'll work.

Looks like the reverse is not true though: an older ctl_mboxlist will slurp the '>' and all the new fields after it into the ACL string. Not much we can do about that, but if someone could install a patch to fix it they could just install this patch this instead.

Once this is merged, I think I plan to backport it to all the previous versions for which it makes sense, because it would be useful for older deployments to have it available during upgrades.

@brong @rsto @ksmurchison: can one of you have a look at this too please? I almost never use scanf myself, so I'd appreciate a second set of eyes here.

imap/ctl_mboxlist.c Outdated Show resolved Hide resolved
@dilyanpalauzov
Copy link
Contributor Author

dilyanpalauzov commented Dec 17, 2021

As modseq_t is unsigned long long int I have changed the originial patch to use %llu instead of %lli for mbentry->createdmodseq and mbentry->foldermodseq in both printf() and scanf().

In the meantime, the field ptrarray_t name_history was added to struct mboxlist_entry and the type of mbtype was changed from int to uint32_t. These changes are not handled by the current patch.

@elliefm
Copy link
Contributor

elliefm commented Mar 4, 2022

I just tried using the github web "Resolve conflicts" thing to resolve the one conflict on this, and it handled it as merging master back into the feature branch. Nasty. I won't do that anymore.

The conflict that needed to be fixed was around here:

        /* generate a new entry */
        int r = mboxlist_update(newmbentry, /*localonly*/1);

It now wants to be:

        /* generate a new entry */
        int r = mboxlist_updatelock(newmbentry, /*localonly*/1);

i.e. it should now call mboxlist_updatelock instead of mboxlist_update.

Do you mind rebasing your branch to squash out the merge commit, as if your commit had said mboxlist_updatelock all along? Thanks, and sorry for the nuisance.

I'd like to get this merged and backported, I have new work I'm building on top of it.

@dilyanpalauzov dilyanpalauzov deleted the ctl_mboxlist_fix_undump branch March 4, 2022 07:59
@dilyanpalauzov
Copy link
Contributor Author

Accidentally I closed this PR. I do not want to do rebase. These are the changes: file.patch.txt Do with them whatever you want.

@elliefm
Copy link
Contributor

elliefm commented Mar 6, 2022

No worries, I'll just integrate your commit directly into my own branch. Thanks!

@elliefm
Copy link
Contributor

elliefm commented Mar 6, 2022

For the history: this will become part of the solution to #3896

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.

None yet

2 participants