reconstruct failures #16
Comments
|
This is against master at 2072f6c. Backtrace from the segmentation fault: And analysis so far, pasted from IRC:
|
|
This seems to be a totally different issue from https://bugzilla.cyrusimap.org/show_bug.cgi?id=3919 |
|
11772bb fixes the double handling of mailboxes by keeping a hash of the visited mailboxes, and skipping anything that's already in it. Looks like we used to already do something like this, but could only remember one visited item, so if it ended up trying to process A, A, then A would be skipped the second time, but if it was trying to process A, B, A, then A would get processed twice. |
|
On Mon, 15 Aug 2016, at 14:57, elliefm wrote:
That's also a bug, we shouldn't allow this to happen. -r shouldn't use
Leading to this - a brand new bug. Creating new uniqueids due to a
Yep, it is.
Yeah, that's bogus. What happens if you run 'reconstruct -u
That's definitely bogus - it shouldn't be possible to double-handle Bron Gondwana |
|
On Mon, 15 Aug 2016, at 17:00, elliefm wrote:
I am satisfied that this is generally a good belt-and-suspenders even once we fix the stupid -r behaviour.Bron Gondwana Links: |
|
IIRC I think reconstruct -u username worked okay, but I'll verify tomorrow when I have my vm up again |
|
I've just painstakingly stepped through ... but for each one encountered, find_p ends up rejecting it, so nothing happens. Note that rock->mb_category is zero. I'm not sure what this means yet. Stepping through this though, I have noticed that I missed passing the reconstruct_rock to do_reconstruct_p() rock in my last fix, so if it did manage to get through as far as do_reconstruct() it would promptly crash out, doh! So I'll fix that, and then see if I can see what this mb_category business is about. |
|
On Tue, 16 Aug 2016, at 09:03, elliefm wrote:
Ahh, yeah - OK - that could explain some bits. mb_category is /* categorise mailboxes */ So what we probably want there is something like: if (rock->mb_category && mbname_category(rock->mbname, rock->namespace, So that in the zero case, all mailboxes match. Bron. Bron Gondwana |
|
That makes sense For what it's worth, mbname_category(...) is returning MBNAME_OTHERUSER in this case, because: The userid passed in is NULL, which doesn't match "cassandane" as determined by mbname_userid(mbname). I'm interpreting the "userid" parameter as "the user performing this operation"? Which in this case would logically be CYRUS_USER I suppose, since we're in a system tool, not on a socket. So that makes sense actually. |
|
After d15fcce it still doesn't make it through find_p positively: The I've experimentally tried the latter, and |
|
Okay so mboxlist_findone() looks like it can't ever work at the moment, because it sets up the It's used twice by reconstruct, once by autocreate, and once by chk_cyrus. mboxlist_findone() gets an internal mailbox name as one of its arguments. find_p() matches the globs against an external mailbox name. So for mboxlist_findone() to set up a pattern, it would need to create the pattern in the external namespace -- which it could do, because it does get a namespace pointer. Though I'm not sure how to set up a pattern that matches a string exactly -- Okay nevermind that actually. mboxlist_findone() calls cyrusdb_forone(..., p=&find_p, cb=&find_cb, ...) which only ever finds a single element anyway, which it looks up by the given intname, so instead of jumping through hoops to make find_p also match exactly one item correctly, just pass NULL to cyrusdb_forone's |
|
But wait. find_p does some extra checks which maybe are important -- even if we've found the correct mailbox, it might decide it needs to pretend there is no such mailbox. Hmm. |
|
Adding the pattern to mboxlist_findone appears to be the magic ticket -- it fixes the problem where find_p ignores the results, and then also fixes the problem where find_cb doesn't initialise findall_data.mbname, so we no longer crash out |
|
great :) On Tue, 16 Aug 2016, at 11:27, elliefm wrote:
Bron Gondwana Links: |
|
Need to test a couple more things, and needs some cleanup. But I have to head out for a bit, so WIP is here and I'll finish it off later: master...elliefm:v30/fix-reconstruct-crash |
|
My wip patch fixes |
|
Okay so there's another thing going on here, and this is partially a user-interface specification issue: When not using -u mode, reconstruct's arguments are understood as being mailbox patterns. We construct (with glob_init()) a pattern from each argument foo with heir_sep x thus: This means find_cb has some handling (rock->singlepercent) that looks like maybe it's trying to accommodate the possibility that we're matching on a pattern? Except that "user.cassandane" is a pattern, but contains no wildcards, so we don't end up setting rock->singlepercent. We (now, with my patch) handle user.cassandane itself okay (because [Aside: looking at the rock->singlepercent code path in find_cb(), fbdata.mbname isn't set there either. So do_reconstruct will probably crash in this case too, I just haven't tickled it yet.] So again, a couple of aspects:
|
|
Turns out NULL fdata.mbname is find_cb's way of indicating partial matches to callers who need that, and other things that only care about exact matches just return 0 in this case. So reconstruct now does the same: e72821c reconstruct now passes all the initial checks I threw at it, plus the -u username check discussed later. There's still the matter of whether or not -r should accept wildcards or not -- at the moment it still does, but it at least handles them sensibly now instead of stomping all over itself. |
First, let's see what we have. Ignore the folder names, these are (still) just ordinary folders. Stumbled across this while setting up something unrelated.
Cool. Let's reconstruct user.cassandane:
Uh oh? Likewise with -r (recursive):
What if I use a pattern?
Okay that's better, but it's missed the Inbox because of that "."...
Okay that's done the trick. (Lucky we don't have some other user called "user.cassandane2" or something....) What about recursively?
Hmm, it's done all the subfolders twice, I suppose because they all match the pattern but they also exist under user.cassandane. What if we exclude the top level?
Whoa what the hell? Syslog says:
Playing around, that latter case looks like it only crops up after a reconstruct of "user.cassandane*" (the one that reconstructs all the subfolders twice), so it /might/ actually be fixing something that the earlier reconstruct broke.
So two problems, probably unrelated:
The text was updated successfully, but these errors were encountered: