[sheepdog] [PATCH] sheep: cleanup do_recover_object

[Liu Yuan]

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 <hch at lst.de>
> 
> 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