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

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

[Matt Helsley]

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>
---
 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

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

[Serge E. Hallyn]

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

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

[Oren Laadan]

Serge E. Hallyn wrote:
> 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.
> 

Yes, I'll do just that.
(Thanks for the fix, Matt).

Oren.

[PATCH] c/r: factor out objref handling from {_,}ckpt_read_obj()

[Oren Laadan]

As noted by Matt Helsley, a portion of one of these functions was
missing the block "dispatching" errors. Otherwise the handling of
shard objects is pretty much the same, so this patch factors out the
common code.

Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
---
 checkpoint/restart.c |   58 ++++++++++++++++++++++++++-----------------------
 1 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/checkpoint/restart.c b/checkpoint/restart.c
index 9b75de8..130b4b2 100644
--- a/checkpoint/restart.c
+++ b/checkpoint/restart.c
@@ -238,6 +238,35 @@ static int _ckpt_read_objref(struct ckpt_ctx *ctx, struct ckpt_hdr *hh)
 	return ret;
 }
 
+/**
+ * ckpt_read_obj_dispatch - dispatch ERRORs and OBJREFs; don't return them
+ * @ctx: checkpoint context
+ * @h: desired ckpt_hdr
+ */
+static int ckpt_read_obj_dispatch(struct ckpt_ctx *ctx, struct ckpt_hdr *h)
+{
+	int ret;
+
+	while (1) {
+		ret = ckpt_kread(ctx, h, sizeof(*h));
+		if (ret < 0)
+			return ret;
+		_ckpt_debug(CKPT_DRW, "type %d len %d\n", h->type, h->len);
+		if (h->len < sizeof(*h))
+			return -EINVAL;
+
+		if (h->type == CKPT_HDR_ERROR) {
+			ret = _ckpt_read_err(ctx, h);
+			if (ret < 0)
+				return ret;
+		} else if (h->type == CKPT_HDR_OBJREF) {
+			ret = _ckpt_read_objref(ctx, h);
+			if (ret < 0)
+				return ret;
+		} else
+			return 0;
+	}
+}
 
 /**
  * _ckpt_read_obj - read an object (ckpt_hdr followed by payload)
@@ -254,26 +283,11 @@ static int _ckpt_read_obj(struct ckpt_ctx *ctx, struct ckpt_hdr *h,
 {
 	int ret;
 
- again:
-	ret = ckpt_kread(ctx, h, sizeof(*h));
+	ret = ckpt_read_obj_dispatch(ctx, h);
 	if (ret < 0)
 		return ret;
 	_ckpt_debug(CKPT_DRW, "type %d len %d(%d,%d)\n",
 		    h->type, h->len, len, max);
-	if (h->len < sizeof(*h))
-		return -EINVAL;
-
-	if (h->type == CKPT_HDR_ERROR) {
-		ret = _ckpt_read_err(ctx, h);
-		if (ret < 0)
-			return ret;
-		goto again;
-	} else if (h->type == CKPT_HDR_OBJREF) {
-		ret = _ckpt_read_objref(ctx, h);
-		if (ret < 0)
-			return ret;
-		goto again;
-	}
 
 	/* if len specified, enforce, else if maximum specified, enforce */
 	if ((len && h->len != len) || (!len && max && h->len > max))
@@ -362,21 +376,11 @@ static void *ckpt_read_obj(struct ckpt_ctx *ctx, int len, int max)
 	struct ckpt_hdr *h;
 	int ret;
 
- again:
-	ret = ckpt_kread(ctx, &hh, sizeof(hh));
+	ret = ckpt_read_obj_dispatch(ctx, &hh);
 	if (ret < 0)
 		return ERR_PTR(ret);
 	_ckpt_debug(CKPT_DRW, "type %d len %d(%d,%d)\n",
 		    hh.type, hh.len, len, 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);
-		goto again;
-	}
 
 	/* if len specified, enforce, else if maximum specified, enforce */
 	if ((len && hh.len != len) || (!len && max && hh.len > max))
-- 
1.6.0.4

Re: [PATCH] c/r: factor out objref handling from {_,}ckpt_read_obj()

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> As noted by Matt Helsley, a portion of one of these functions was
> missing the block "dispatching" errors. Otherwise the handling of
> shard objects is pretty much the same, so this patch factors out the
> common code.
> 
> Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Although you could go one step further and pass in len and max
and check those in ckpt_read_obj_dispatch() as well.

-serge

> ---
>  checkpoint/restart.c |   58 ++++++++++++++++++++++++++-----------------------
>  1 files changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> index 9b75de8..130b4b2 100644
> --- a/checkpoint/restart.c
> +++ b/checkpoint/restart.c
> @@ -238,6 +238,35 @@ static int _ckpt_read_objref(struct ckpt_ctx *ctx, struct ckpt_hdr *hh)
>  	return ret;
>  }
> 
> +/**
> + * ckpt_read_obj_dispatch - dispatch ERRORs and OBJREFs; don't return them
> + * @ctx: checkpoint context
> + * @h: desired ckpt_hdr
> + */
> +static int ckpt_read_obj_dispatch(struct ckpt_ctx *ctx, struct ckpt_hdr *h)
> +{
> +	int ret;
> +
> +	while (1) {
> +		ret = ckpt_kread(ctx, h, sizeof(*h));
> +		if (ret < 0)
> +			return ret;
> +		_ckpt_debug(CKPT_DRW, "type %d len %d\n", h->type, h->len);
> +		if (h->len < sizeof(*h))
> +			return -EINVAL;
> +
> +		if (h->type == CKPT_HDR_ERROR) {
> +			ret = _ckpt_read_err(ctx, h);
> +			if (ret < 0)
> +				return ret;
> +		} else if (h->type == CKPT_HDR_OBJREF) {
> +			ret = _ckpt_read_objref(ctx, h);
> +			if (ret < 0)
> +				return ret;
> +		} else
> +			return 0;
> +	}
> +}
> 
>  /**
>   * _ckpt_read_obj - read an object (ckpt_hdr followed by payload)
> @@ -254,26 +283,11 @@ static int _ckpt_read_obj(struct ckpt_ctx *ctx, struct ckpt_hdr *h,
>  {
>  	int ret;
> 
> - again:
> -	ret = ckpt_kread(ctx, h, sizeof(*h));
> +	ret = ckpt_read_obj_dispatch(ctx, h);
>  	if (ret < 0)
>  		return ret;
>  	_ckpt_debug(CKPT_DRW, "type %d len %d(%d,%d)\n",
>  		    h->type, h->len, len, max);
> -	if (h->len < sizeof(*h))
> -		return -EINVAL;
> -
> -	if (h->type == CKPT_HDR_ERROR) {
> -		ret = _ckpt_read_err(ctx, h);
> -		if (ret < 0)
> -			return ret;
> -		goto again;
> -	} else if (h->type == CKPT_HDR_OBJREF) {
> -		ret = _ckpt_read_objref(ctx, h);
> -		if (ret < 0)
> -			return ret;
> -		goto again;
> -	}
> 
>  	/* if len specified, enforce, else if maximum specified, enforce */
>  	if ((len && h->len != len) || (!len && max && h->len > max))
> @@ -362,21 +376,11 @@ static void *ckpt_read_obj(struct ckpt_ctx *ctx, int len, int max)
>  	struct ckpt_hdr *h;
>  	int ret;
> 
> - again:
> -	ret = ckpt_kread(ctx, &hh, sizeof(hh));
> +	ret = ckpt_read_obj_dispatch(ctx, &hh);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
>  	_ckpt_debug(CKPT_DRW, "type %d len %d(%d,%d)\n",
>  		    hh.type, hh.len, len, 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);
> -		goto again;
> -	}
> 
>  	/* if len specified, enforce, else if maximum specified, enforce */
>  	if ((len && hh.len != len) || (!len && max && hh.len > max))
> -- 
> 1.6.0.4
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH] c/r: factor out objref handling from {_,}ckpt_read_obj()

[Matt Helsley]

On Thu, Oct 29, 2009 at 02:52:13PM -0500, Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> > As noted by Matt Helsley, a portion of one of these functions was
> > missing the block "dispatching" errors. Otherwise the handling of
> > shard objects is pretty much the same, so this patch factors out the
> > common code.
> > 
> > Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
> 
> Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Although you could go one step further and pass in len and max
> and check those in ckpt_read_obj_dispatch() as well.

Actually I think it should only "do the dispatch" and nothing else. Otherwise
it becomes "ckpt_alloc_and_read_and_dispatch_and_check_obj()" (ok, I'm
exaagerating a bit..).

