[PATCH] Add a ckpt_read_string() function (v3)

[PATCH] Add a ckpt_read_string() function (v3)

[Dan Smith]

Add a ckpt_read_string() function to allow reading of a variable-length
(but length-capped) string from the checkpoint stream.

Changes in v3:
 - Return immediately on allocation failure instead of falling through to the
   inevitable crash

Changes in v2:
 - Avoid memcpy() by reading into the allocated buffer directly

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/restart.c       |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/checkpoint.h |    1 +
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/checkpoint/restart.c b/checkpoint/restart.c
index 65cafd9..b1ffc54 100644
--- a/checkpoint/restart.c
+++ b/checkpoint/restart.c
@@ -285,6 +285,42 @@ int ckpt_read_consume(struct ckpt_ctx *ctx, int len, int type)
 	return ret;
 }
 
+/**
+ * ckpt_read_string - read a string (variable length)
+ * @ctx: checkpoint context
+ * @str: pointer to buffer to store allocated string (caller must kfree())
+ * @max: maximum acceptable length
+ *
+ * This can be used to read a variable-length string from the checkpoint
+ * stream. @max limits the size of the resulting buffer.  Returns zero on
+ * success, negative on failure.
+ */
+int ckpt_read_string(struct ckpt_ctx *ctx, char **str, int max)
+{
+	int len;
+	int ret = 0;
+
+	*str = NULL;
+
+	len = _ckpt_read_obj_type(ctx, NULL, 0, CKPT_HDR_STRING);
+	if (len < 0)
+		return len;
+	else if (len > max)
+		return -EINVAL;
+
+	*str = kzalloc(len + 1, GFP_KERNEL);
+	if (!*str)
+		return -ENOMEM;
+
+	ret = ckpt_kread(ctx, *str, len);
+	if (ret < 0) {
+		kfree(*str);
+		*str = NULL;
+	}
+
+	return ret;
+}
+
 /***********************************************************************
  * Restart
  */
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index 87b683b..a6935b3 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -68,6 +68,7 @@ extern int _ckpt_read_obj_type(struct ckpt_ctx *ctx,
 extern int _ckpt_read_nbuffer(struct ckpt_ctx *ctx, void *ptr, int len);
 extern int _ckpt_read_buffer(struct ckpt_ctx *ctx, void *ptr, int len);
 extern int _ckpt_read_string(struct ckpt_ctx *ctx, void *ptr, int len);
+extern int ckpt_read_string(struct ckpt_ctx *ctx, char **str, int max);
 extern void *ckpt_read_obj_type(struct ckpt_ctx *ctx, int len, int type);
 extern void *ckpt_read_buf_type(struct ckpt_ctx *ctx, int len, int type);
 extern int ckpt_read_consume(struct ckpt_ctx *ctx, int len, int type);
-- 
1.6.0.4

Re: [PATCH] Add a ckpt_read_string() function (v3)

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Add a ckpt_read_string() function to allow reading of a variable-length
> (but length-capped) string from the checkpoint stream.
> 
> Changes in v3:
>  - Return immediately on allocation failure instead of falling through to the
>    inevitable crash
> 
> Changes in v2:
>  - Avoid memcpy() by reading into the allocated buffer directly
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

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

> ---
>  checkpoint/restart.c       |   36 ++++++++++++++++++++++++++++++++++++
>  include/linux/checkpoint.h |    1 +
>  2 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> index 65cafd9..b1ffc54 100644
> --- a/checkpoint/restart.c
> +++ b/checkpoint/restart.c
> @@ -285,6 +285,42 @@ int ckpt_read_consume(struct ckpt_ctx *ctx, int len, int type)
>  	return ret;
>  }
> 
> +/**
> + * ckpt_read_string - read a string (variable length)
> + * @ctx: checkpoint context
> + * @str: pointer to buffer to store allocated string (caller must kfree())
> + * @max: maximum acceptable length
> + *
> + * This can be used to read a variable-length string from the checkpoint
> + * stream. @max limits the size of the resulting buffer.  Returns zero on
> + * success, negative on failure.
> + */
> +int ckpt_read_string(struct ckpt_ctx *ctx, char **str, int max)
> +{
> +	int len;
> +	int ret = 0;
> +
> +	*str = NULL;
> +
> +	len = _ckpt_read_obj_type(ctx, NULL, 0, CKPT_HDR_STRING);
> +	if (len < 0)
> +		return len;
> +	else if (len > max)
> +		return -EINVAL;
> +
> +	*str = kzalloc(len + 1, GFP_KERNEL);
> +	if (!*str)
> +		return -ENOMEM;
> +
> +	ret = ckpt_kread(ctx, *str, len);
> +	if (ret < 0) {
> +		kfree(*str);
> +		*str = NULL;
> +	}
> +
> +	return ret;
> +}
> +
>  /***********************************************************************
>   * Restart
>   */
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index 87b683b..a6935b3 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -68,6 +68,7 @@ extern int _ckpt_read_obj_type(struct ckpt_ctx *ctx,
>  extern int _ckpt_read_nbuffer(struct ckpt_ctx *ctx, void *ptr, int len);
>  extern int _ckpt_read_buffer(struct ckpt_ctx *ctx, void *ptr, int len);
>  extern int _ckpt_read_string(struct ckpt_ctx *ctx, void *ptr, int len);
> +extern int ckpt_read_string(struct ckpt_ctx *ctx, char **str, int max);
>  extern void *ckpt_read_obj_type(struct ckpt_ctx *ctx, int len, int type);
>  extern void *ckpt_read_buf_type(struct ckpt_ctx *ctx, int len, int type);
>  extern int ckpt_read_consume(struct ckpt_ctx *ctx, int len, int type);
> -- 
> 1.6.0.4
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH] Add a ckpt_read_string() function (v3)

[Nathan Lynch]

Hi Dan,

Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
> +/**
> + * ckpt_read_string - read a string (variable length)
> + * @ctx: checkpoint context
> + * @str: pointer to buffer to store allocated string (caller must kfree())
> + * @max: maximum acceptable length
> + *
> + * This can be used to read a variable-length string from the checkpoint
> + * stream. @max limits the size of the resulting buffer.  Returns zero on
> + * success, negative on failure.
> + */
> +int ckpt_read_string(struct ckpt_ctx *ctx, char **str, int max)
> +{
> +	int len;
> +	int ret = 0;
> +
> +	*str = NULL;
> +
> +	len = _ckpt_read_obj_type(ctx, NULL, 0, CKPT_HDR_STRING);
> +	if (len < 0)
> +		return len;
> +	else if (len > max)
> +		return -EINVAL;
> +
> +	*str = kzalloc(len + 1, GFP_KERNEL);
> +	if (!*str)
> +		return -ENOMEM;
> +
> +	ret = ckpt_kread(ctx, *str, len);
> +	if (ret < 0) {
> +		kfree(*str);
> +		*str = NULL;
> +	}
> +
> +	return ret;
> +}

Maybe I'm missing the utility this provides, but I think this helper is
trying to do too much.  It's often more straightforward to allow (or
force, depending on your POV) the caller to handle the allocation if
it's going to have to free it anyway.  It also allows the caller to
choose its own allocation method, e.g. on-stack for small things, or
maybe some filesystem-related code will need to use GFP_NOFS.

So I imagine a simple wrapper to probe the length of the string would
suffice?  Something like

ssize_t ckpt_next_string_length(struct ckpt_ctx *ctx)
{
        return _ckpt_read_obj_type(ctx, NULL, 0, CKPT_HDR_STRING);
}

...

len = ckpt_next_string_length(ctx);
if (len < 1 || len > max)
   return -EINVAL;

buf = kzalloc(len, GFP_KERNEL);
if (!buf)
   return -ENOMEM;

rc = ckpt_kread(ctx, buf, len);
if (rc < 0) {
   kfree(buf)
   return -EINVAL;
}

kfree(buf);

Re: [PATCH] Add a ckpt_read_string() function (v3)

[Dan Smith]

NL> So I imagine a simple wrapper to probe the length of the string
NL> would suffice?  Something like

We've been there already, a few iterations ago, right?  It's been
revised so many times, I've lost count, but I believe that's what I
started with.

I'd really like Oren to own this helper function along with all the
rest, and I'll use whatever API is available at any given point :)

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org