make server connection a cancellable operation

this entails splitting drv->open_store() into alloc_store() and
connect_store().

src/driver.h

@@ -121,8 +121,10 @@ typedef struct {
 #define DRV_MSG_BAD     1
 /* Something is wrong with the current mailbox - probably it is somehow inaccessible. */
 #define DRV_BOX_BAD     2
+/* Failed to connect store. */
+#define DRV_STORE_BAD   3
 /* The command has been cancel()ed or cancel_store()d. */
-#define DRV_CANCELED    3
+#define DRV_CANCELED    4
 
 /* All memory belongs to the driver's user, unless stated otherwise. */
 
@@ -146,16 +148,19 @@ struct driver {
 	/* Parse configuration. */
 	int (*parse_store)( conffile_t *cfg, store_conf_t **storep );
 
-	/* Close remaining server connections. All stores must be disowned first. */
+	/* Close remaining server connections. All stores must be discarded first. */
 	void (*cleanup)( void );
 
-	/* Open a store with the given configuration. This may recycle existing
-	 * server connections. Upon failure, a null store is passed to the callback. */
-	void (*open_store)( store_conf_t *conf, const char *label,
-	                    void (*cb)( store_t *ctx, void *aux ), void *aux );
+	/* Allocate a store with the given configuration. This is expected to
+	 * return quickly, and must not fail. */
+	store_t *(*alloc_store)( store_conf_t *conf, const char *label );
 
-	/* Mark the store as available for recycling. Server connection may be kept alive. */
-	void (*disown_store)( store_t *ctx );
+	/* Open/connect the store. This may recycle existing server connections. */
+	void (*connect_store)( store_t *ctx,
+	                       void (*cb)( int sts, void *aux ), void *aux );
+
+	/* Discard the store. Underlying server connection may be kept alive. */
+	void (*free_store)( store_t *ctx );
 
 	/* Discard the store after a bad_callback. The server connections will be closed.
 	 * Pending commands will have their callbacks synchronously invoked with DRV_CANCELED. */

src/drv_imap.c

@@ -100,6 +100,7 @@ typedef struct imap_store {
 	const char *prefix;
 	const char *name;
 	int ref_count;
+	enum { SST_BAD, SST_HALF, SST_GOOD } state;
 	/* trash folder's existence is not confirmed yet */
 	enum { TrashUnknown, TrashChecking, TrashKnown } trashnc;
 	uint got_namespace:1;
@@ -121,7 +122,7 @@ typedef struct imap_store {
 	int expectEOF; /* received LOGOUT's OK or unsolicited BYE */
 	int canceling; /* imap_cancel() is in progress */
 	union {
-		void (*imap_open)( store_t *srv, void *aux );
+		void (*imap_open)( int sts, void *aux );
 		void (*imap_cancel)( void *aux );
 	} callbacks;
 	void *callback_aux;
@@ -1423,6 +1424,15 @@ get_cmd_result_p2( imap_store_t *ctx, struct imap_cmd *cmd, int response )
 
 /******************* imap_cancel_store *******************/
 
+
+static void
+imap_cleanup_store( imap_store_t *ctx )
+{
+	free_generic_messages( ctx->gen.msgs );
+	free_string_list( ctx->gen.boxes );
+	free( ctx->delimiter );
+}
+
 static void
 imap_cancel_store( store_t *gctx )
 {
@@ -1434,13 +1444,11 @@ imap_cancel_store( store_t *gctx )
 	socket_close( &ctx->conn );
 	cancel_sent_imap_cmds( ctx );
 	cancel_pending_imap_cmds( ctx );
-	free_generic_messages( ctx->gen.msgs );
-	free_string_list( ctx->gen.boxes );
 	free_list( ctx->ns_personal );
 	free_list( ctx->ns_other );
 	free_list( ctx->ns_shared );
 	free_string_list( ctx->auth_mechs );
-	free( ctx->delimiter );
+	imap_cleanup_store( ctx );
 	imap_deref( ctx );
 }
 
@@ -1460,7 +1468,7 @@ imap_invoke_bad_callback( imap_store_t *ctx )
 	ctx->gen.bad_callback( ctx->gen.bad_callback_aux );
 }
 
-/******************* imap_disown_store *******************/
+/******************* imap_free_store *******************/
 
 static store_t *unowned;
 
@@ -1478,7 +1486,7 @@ imap_cancel_unowned( void *gctx )
 }
 
 static void
-imap_disown_store( store_t *gctx )
+imap_free_store( store_t *gctx )
 {
 	free_generic_messages( gctx->msgs );
 	gctx->msgs = 0;
@@ -1542,61 +1550,64 @@ static void imap_open_store_ssl_bail( imap_store_t * );
 #endif
 static void imap_open_store_bail( imap_store_t *, int );
 
-static void
-imap_open_store_bad( void *aux )
-{
-	imap_open_store_bail( (imap_store_t *)aux, FAIL_TEMP );
-}
-
-static void
-imap_open_store( store_conf_t *conf, const char *label,
-                 void (*cb)( store_t *srv, void *aux ), void *aux )
+static store_t *
+imap_alloc_store( store_conf_t *conf, const char *label )
 {
 	imap_store_conf_t *cfg = (imap_store_conf_t *)conf;
 	imap_server_conf_t *srvc = cfg->server;
 	imap_store_t *ctx;
 	store_t **ctxp;
 
+	/* First try to recycle a whole store. */
 	for (ctxp = &unowned; (ctx = (imap_store_t *)*ctxp); ctxp = &ctx->gen.next)
-		if (ctx->gen.conf == conf) {
+		if (ctx->state == SST_GOOD && ctx->gen.conf == conf) {
 			*ctxp = ctx->gen.next;
 			ctx->label = label;
-			cb( &ctx->gen, aux );
-			return;
+			return &ctx->gen;
 		}
+
+	/* Then try to recycle a server connection. */
 	for (ctxp = &unowned; (ctx = (imap_store_t *)*ctxp); ctxp = &ctx->gen.next)
-		if (((imap_store_conf_t *)ctx->gen.conf)->server == srvc) {
+		if (ctx->state != SST_BAD && ((imap_store_conf_t *)ctx->gen.conf)->server == srvc) {
 			*ctxp = ctx->gen.next;
-			ctx->label = label;
+			imap_cleanup_store( ctx );
 			/* One could ping the server here, but given that the idle timeout
 			 * is at least 30 minutes, this sounds pretty pointless. */
-			free_string_list( ctx->gen.boxes );
-			ctx->gen.boxes = 0;
-			ctx->gen.listed = 0;
-			ctx->gen.conf = conf;
-			free( ctx->delimiter );
-			ctx->delimiter = 0;
-			ctx->callbacks.imap_open = cb;
-			ctx->callback_aux = aux;
-			set_bad_callback( &ctx->gen, imap_open_store_bad, ctx );
-			imap_open_store_namespace( ctx );
-			return;
+			ctx->state = SST_HALF;
+			goto gotsrv;
 		}
 
+	/* Finally, schedule opening a new server connection. */
 	ctx = nfcalloc( sizeof(*ctx) );
+	socket_init( &ctx->conn, &srvc->sconf,
+	             (void (*)( void * ))imap_invoke_bad_callback,
+	             imap_socket_read, (void (*)(void *))flush_imap_cmds, ctx );
+	ctx->in_progress_append = &ctx->in_progress;
+	ctx->pending_append = &ctx->pending;
+
+  gotsrv:
 	ctx->gen.conf = conf;
 	ctx->label = label;
 	ctx->ref_count = 1;
-	ctx->callbacks.imap_open = cb;
-	ctx->callback_aux = aux;
-	set_bad_callback( &ctx->gen, imap_open_store_bad, ctx );
-	ctx->in_progress_append = &ctx->in_progress;
-	ctx->pending_append = &ctx->pending;
+	return &ctx->gen;
+}
 
-	socket_init( &ctx->conn, &srvc->sconf,
-	             (void (*)( void * ))imap_invoke_bad_callback,
-	             imap_socket_read, (void (*)(void *))flush_imap_cmds, ctx );
-	socket_connect( &ctx->conn, imap_open_store_connected );
+static void
+imap_connect_store( store_t *gctx,
+                    void (*cb)( int sts, void *aux ), void *aux )
+{
+	imap_store_t *ctx = (imap_store_t *)gctx;
+
+	if (ctx->state == SST_GOOD) {
+		cb( DRV_OK, aux );
+	} else {
+		ctx->callbacks.imap_open = cb;
+		ctx->callback_aux = aux;
+		if (ctx->state == SST_HALF)
+			imap_open_store_namespace( ctx );
+		else
+			socket_connect( &ctx->conn, imap_open_store_connected );
+	}
 }
 
 static void
@@ -2091,6 +2102,7 @@ imap_open_store_namespace( imap_store_t *ctx )
 {
 	imap_store_conf_t *cfg = (imap_store_conf_t *)ctx->gen.conf;
 
+	ctx->state = SST_HALF;
 	ctx->prefix = cfg->gen.path;
 	ctx->delimiter = cfg->delimiter ? nfstrdup( cfg->delimiter ) : 0;
 	if (((!ctx->prefix && cfg->use_namespace) || !cfg->delimiter) && CAP(NAMESPACE)) {
@@ -2140,11 +2152,11 @@ imap_open_store_namespace2( imap_store_t *ctx )
 static void
 imap_open_store_finalize( imap_store_t *ctx )
 {
-	set_bad_callback( &ctx->gen, 0, 0 );
+	ctx->state = SST_GOOD;
 	if (!ctx->prefix)
 		ctx->prefix = "";
 	ctx->trashnc = TrashUnknown;
-	ctx->callbacks.imap_open( &ctx->gen, ctx->callback_aux );
+	ctx->callbacks.imap_open( DRV_OK, ctx->callback_aux );
 }
 
 #ifdef HAVE_LIBSSL
@@ -2160,11 +2172,8 @@ imap_open_store_ssl_bail( imap_store_t *ctx )
 static void
 imap_open_store_bail( imap_store_t *ctx, int failed )
 {
-	void (*cb)( store_t *srv, void *aux ) = ctx->callbacks.imap_open;
-	void *aux = ctx->callback_aux;
 	((imap_store_conf_t *)ctx->gen.conf)->server->failed = failed;
-	imap_cancel_store( &ctx->gen );
-	cb( 0, aux );
+	ctx->callbacks.imap_open( DRV_STORE_BAD, ctx->callback_aux );
 }
 
 /******************* imap_open_box *******************/
@@ -2945,8 +2954,9 @@ struct driver imap_driver = {
 	DRV_CRLF | DRV_VERBOSE,
 	imap_parse_store,
 	imap_cleanup,
-	imap_open_store,
-	imap_disown_store,
+	imap_alloc_store,
+	imap_connect_store,
+	imap_free_store,
 	imap_cancel_store,
 	imap_list_store,
 	imap_select_box,

src/drv_maildir.c

@@ -194,35 +194,40 @@ maildir_validate_path( maildir_store_conf_t *conf )
 
 static void lcktmr_timeout( void *aux );
 
-static void
-maildir_open_store( store_conf_t *gconf, const char *label ATTR_UNUSED,
-                    void (*cb)( store_t *ctx, void *aux ), void *aux )
+static store_t *
+maildir_alloc_store( store_conf_t *gconf, const char *label ATTR_UNUSED )
 {
-	maildir_store_conf_t *conf = (maildir_store_conf_t *)gconf;
 	maildir_store_t *ctx;
 
 	ctx = nfcalloc( sizeof(*ctx) );
 	ctx->gen.conf = gconf;
 	ctx->uvfd = -1;
 	init_wakeup( &ctx->lcktmr, lcktmr_timeout, ctx );
-	if (gconf->path && maildir_validate_path( conf ) < 0) {
-		free( ctx );
-		cb( 0, aux );
+	return &ctx->gen;
+}
+
+static void
+maildir_connect_store( store_t *gctx,
+                       void (*cb)( int sts, void *aux ), void *aux )
+{
+	maildir_store_t *ctx = (maildir_store_t *)gctx;
+	maildir_store_conf_t *conf = (maildir_store_conf_t *)ctx->gen.conf;
+
+	if (conf->gen.path && maildir_validate_path( conf ) < 0) {
+		cb( DRV_STORE_BAD, aux );
 		return;
 	}
-	if (gconf->trash) {
+	if (conf->gen.trash) {
 		if (maildir_ensure_path( conf ) < 0) {
-			free( ctx );
-			cb( 0, aux );
+			cb( DRV_STORE_BAD, aux );
 			return;
 		}
-		if (!(ctx->trash = maildir_join_path( conf, gconf->path, gconf->trash ))) {
-			free( ctx );
-			cb( 0, aux );
+		if (!(ctx->trash = maildir_join_path( conf, conf->gen.path, conf->gen.trash ))) {
+			cb( DRV_STORE_BAD, aux );
 			return;
 		}
 	}
-	cb( &ctx->gen, aux );
+	cb( DRV_OK, aux );
 }
 
 static void
@@ -256,7 +261,7 @@ maildir_cleanup( store_t *gctx )
 }
 
 static void
-maildir_disown_store( store_t *gctx )
+maildir_free_store( store_t *gctx )
 {
 	maildir_store_t *ctx = (maildir_store_t *)gctx;
 
@@ -1761,9 +1766,10 @@ struct driver maildir_driver = {
 	0, /* XXX DRV_CRLF? */
 	maildir_parse_store,
 	maildir_cleanup_drv,
-	maildir_open_store,
-	maildir_disown_store,
-	maildir_disown_store, /* _cancel_, but it's the same */
+	maildir_alloc_store,
+	maildir_connect_store,
+	maildir_free_store,
+	maildir_free_store, /* _cancel_, but it's the same */
 	maildir_list_store,
 	maildir_select_box,
 	maildir_create_box,

src/main.c

@@ -727,10 +727,33 @@ main( int argc, char **argv )
 }
 
 #define ST_FRESH     0
-#define ST_OPEN      1
-#define ST_CLOSED    2
+#define ST_CONNECTED 1
+#define ST_OPEN      2
+#define ST_CANCELING 3
+#define ST_CLOSED    4
 
-static void store_opened( store_t *ctx, void *aux );
+static void
+cancel_prep_done( void *aux )
+{
+	MVARS(aux)
+
+	mvars->drv[t]->free_store( mvars->ctx[t] );
+	mvars->state[t] = ST_CLOSED;
+	sync_chans( mvars, E_OPEN );
+}
+
+static void
+store_bad( void *aux )
+{
+	MVARS(aux)
+
+	mvars->drv[t]->cancel_store( mvars->ctx[t] );
+	mvars->state[t] = ST_CLOSED;
+	mvars->ret = mvars->skip = 1;
+	sync_chans( mvars, E_OPEN );
+}
+
+static void store_connected( int sts, void *aux );
 static void store_listed( int sts, void *aux );
 static int sync_listed_boxes( main_vars_t *mvars, box_ent_t *mbox );
 static void done_sync_2_dyn( int sts, void *aux );
@@ -771,17 +794,18 @@ sync_chans( main_vars_t *mvars, int ent )
 			labels[M] = "M: ", labels[S] = "S: ";
 		else
 			labels[M] = labels[S] = "";
+		for (t = 0; t < 2; t++) {
+			mvars->drv[t] = mvars->chan->stores[t]->driver;
+			mvars->ctx[t] = mvars->drv[t]->alloc_store( mvars->chan->stores[t], labels[t] );
+			set_bad_callback( mvars->ctx[t], store_bad, AUX );
+		}
 		for (t = 0; ; t++) {
 			info( "Opening %s store %s...\n", str_ms[t], mvars->chan->stores[t]->name );
-			mvars->drv[t] = mvars->chan->stores[t]->driver;
-			mvars->drv[t]->open_store( mvars->chan->stores[t], labels[t], store_opened, AUX );
-			if (t)
+			mvars->drv[t]->connect_store( mvars->ctx[t], store_connected, AUX );
+			if (t || mvars->skip)
 				break;
-			if (mvars->skip) {
-				mvars->state[1] = ST_CLOSED;
-				break;
-			}
 		}
+
 		mvars->cben = 1;
 	  opened:
 		if (mvars->skip)
@@ -856,13 +880,19 @@ sync_chans( main_vars_t *mvars, int ent )
 		}
 
 	  next:
+		mvars->cben = 0;
 		for (t = 0; t < 2; t++)
-			if (mvars->state[t] == ST_OPEN) {
-				mvars->drv[t]->disown_store( mvars->ctx[t] );
+			if (mvars->state[t] == ST_FRESH) {
+				/* An unconnected store may be only cancelled. */
 				mvars->state[t] = ST_CLOSED;
+				mvars->drv[t]->cancel_store( mvars->ctx[t] );
+			} else if (mvars->state[t] == ST_CONNECTED || mvars->state[t] == ST_OPEN) {
+				mvars->state[t] = ST_CANCELING;
+				mvars->drv[t]->cancel_cmds( mvars->ctx[t], cancel_prep_done, AUX );
 			}
+		mvars->cben = 1;
 		if (mvars->state[M] != ST_CLOSED || mvars->state[S] != ST_CLOSED) {
-			mvars->skip = mvars->cben = 1;
+			mvars->skip = 1;
 			return;
 		}
 		if (mvars->chanptr->boxlist == 2) {
@@ -885,72 +915,64 @@ sync_chans( main_vars_t *mvars, int ent )
 }
 
 static void
-store_bad( void *aux )
-{
-	MVARS(aux)
-
-	mvars->drv[t]->cancel_store( mvars->ctx[t] );
-	mvars->ret = mvars->skip = 1;
-	mvars->state[t] = ST_CLOSED;
-	sync_chans( mvars, E_OPEN );
-}
-
-static void
-store_opened( store_t *ctx, void *aux )
+store_connected( int sts, void *aux )
 {
 	MVARS(aux)
 	string_list_t *cpat;
 	int cflags;
 
-	if (!ctx) {
-		mvars->ret = mvars->skip = 1;
-		mvars->state[t] = ST_CLOSED;
-		sync_chans( mvars, E_OPEN );
+	switch (sts) {
+	case DRV_CANCELED:
 		return;
-	}
-	mvars->ctx[t] = ctx;
-	if (!mvars->skip && !mvars->chanptr->boxlist && mvars->chan->patterns && !ctx->listed) {
-		for (cflags = 0, cpat = mvars->chan->patterns; cpat; cpat = cpat->next) {
-			const char *pat = cpat->string;
-			if (*pat != '!') {
-				char buf[8];
-				int bufl = snprintf( buf, sizeof(buf), "%s%s", nz( mvars->chan->boxes[t], "" ), pat );
-				int flags = 0;
-				/* Partial matches like "INB*" or even "*" are not considered,
-				 * except implicity when the INBOX lives under Path. */
-				if (starts_with( buf, bufl, "INBOX", 5 )) {
-					char c = buf[5];
-					if (!c) {
-						/* User really wants the INBOX. */
-						flags |= LIST_INBOX;
-					} else if (c == '/') {
-						/* Flattened sub-folders of INBOX actually end up in Path. */
-						if (ctx->conf->flat_delim)
-							flags |= LIST_PATH;
-						else
+	case DRV_OK:
+		if (!mvars->skip && !mvars->chanptr->boxlist && mvars->chan->patterns && !mvars->ctx[t]->listed) {
+			for (cflags = 0, cpat = mvars->chan->patterns; cpat; cpat = cpat->next) {
+				const char *pat = cpat->string;
+				if (*pat != '!') {
+					char buf[8];
+					int bufl = snprintf( buf, sizeof(buf), "%s%s", nz( mvars->chan->boxes[t], "" ), pat );
+					int flags = 0;
+					/* Partial matches like "INB*" or even "*" are not considered,
+					 * except implicity when the INBOX lives under Path. */
+					if (starts_with( buf, bufl, "INBOX", 5 )) {
+						char c = buf[5];
+						if (!c) {
+							/* User really wants the INBOX. */
 							flags |= LIST_INBOX;
+						} else if (c == '/') {
+							/* Flattened sub-folders of INBOX actually end up in Path. */
+							if (mvars->ctx[t]->conf->flat_delim)
+								flags |= LIST_PATH;
+							else
+								flags |= LIST_INBOX;
+						} else {
+							/* User may not want the INBOX after all ... */
+							flags |= LIST_PATH;
+							/* ... but maybe he does.
+							 * The flattened sub-folder case is implicitly covered by the previous line. */
+							if (c == '*' || c == '%')
+								flags |= LIST_INBOX;
+						}
 					} else {
-						/* User may not want the INBOX after all ... */
 						flags |= LIST_PATH;
-						/* ... but maybe he does.
-						 * The flattened sub-folder case is implicitly covered by the previous line. */
-						if (c == '*' || c == '%')
-							flags |= LIST_INBOX;
 					}
-				} else {
-					flags |= LIST_PATH;
+					debug( "pattern '%s' (effective '%s'): %sPath, %sINBOX\n",
+					       pat, buf, (flags & LIST_PATH) ? "" : "no ",  (flags & LIST_INBOX) ? "" : "no ");
+					cflags |= flags;
 				}
-				debug( "pattern '%s' (effective '%s'): %sPath, %sINBOX\n",
-				       pat, buf, (flags & LIST_PATH) ? "" : "no ",  (flags & LIST_INBOX) ? "" : "no ");
-				cflags |= flags;
 			}
+			mvars->state[t] = ST_CONNECTED;
+			mvars->drv[t]->list_store( mvars->ctx[t], cflags, store_listed, AUX );
+			return;
 		}
-		set_bad_callback( ctx, store_bad, AUX );
-		mvars->drv[t]->list_store( ctx, cflags, store_listed, AUX );
-	} else {
 		mvars->state[t] = ST_OPEN;
-		sync_chans( mvars, E_OPEN );
+		break;
+	default:
+		mvars->ret = mvars->skip = 1;
+		mvars->state[t] = ST_OPEN;
+		break;
 	}
+	sync_chans( mvars, E_OPEN );
 }
 
 static void

src/socket.c

@@ -516,6 +516,7 @@ socket_connected( conn_t *conn )
 {
 #ifdef HAVE_IPV6
 	freeaddrinfo( conn->addrs );
+	conn->addrs = 0;
 #endif
 	conf_notifier( &conn->notify, 0, POLLIN );
 	socket_expect_read( conn, 0 );
@@ -528,6 +529,7 @@ socket_connect_bail( conn_t *conn )
 {
 #ifdef HAVE_IPV6
 	freeaddrinfo( conn->addrs );
+	conn->addrs = 0;
 #endif
 	free( conn->name );
 	conn->name = 0;
@@ -543,6 +545,12 @@ socket_close( conn_t *sock )
 		socket_close_internal( sock );
 	free( sock->name );
 	sock->name = 0;
+#ifdef HAVE_IPV6
+	if (sock->addrs) {
+		freeaddrinfo( sock->addrs );
+		sock->addrs = 0;
+	}
+#endif
 #ifdef HAVE_LIBSSL
 	if (sock->ssl) {
 		SSL_free( sock->ssl );