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

[Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>]

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