On 06/05/2012 08:16 PM, Christoph Hellwig wrote: > Use a switch statement to deal with the different return values from > recover_object_from_replica, and move the switch to and older epoch directly > into the case label where it is required instead of using an obsfucating > control flow. > > Signed-off-by: Christoph Hellwig <[email protected]> > > diff --git a/sheep/recovery.c b/sheep/recovery.c > index 2597126..17f6038 100644 > --- a/sheep/recovery.c > +++ b/sheep/recovery.c > @@ -177,7 +177,7 @@ static int is_invalid_vnode(struct sd_vnode *entry, > struct sd_node *nodes, > */ > static int do_recover_object(struct recovery_work *rw) > { > - struct vnode_info *old; > + struct vnode_info *old, *new_old; > uint64_t oid = rw->oids[rw->done]; > uint32_t epoch = rw->epoch, tgt_epoch = rw->epoch - 1; > int nr_copies, ret, i; > @@ -198,38 +198,37 @@ again: > continue; > ret = recover_object_from_replica(oid, tgt_vnode, > epoch, tgt_epoch); > - if (ret == 0) { > + switch (ret) { > + case 0: > /* Succeed */ > - break; > - } else if (SD_RES_OLD_NODE_VER == ret) { > + goto out; > + case SD_RES_OLD_NODE_VER: > rw->stop = 1; > - goto err; > - } else > - ret = -1; > - } > - > - /* No luck, roll back to an older configuration and try again */ > - if (ret < 0) { > - struct vnode_info *new_old; > + goto out; > + default: > + /* > + * No luck, roll back to an older configuration and > + * try again. > + */ > + tgt_epoch--; > + if (tgt_epoch < 1) { > + eprintf("can not recover oid %"PRIx64"\n", oid); > + ret = SD_RES_EIO; > + goto out; > + } > > - tgt_epoch--; > - if (tgt_epoch < 1) { > - eprintf("can not recover oid %"PRIx64"\n", oid); > - ret = -1; > - goto err; > - } > + new_old = get_vnodes_from_epoch(tgt_epoch); > + if (!new_old) { > + ret = SD_RES_EIO; > + goto out; > + } > > - new_old = get_vnodes_from_epoch(tgt_epoch); > - if (!new_old) { > - ret = -1; > - goto err; > + put_vnode_info(old); > + old = new_old; > + goto again; > } > - > - put_vnode_info(old); > - old = new_old; > - goto again; > } > -err: > +out: > put_vnode_info(old); > return ret; > }
Nack, this patch intrusively changes the recovery logic and don't allow us to do a breadth-first recovery of the copy. Also, nesting switch case inside for loop doesn't make the code looks simpler and cleaner, yet more tricky. Thanks, Yuan -- sheepdog mailing list [email protected] http://lists.wpkg.org/mailman/listinfo/sheepdog
