Re: [PATCH] [RFC] Factor out shared portion of [_]ckpt_read_obj

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> It looked like a portion of one of these functions was missing
> the block "dispatching" errors so I factored this section out
> and called it from both functions. Was there a good reason it
> was missing?
> 
> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Good catch, but wouldn't it be better to instead factor the code
from label again: up to

	/* if len specified, enforce, else if maximum specified, enforce */
	if ((len && h->len != len) || (!len && max && h->len > max))
		return -EINVAL;

out of both into one function?  This feels more artificial, and I don't
see any other meaningful differences.

-serge

> ---
>  checkpoint/restart.c |   49 ++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> index 9b75de8..439206b 100644
> --- a/checkpoint/restart.c
> +++ b/checkpoint/restart.c
> @@ -238,6 +238,31 @@ static int _ckpt_read_objref(struct ckpt_ctx *ctx, struct ckpt_hdr *hh)
>  	return ret;
>  }
> 
> +/**
> + * _ckpt_read_early_dispatch - Dispatch ERRORs and OBJREFs; don't return them.
> + * @ctx: checkpoint context
> + * @h:   desired ckpt_hdr
> + *
> + * Returns -EFOO, 0 if it succeeded, 1 if h has not been dispatched yet
> + * (so it should be returned rather than ignored)
> + */
> +static int _ckpt_read_early_dispatch(struct ckpt_ctx *ctx, struct ckpt_hdr *h)
> +{
> +	int ret;
> +
> +	if (h->type == CKPT_HDR_ERROR) {
> +		ret = _ckpt_read_err(ctx, h);
> +		if (ret < 0)
> +			return ret;
> +		return 0;
> +	} else if (h->type == CKPT_HDR_OBJREF) {
> +		ret = _ckpt_read_objref(ctx, h);
> +		if (ret < 0)
> +			return ret;
> +		return 0;
> +	}
> +	return 1;
> +}
> 
>  /**
>   * _ckpt_read_obj - read an object (ckpt_hdr followed by payload)
> @@ -263,17 +288,11 @@ static int _ckpt_read_obj(struct ckpt_ctx *ctx, struct ckpt_hdr *h,
>  	if (h->len < sizeof(*h))
>  		return -EINVAL;
> 
> -	if (h->type == CKPT_HDR_ERROR) {
> -		ret = _ckpt_read_err(ctx, h);
> -		if (ret < 0)
> -			return ret;
> +	ret = _ckpt_read_early_dispatch(ctx, h);
> +	if (!ret)
>  		goto again;
> -	} else if (h->type == CKPT_HDR_OBJREF) {
> -		ret = _ckpt_read_objref(ctx, h);
> -		if (ret < 0)
> -			return ret;
> -		goto again;
> -	}
> +	else if (ret < 0)
> +		return ret;
> 
>  	/* if len specified, enforce, else if maximum specified, enforce */
>  	if ((len && h->len != len) || (!len && max && h->len > max))
> @@ -371,12 +390,12 @@ static void *ckpt_read_obj(struct ckpt_ctx *ctx, int len, int max)
>  	if (hh.len < sizeof(*h))
>  		return ERR_PTR(-EINVAL);
> 
> -	if (hh.type == CKPT_HDR_OBJREF) {
> -		ret = _ckpt_read_objref(ctx, &hh);
> -		if (ret < 0)
> -			return ERR_PTR(ret);
> +
> +	ret = _ckpt_read_early_dispatch(ctx, &hh);
> +	if (!ret)
>  		goto again;
> -	}
> +	else if (ret < 0)
> +		return ERR_PTR(ret);
> 
>  	/* if len specified, enforce, else if maximum specified, enforce */
>  	if ((len && hh.len != len) || (!len && max && hh.len > max))
> -- 
> 1.5.6.3
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers