From c47ee1c8c4bc1961cf7fd665326bfa05504ab0e7 Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Wed, 11 Dec 2013 16:13:49 +0100 Subject: [PATCH] fix error paths wrt sync drivers, take 3 msgs_copied() was not checked at all, and msgs_flags_set() was doing it wrong (sync_close() was not checked). instead of trying to fix/extend the msgs_flags_set() model (ref-counting and cancelation checking in lower-level functions, and return values to propagate the status), place the refs/derefs around higher-level scopes and do the checking only there. this is effectively simpler, and does away with some obscure macros. --- src/sync.c | 98 ++++++++++++++++++++++++++---------------------------- 1 file changed, 47 insertions(+), 51 deletions(-) diff --git a/src/sync.c b/src/sync.c index bc55911..94b3fb7 100644 --- a/src/sync.c +++ b/src/sync.c @@ -166,25 +166,9 @@ typedef struct { } sync_vars_t; static void sync_ref( sync_vars_t *svars ) { ++svars->ref_count; } -static int sync_deref( sync_vars_t *svars ); -static int deref_check_cancel( sync_vars_t *svars ); +static void sync_deref( sync_vars_t *svars ); static int check_cancel( sync_vars_t *svars ); -#define DRIVER_CALL_RET(call) \ - do { \ - sync_ref( svars ); \ - svars->drv[t]->call; \ - return deref_check_cancel( svars ); \ - } while (0) - -#define DRIVER_CALL(call) \ - do { \ - sync_ref( svars ); \ - svars->drv[t]->call; \ - if (deref_check_cancel( svars )) \ - return; \ - } while (0) - #define AUX &svars->t[t] #define INV_AUX &svars->t[1-t] #define DECL_SVARS \ @@ -282,7 +266,7 @@ typedef struct copy_vars { static void msg_fetched( int sts, void *aux ); -static int +static void copy_msg( copy_vars_t *vars ) { DECL_INIT_SVARS(vars->aux); @@ -290,7 +274,7 @@ copy_msg( copy_vars_t *vars ) t ^= 1; vars->data.flags = vars->msg->flags; vars->data.date = svars->chan->use_internal_date ? -1 : 0; - DRIVER_CALL_RET(fetch_msg( svars->ctx[t], vars->msg, &vars->data, msg_fetched, vars )); + svars->drv[t]->fetch_msg( svars->ctx[t], vars->msg, &vars->data, msg_fetched, vars ); } static void msg_stored( int sts, int uid, void *aux ); @@ -533,14 +517,6 @@ store_bad( void *aux ) cancel_sync( svars ); } -static int -deref_check_cancel( sync_vars_t *svars ) -{ - if (sync_deref( svars )) - return -1; - return check_cancel( svars ); -} - static int check_cancel( sync_vars_t *svars ) { @@ -640,13 +616,17 @@ sync_boxes( store_t *ctx[], const char *names[], channel_conf_t *chan, } /* Both boxes must be fully set up at this point, so that error exit paths * don't run into uninitialized variables. */ + sync_ref( svars ); for (t = 0; t < 2; t++) { info( "Selecting %s %s...\n", str_ms[t], ctx[t]->orig_name ); - DRIVER_CALL(select( ctx[t], (chan->ops[t] & OP_CREATE) != 0, box_selected, AUX )); + svars->drv[t]->select( ctx[t], (chan->ops[t] & OP_CREATE) != 0, box_selected, AUX ); + if (check_cancel( svars )) + break; } + sync_deref( svars ); } -static int load_box( sync_vars_t *svars, int t, int minwuid, int *mexcs, int nmexcs ); +static void load_box( sync_vars_t *svars, int t, int minwuid, int *mexcs, int nmexcs ); static void box_selected( int sts, void *aux ) @@ -1083,14 +1063,16 @@ box_selected( int sts, void *aux ) } else { minwuid = INT_MAX; } - if (load_box( svars, M, minwuid, mexcs, nmexcs )) - return; - load_box( svars, S, (ctx[S]->opts & OPEN_OLD) ? 1 : INT_MAX, 0, 0 ); + sync_ref( svars ); + load_box( svars, M, minwuid, mexcs, nmexcs ); + if (!check_cancel( svars )) + load_box( svars, S, (ctx[S]->opts & OPEN_OLD) ? 1 : INT_MAX, 0, 0 ); + sync_deref( svars ); } static void box_loaded( int sts, void *aux ); -static int +static void load_box( sync_vars_t *svars, int t, int minwuid, int *mexcs, int nmexcs ) { sync_rec_t *srec; @@ -1109,7 +1091,7 @@ load_box( sync_vars_t *svars, int t, int minwuid, int *mexcs, int nmexcs ) maxwuid = 0; info( "Loading %s...\n", str_ms[t] ); debug( maxwuid == INT_MAX ? "loading %s [%d,inf]\n" : "loading %s [%d,%d]\n", str_ms[t], minwuid, maxwuid ); - DRIVER_CALL_RET(load( svars->ctx[t], minwuid, maxwuid, svars->newuid[t], mexcs, nmexcs, box_loaded, AUX )); + svars->drv[t]->load( svars->ctx[t], minwuid, maxwuid, svars->newuid[t], mexcs, nmexcs, box_loaded, AUX ); } typedef struct { @@ -1125,7 +1107,7 @@ typedef struct { static void flags_set( int sts, void *aux ); static void flags_set_p2( sync_vars_t *svars, sync_rec_t *srec, int t ); -static int msgs_flags_set( sync_vars_t *svars, int t ); +static void msgs_flags_set( sync_vars_t *svars, int t ); static void msg_copied( int sts, int uid, copy_vars_t *vars ); static void msg_copied_p2( sync_vars_t *svars, sync_rec_t *srec, int t, int uid ); static void msgs_copied( sync_vars_t *svars, int t ); @@ -1459,6 +1441,8 @@ box_loaded( int sts, void *aux ) } } + sync_ref( svars ); + debug( "synchronizing flags\n" ); for (srec = svars->srecs; srec; srec = srec->next) { if ((srec->status & S_DEAD) || srec->uid[M] <= 0 || srec->uid[S] <= 0) @@ -1502,7 +1486,9 @@ box_loaded( int sts, void *aux ) fv->srec = srec; fv->aflags = aflags; fv->dflags = dflags; - DRIVER_CALL(set_flags( svars->ctx[t], srec->msg[t], srec->uid[t], aflags, dflags, flags_set, fv )); + svars->drv[t]->set_flags( svars->ctx[t], srec->msg[t], srec->uid[t], aflags, dflags, flags_set, fv ); + if (check_cancel( svars )) + goto out; } else flags_set_p2( svars, srec, t ); } @@ -1510,8 +1496,9 @@ box_loaded( int sts, void *aux ) for (t = 0; t < 2; t++) { svars->drv[t]->commit( svars->ctx[t] ); svars->state[t] |= ST_SENT_FLAGS; - if (msgs_flags_set( svars, t )) - return; + msgs_flags_set( svars, t ); + if (check_cancel( svars )) + goto out; } debug( "propagating new messages\n" ); @@ -1528,13 +1515,19 @@ box_loaded( int sts, void *aux ) cv->aux = AUX; cv->srec = srec; cv->msg = tmsg; - if (copy_msg( cv )) - return; + copy_msg( cv ); + if (check_cancel( svars )) + goto out; } } svars->state[t] |= ST_SENT_NEW; msgs_copied( svars, t ); + if (check_cancel( svars )) + goto out; } + + out: + sync_deref( svars ); } static void @@ -1687,14 +1680,16 @@ flags_set_p2( sync_vars_t *svars, sync_rec_t *srec, int t ) static void msg_trashed( int sts, void *aux ); static void msg_rtrashed( int sts, int uid, copy_vars_t *vars ); -static int +static void msgs_flags_set( sync_vars_t *svars, int t ) { message_t *tmsg; copy_vars_t *cv; if (!(svars->state[t] & ST_SENT_FLAGS) || svars->flags_done[t] < svars->flags_total[t]) - return 0; + return; + + sync_ref( svars ); if ((svars->chan->ops[t] & OP_EXPUNGE) && (svars->ctx[t]->conf->trash || (svars->ctx[1-t]->conf->trash && svars->ctx[1-t]->conf->trash_remote_new))) { @@ -1706,10 +1701,9 @@ msgs_flags_set( sync_vars_t *svars, int t ) debug( "%s: trashing message %d\n", str_ms[t], tmsg->uid ); svars->trash_total[t]++; stats( svars ); - sync_ref( svars ); svars->drv[t]->trash_msg( svars->ctx[t], tmsg, msg_trashed, AUX ); - if (deref_check_cancel( svars )) - return -1; + if (check_cancel( svars )) + goto out; } else debug( "%s: not trashing message %d - not new\n", str_ms[t], tmsg->uid ); } else { @@ -1723,8 +1717,9 @@ msgs_flags_set( sync_vars_t *svars, int t ) cv->aux = INV_AUX; cv->srec = 0; cv->msg = tmsg; - if (copy_msg( cv )) - return -1; + copy_msg( cv ); + if (check_cancel( svars )) + goto out; } else debug( "%s: not remote trashing message %d - too big\n", str_ms[t], tmsg->uid ); } else @@ -1734,7 +1729,9 @@ msgs_flags_set( sync_vars_t *svars, int t ) } svars->state[t] |= ST_SENT_TRASH; sync_close( svars, t ); - return 0; + + out: + sync_deref( svars ); } static void @@ -1914,7 +1911,8 @@ sync_bail3( sync_vars_t *svars ) sync_deref( svars ); } -static int sync_deref( sync_vars_t *svars ) +static void +sync_deref( sync_vars_t *svars ) { if (!--svars->ref_count) { void (*cb)( int sts, void *aux ) = svars->cb; @@ -1922,7 +1920,5 @@ static int sync_deref( sync_vars_t *svars ) int ret = svars->ret; free( svars ); cb( ret, aux ); - return -1; } - return 0; }