There's alot of variation in these ckpt_read* functions based on issues
like:
	1. When to alloc the header (and how much to alloc)
	2. When/what to compare the header len to
	3. If/When to alloc the payload
	4. If/When to read the payload

So merging the length/max parameter check with
ckpt_read_obj_dispatch() may limit its usefulness or cause its parameter
list to explode.

That said, the looping, reading, and minimal length checking
performed here looks pretty nice.

Cheers,
	-Matt Helsley

> 
> -serge
> 
> > ---
> >  checkpoint/restart.c |   58 ++++++++++++++++++++++++++-----------------------
> >  1 files changed, 31 insertions(+), 27 deletions(-)
> > 
> > diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> > index 9b75de8..130b4b2 100644
> > --- a/checkpoint/restart.c
> > +++ b/checkpoint/restart.c
> > @@ -238,6 +238,35 @@ static int _ckpt_read_objref(struct ckpt_ctx *ctx, struct ckpt_hdr *hh)
> >  	return ret;
> >  }
> > 
> > +/**
> > + * ckpt_read_obj_dispatch - dispatch ERRORs and OBJREFs; don't return them
> > + * @ctx: checkpoint context
> > + * @h: desired ckpt_hdr
> > + */
> > +static int ckpt_read_obj_dispatch(struct ckpt_ctx *ctx, struct ckpt_hdr *h)
> > +{
> > +	int ret;
> > +
> > +	while (1) {
> > +		ret = ckpt_kread(ctx, h, sizeof(*h));
> > +		if (ret < 0)
> > +			return ret;
> > +		_ckpt_debug(CKPT_DRW, "type %d len %d\n", h->type, h->len);
> > +		if (h->len < sizeof(*h))
> > +			return -EINVAL;
> > +
> > +		if (h->type == CKPT_HDR_ERROR) {
> > +			ret = _ckpt_read_err(ctx, h);
> > +			if (ret < 0)
> > +				return ret;
> > +		} else if (h->type == CKPT_HDR_OBJREF) {
> > +			ret = _ckpt_read_objref(ctx, h);
> > +			if (ret < 0)
> > +				return ret;
> > +		} else
> > +			return 0;
> > +	}
> > +}
> > 
> >  /**
> >   * _ckpt_read_obj - read an object (ckpt_hdr followed by payload)
> > @@ -254,26 +283,11 @@ static int _ckpt_read_obj(struct ckpt_ctx *ctx, struct ckpt_hdr *h,
> >  {
> >  	int ret;
> > 
> > - again:
> > -	ret = ckpt_kread(ctx, h, sizeof(*h));
> > +	ret = ckpt_read_obj_dispatch(ctx, h);
> >  	if (ret < 0)
> >  		return ret;
> >  	_ckpt_debug(CKPT_DRW, "type %d len %d(%d,%d)\n",
> >  		    h->type, h->len, len, max);
> > -	if (h->len < sizeof(*h))
> > -		return -EINVAL;
> > -
> > -	if (h->type == CKPT_HDR_ERROR) {
> > -		ret = _ckpt_read_err(ctx, h);
> > -		if (ret < 0)
> > -			return ret;
> > -		goto again;
> > -	} else if (h->type == CKPT_HDR_OBJREF) {
> > -		ret = _ckpt_read_objref(ctx, h);
> > -		if (ret < 0)
> > -			return ret;
> > -		goto again;
> > -	}
> > 
> >  	/* if len specified, enforce, else if maximum specified, enforce */
> >  	if ((len && h->len != len) || (!len && max && h->len > max))
> > @@ -362,21 +376,11 @@ static void *ckpt_read_obj(struct ckpt_ctx *ctx, int len, int max)
> >  	struct ckpt_hdr *h;
> >  	int ret;
> > 
> > - again:
> > -	ret = ckpt_kread(ctx, &hh, sizeof(hh));
> > +	ret = ckpt_read_obj_dispatch(ctx, &hh);
> >  	if (ret < 0)
> >  		return ERR_PTR(ret);
> >  	_ckpt_debug(CKPT_DRW, "type %d len %d(%d,%d)\n",
> >  		    hh.type, hh.len, len, 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);
> > -		goto again;
> > -	}
> > 
> >  	/* if len specified, enforce, else if maximum specified, enforce */
> >  	if ((len && hh.len != len) || (!len && max && hh.len > max))
> > -- 
> > 1.6.0.4
> > 
> > _______________________________________________
> > Containers mailing list
> > Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> > https://lists.linux-foundation.org/mailman/listinfo/containers
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH] c/r: factor out objref handling from {_,}ckpt_read_obj()

