On Wed, Nov 21, 2007 at 06:37:17AM -0500, Ken Murchison wrote:
> Bron Gondwana wrote:
>> On Wed, 21 Nov 2007 07:40:21 +0100 (CET), "Simon Matter"
>> <simon.matter@xxxxxxxxx> said:
>>> Hi Bron,
>>>
>>> Did you consider this one
>>> http://bugzilla.andrew.cmu.edu/show_bug.cgi?id=3006 in the patch above?
>>> From a quick look it seems both patches conflict, is #3006 obsolete now?
>> You're right, they probably do conflict and Ken has applied 3006. I
>> should
>> have checked on that. Hmm...
>> Actually, my patch doesn't even fix #3006. It does sort of suck to be
>> working
>> from a stable past point rather than the latest CVS actually when I do
>> these
>> patches - though it's nice from a stability point of view.
>> OK - Ken, would you like me to re-build this patch on top of current CVS,
>> or
>> would you like to do it yourself? My patch has the replication bugfixes
>> which
>> are still worth doing, but Simon's logic is actually correct for the test
>> while mine isn't (though I renamed the function he's using...)
>
> If you have the time. I'm off banging on other things at the moment.
I guess I should attach them!
Bron.
Fix Delayed Delete replication
<b>Candidate for upstream - now respun on top of
Simon Matter's allowusermoves fixes from CVS</b>
Deleting user.foo was broken on replicas thanks to
me choosing a bad method for making it work.
Also, it was leaving foo.TIMESTAMP.sub files because
it considered that to be a valid "user". Oops.
This patch takes a different approach (on top of the
delayed delete code already in Cyrus 2.3.10):
1) mboxname_isusermailbox() no longer returns true for
DELETED.user.foo.TIMESTAMP
2) The "can I rename this" code checks for the target
name being either another username or something in the
DELETED. namespace.
2a) mboxname_isdeletedmailbox() rather than the somewhat
(IMHO) misplaced mboxlist_in_deletedhierarchy().
3) if "forceuser" is passed (only done by sync_server),
then all ACL checks are completely bypassed, so the
rename will always succeed on the replica.
Index: cyrus-imapd-2.3.10/imap/mboxname.c
===================================================================
--- cyrus-imapd-2.3.10.orig/imap/mboxname.c 2007-11-22 01:22:08.000000000 -0500
+++ cyrus-imapd-2.3.10/imap/mboxname.c 2007-11-22 01:29:17.000000000 -0500
@@ -600,35 +600,47 @@
{
const char *p;
const char *start = name;
- const char *deletedprefix = config_getstring(IMAPOPT_DELETEDPREFIX);
- size_t len = strlen(deletedprefix);
- int isdel = 0;
/* step past the domain part */
if (config_virtdomains && (p = strchr(start, '!')))
start = p + 1;
- /* step past any deletedprefix */
- if (mboxlist_delayed_delete_isenabled() && strlen(start) > len+1 &&
- !strncmp(start, deletedprefix, len) && start[len] == '.') {
- start += len + 1;
- isdel = 1; /* there's an add'l sep + hextimestamp on isdel folders */
- }
-
/* starts with "user." AND
* we don't care if it's an inbox OR
- * there's no dots after the username OR
- * it's deleted and there's only one more dot
+ * there's no dots after the username
*/
- if (!strncmp(start, "user.", 5) &&
- (!isinbox || !strchr(start+5, '.') ||
- (isdel && (p = strchr(start+5, '.')) && !strchr(p+1, '.'))))
- return (char*) start+5; /* could have trailing bits if isinbox+isdel */
+ if (!strncmp(start, "user.", 5) && (!isinbox || !strchr(start+5, '.')))
+ return (char*) start+5;
else
return NULL;
}
/*
+ * If (internal) mailbox 'name' is a DELETED mailbox
+ * returns boolean
+ */
+int mboxname_isdeletedmailbox(const char *name)
+{
+ static const char *deletedprefix = NULL;
+ static int deletedprefix_len = 0;
+ int domainlen = 0;
+ char *p;
+
+ if (!mboxlist_delayed_delete_isenabled()) return(0);
+
+ if (!deletedprefix) {
+ deletedprefix = config_getstring(IMAPOPT_DELETEDPREFIX);
+ deletedprefix_len = strlen(deletedprefix);
+ }
+
+ if (config_virtdomains && (p = strchr(name, '!')))
+ domainlen = p - name + 1;
+
+ return ((!strncmp(name + domainlen, deletedprefix, deletedprefix_len) &&
+ name[domainlen + deletedprefix_len] == '.') ? 1 : 0);
+}
+
+/*
* Translate (internal) inboxname into corresponding userid.
*/
char *mboxname_inbox_touserid(const char *inboxname)
Index: cyrus-imapd-2.3.10/imap/mboxlist.c
===================================================================
--- cyrus-imapd-2.3.10.orig/imap/mboxlist.c 2007-11-22 01:29:03.000000000 -0500
+++ cyrus-imapd-2.3.10/imap/mboxlist.c 2007-11-22 01:32:08.000000000 -0500
@@ -1223,8 +1223,7 @@
isusermbox = 1;
} else if ((config_getswitch(IMAPOPT_ALLOWUSERMOVES) &&
mboxname_isusermailbox(newname, 1)) ||
- mboxlist_in_deletedhierarchy(oldname) ||
- mboxlist_in_deletedhierarchy(newname)) {
+ mboxname_isdeletedmailbox(newname)) {
/* Special case of renaming a user */
access = cyrus_acl_myrights(auth_state, oldacl);
if (!(access & ACL_DELETEMBOX) && !isadmin) {
@@ -1257,8 +1256,7 @@
if (mboxname_isusermailbox(newname, 1)) {
if ((config_getswitch(IMAPOPT_ALLOWUSERMOVES) &&
mboxname_isusermailbox(oldname, 1)) ||
- mboxlist_in_deletedhierarchy(oldname) ||
- mboxlist_in_deletedhierarchy(newname)) {
+ mboxname_isdeletedmailbox(oldname)) {
if (!isadmin) {
/* Only admins can rename users (INBOX to INBOX) */
r = IMAP_MAILBOX_NOTSUPPORTED;
@@ -1975,7 +1973,7 @@
memcpy(namebuf, key, keylen);
namebuf[keylen] = '\0';
if (mboxlist_delayed_delete_isenabled() &&
- mboxlist_in_deletedhierarchy(namebuf))
+ mboxname_isdeletedmailbox(namebuf))
return 0;
}
@@ -3382,24 +3380,3 @@
return(config_delete_mode == IMAP_ENUM_DELETE_MODE_DELAYED);
}
-
-int mboxlist_in_deletedhierarchy(const char *mailboxname)
-{
- static const char *deletedprefix = NULL;
- static int deletedprefix_len = 0;
- int domainlen = 0;
- char *p;
-
- if (!mboxlist_delayed_delete_isenabled()) return(0);
-
- if (!deletedprefix) {
- deletedprefix = config_getstring(IMAPOPT_DELETEDPREFIX);
- deletedprefix_len = strlen(deletedprefix);
- }
-
- if (config_virtdomains && (p = strchr(mailboxname, '!')))
- domainlen = p - mailboxname + 1;
-
- return ((!strncmp(mailboxname + domainlen, deletedprefix, deletedprefix_len) &&
- mailboxname[domainlen + deletedprefix_len] == '.') ? 1 : 0);
-}
Index: cyrus-imapd-2.3.10/imap/mboxlist.h
===================================================================
--- cyrus-imapd-2.3.10.orig/imap/mboxlist.h 2007-11-22 01:22:08.000000000 -0500
+++ cyrus-imapd-2.3.10/imap/mboxlist.h 2007-11-22 01:29:17.000000000 -0500
@@ -211,5 +211,4 @@
int mboxlist_abort(struct txn *tid);
int mboxlist_delayed_delete_isenabled(void);
-int mboxlist_in_deletedhierarchy(const char *mailboxname);
#endif
Index: cyrus-imapd-2.3.10/imap/mboxname.h
===================================================================
--- cyrus-imapd-2.3.10.orig/imap/mboxname.h 2007-11-22 01:22:08.000000000 -0500
+++ cyrus-imapd-2.3.10/imap/mboxname.h 2007-11-22 01:29:17.000000000 -0500
@@ -109,6 +109,12 @@
char *mboxname_isusermailbox(const char *name, int isinbox);
/*
+ * If (internal) mailbox 'name' is in the DELETED namespace
+ * returns boolean
+ */
+int mboxname_isdeletedmailbox(const char *name);
+
+/*
* Translate (internal) inboxname into corresponding userid.
*/
char *mboxname_inbox_touserid(const char *inboxname);
Index: cyrus-imapd-2.3.10/imap/imapd.c
===================================================================
--- cyrus-imapd-2.3.10.orig/imap/imapd.c 2007-11-22 01:22:08.000000000 -0500
+++ cyrus-imapd-2.3.10/imap/imapd.c 2007-11-22 01:29:17.000000000 -0500
@@ -5004,7 +5004,7 @@
r = mboxlist_deletemailbox(name, imapd_userisadmin,
imapd_userid, imapd_authstate,
0, 0, 0);
- } else if (imapd_userisadmin && mboxlist_in_deletedhierarchy(name)) {
+ } else if (imapd_userisadmin && mboxname_isdeletedmailbox(name)) {
r = mboxlist_deletemailbox(name, imapd_userisadmin,
imapd_userid, imapd_authstate,
0, 0, 0);
@@ -5096,7 +5096,7 @@
imapd_userid, imapd_authstate,
1-force, localonly, 0);
} else if (imapd_userisadmin &&
- mboxlist_in_deletedhierarchy(mailboxname)) {
+ mboxname_isdeletedmailbox(mailboxname)) {
r = mboxlist_deletemailbox(mailboxname, imapd_userisadmin,
imapd_userid, imapd_authstate,
0 /* checkacl */, localonly, 0);
Skiplist - more fixes broken out
<b>Candidate for Upstream</b>
These are changes made to skiplist after the earlier bugfixes and
robusify patches were accepted upstream.
* double check that "DELETE" in recovery actually finds a node with
the exact same key, and doesn't trash some other node if there's
corruption.
* handle "DELETE" where there isn't any such name any more and ADD
where the node already exists (possible if other corruption) to
recover the most data possible (complete with funky different
level logic. I checked it over with Rob and little picture of
the links on paper! --Bron)
Index: cyrus-imapd-2.3.10/lib/cyrusdb_skiplist.c
===================================================================
--- cyrus-imapd-2.3.10.orig/lib/cyrusdb_skiplist.c 2007-11-22 01:35:19.000000000 -0500
+++ cyrus-imapd-2.3.10/lib/cyrusdb_skiplist.c 2007-11-22 01:35:29.000000000 -0500
@@ -2114,7 +2114,9 @@
myoff = ntohl(*((bit32 *)(ptr + 4)));
p = db->map_base + myoff;
keyptr = find_node(db, KEY(p), KEYLEN(p), updateoffsets);
- if (keyptr == db->map_base) {
+ if (keyptr == db->map_base ||
+ db->compar(KEY(p), KEYLEN(p), KEY(keyptr), KEYLEN(keyptr))) {
+ /* didn't find exactly this node */
keyptr = NULL;
}
}
@@ -2139,23 +2141,23 @@
/* otherwise if DELETE, throw an error */
} else if (TYPE(ptr) == DELETE) {
syslog(LOG_ERR,
- "DBERROR: skiplist recovery %s: DELETE at %04X doesn't exist",
- db->fname, offset);
- r = CYRUSDB_IOERROR;
-
- /* otherwise if ADD & found key, throw an error */
- } else if (TYPE(ptr) == ADD && keyptr) {
- syslog(LOG_ERR,
- "DBERROR: skiplist recovery %s: ADD at %04X exists",
+ "DBERROR: skiplist recovery %s: DELETE at %04X doesn't exist, skipping",
db->fname, offset);
- r = CYRUSDB_IOERROR;
+ need_checkpoint = 1;
/* otherwise insert it */
} else if (TYPE(ptr) == ADD) {
unsigned int lvl;
bit32 newoffsets[SKIPLIST_MAXLEVEL];
- db->listsize++;
+ if (keyptr) {
+ syslog(LOG_ERR,
+ "DBERROR: skiplist recovery %s: ADD at %04X exists, replacing",
+ db->fname, offset);
+ need_checkpoint = 1;
+ } else {
+ db->listsize++;
+ }
offsetnet = htonl(offset);
lvl = LEVEL(ptr);
@@ -2165,10 +2167,36 @@
db->fname, lvl, SKIPLIST_MAXLEVEL);
r = CYRUSDB_IOERROR;
} else {
+ /* NOTE - in the bogus case where a record with the same key already
+ * exists, there are three possible cases:
+ * lvl == LEVEL(keyptr)
+ * * trivial: all to me, all mine to keyptr's FORWARD
+ * lvl > LEVEL(keyptr) -
+ * * all updateoffsets values should point to me
+ * * up until LEVEL(keyptr) set to keyptr's next values
+ * (updateoffsets[i] should be keyptr in these cases)
+ * then point all my higher pointers are updateoffsets[i]'s
+ * FORWARD instead.
+ * lvl < LEVEL(keyptr)
+ * * updateoffsets values up to lvl should point to me
+ * * all mine should point to keyptr's next values
+ * * from lvl up, all updateoffsets[i] should point to
+ * FORWARD(keyptr, i) instead.
+ *
+ * All of this fully unstitches keyptr from the chain and stitches
+ * the current node in, regardless of height difference. Man what
+ * a pain!
+ */
for (i = 0; i < lvl; i++) {
/* set our next pointers */
- newoffsets[i] =
- htonl(FORWARD(db->map_base + updateoffsets[i], i));
+ if (keyptr && i < LEVEL(keyptr)) {
+ /* need to replace the matching record key */
+ newoffsets[i] =
+ htonl(FORWARD(keyptr, i));
+ } else {
+ newoffsets[i] =
+ htonl(FORWARD(db->map_base + updateoffsets[i], i));
+ }
/* replace 'updateoffsets' to point to me */
lseek(db->fd,
@@ -2179,6 +2207,18 @@
/* write out newoffsets */
lseek(db->fd, FIRSTPTR(ptr) - db->map_base, SEEK_SET);
retry_write(db->fd, (char *) newoffsets, 4 * lvl);
+
+ if (keyptr && lvl < LEVEL(keyptr)) {
+ bit32 newoffsetnet;
+ for (i = lvl; i < LEVEL(keyptr); i++) {
+ newoffsetnet = htonl(FORWARD(keyptr, i));
+ /* replace 'updateoffsets' to point onwards */
+ lseek(db->fd,
+ PTR(db->map_base + updateoffsets[i], i) - db->map_base,
+ SEEK_SET);
+ retry_write(db->fd, (char *) &newoffsetnet, 4);
+ }
+ }
}
/* can't happen */
} else {
----
Cyrus Home Page: http://cyrusimap.web.cmu.edu/
Cyrus Wiki/FAQ: http://cyrusimap.web.cmu.edu/twiki
List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