[Matt Helsley]

On Thu, Oct 29, 2009 at 01:50:12PM -0400, Oren Laadan wrote:
> As noted by Matt Helsley, a portion of one of these functions was
> missing the block "dispatching" errors. Otherwise the handling of
> shard objects is pretty much the same, so this patch factors out the
> common code.
> 
> Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>

Looks good.

Reviewed-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  checkpoint/restart.c |   58 ++++++++++++++++++++++++++-----------------------
>  1 files changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> index 9b75de8..130b4b2 100644
> --- a/checkpoint/restart.c
> +++ b/checkpoint/restart.c
> @@ -238,6 +238,35 @@ static int _ckpt_read_objref(struct ckpt_ctx *ctx, struct ckpt_hdr *hh)
>  	return ret;
>  }
> 
> +/**
> + * ckpt_read_obj_dispatch - dispatch ERRORs and OBJREFs; don't return them
> + * @ctx: checkpoint context
> + * @h: desired ckpt_hdr
> + */
> +static int ckpt_read_obj_dispatch(struct ckpt_ctx *ctx, struct ckpt_hdr *h)
> +{
> +	int ret;
> +
> +	while (1) {
> +		ret = ckpt_kread(ctx, h, sizeof(*h));
> +		if (ret < 0)
> +			return ret;
> +		_ckpt_debug(CKPT_DRW, "type %d len %d\n", h->type, h->len);
> +		if (h->len < sizeof(*h))
> +			return -EINVAL;
> +
> +		if (h->type == CKPT_HDR_ERROR) {
> +			ret = _ckpt_read_err(ctx, h);
> +			if (ret < 0)
> +				return ret;
> +		} else if (h->type == CKPT_HDR_OBJREF) {
> +			ret = _ckpt_read_objref(ctx, h);
> +			if (ret < 0)
> +				return ret;
> +		} else
> +			return 0;
> +	}
> +}
> 
>  /**
>   * _ckpt_read_obj - read an object (ckpt_hdr followed by payload)
> @@ -254,26 +283,11 @@ static int _ckpt_read_obj(struct ckpt_ctx *ctx, struct ckpt_hdr *h,
>  {
>  	int ret;
> 
> - again:
> -	ret = ckpt_kread(ctx, h, sizeof(*h));
> +	ret = ckpt_read_obj_dispatch(ctx, h);
>  	if (ret < 0)
>  		return ret;
>  	_ckpt_debug(CKPT_DRW, "type %d len %d(%d,%d)\n",
>  		    h->type, h->len, len, max);
> -	if (h->len < sizeof(*h))
> -		return -EINVAL;
> -
> -	if (h->type == CKPT_HDR_ERROR) {
> -		ret = _ckpt_read_err(ctx, h);
> -		if (ret < 0)
> -			return ret;
> -		goto again;
> -	} else if (h->type == CKPT_HDR_OBJREF) {
> -		ret = _ckpt_read_objref(ctx, h);
> -		if (ret < 0)
> -			return ret;
> -		goto again;
> -	}
> 
>  	/* if len specified, enforce, else if maximum specified, enforce */
>  	if ((len && h->len != len) || (!len && max && h->len > max))
> @@ -362,21 +376,11 @@ static void *ckpt_read_obj(struct ckpt_ctx *ctx, int len, int max)
>  	struct ckpt_hdr *h;
>  	int ret;
> 
> - again:
> -	ret = ckpt_kread(ctx, &hh, sizeof(hh));
> +	ret = ckpt_read_obj_dispatch(ctx, &hh);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
>  	_ckpt_debug(CKPT_DRW, "type %d len %d(%d,%d)\n",
>  		    hh.type, hh.len, len, 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);
> -		goto again;
> -	}
> 
>  	/* if len specified, enforce, else if maximum specified, enforce */
>  	if ((len && hh.len != len) || (!len && max && hh.len > max))
> -- 
> 1.6.0.4
>