Re: [PATCH 01/17] ckpt_write_err: use single format with %(T) style tokens

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Matt Helsley originally suggested this to avoid having two
> format strings.  This is not bisect-safe and therefore not
> even compile-tested.  Every call to ckpt_write_err must be
> updated to use a single format.
> 
> Changelog:
> 	Oct 29: merge with the next patch, moving ckpt_generate_fmt()
> 		into checkpoing/sys.c and making it non-static so that
> 		we can use it in ckpt_error().
> 	Oct 28: also fix comment above ckpt_generate_fmt()
> 	Oct 27: ensure %(T) has a closing paren
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/checkpoint.c    |  105 +++-----------------------------------------
>  checkpoint/sys.c           |   95 +++++++++++++++++++++++++++++++++++++++
>  include/linux/checkpoint.h |   13 +++--
>  3 files changed, 110 insertions(+), 103 deletions(-)
> 
> diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> index 6eb8f3b..c6be4f9 100644
> --- a/checkpoint/checkpoint.c
> +++ b/checkpoint/checkpoint.c
> @@ -96,104 +96,15 @@ int ckpt_write_string(struct ckpt_ctx *ctx, char *str, int len)
>  	return ckpt_write_obj_type(ctx, str, len, CKPT_HDR_STRING);
>  }
>  

[...]

> diff --git a/checkpoint/sys.c b/checkpoint/sys.c
> index 260a1ee..9b2de18 100644
> --- a/checkpoint/sys.c
> +++ b/checkpoint/sys.c
> @@ -339,6 +339,101 @@ int walk_task_subtree(struct task_struct *root,
>  	return (ret < 0 ? ret : total);
>  }
>  

Some pieces of the comment below are obsolete...

> +/*
> + * ckpt_generate_fmt - generate standard checkpoint error message
> + * @ctx: checkpoint context
> + * @fmt: message format
> + *
> + * This generates a unified format of checkpoint error messages, to
> + * ease (after the failure) inspection by userspace tools. It converts
> + * the (printf) message @fmt into a new format: "[PREFMT]: fmt".
> + *
> + * PREFMT is constructed from @fmt by subtituting format snippets
> + * according to the contents of @fmt.  The format characters in
> + * @fmt can be %(E) (error), %(O) (objref), %(P) (pointer), %(S) (string),
> + * %(C) (bytes read/written out of checkpoint image so far), * and %(V)
							      ^^^^^
> + * (variable/symbol). For example, %(E) will generate a "err %d"
> + * in PREFMT.
      ^^^^^^^^^
> + *
> + * If @fmt begins with %(T), PREFMT will begin with "pid %d tsk %s"
	    ^^^^^^^^^
> + * with the pid and the tsk->comm of the currently checkpointed task.
> + * The latter is taken from ctx->tsk, and is it the responsbilility of
						typo  ^^^^^^^^^^^^^^^^^
> + * the caller to have a valid pointer there (in particular, functions
> + * that iterate on the processes: collect_objects, checkpoint_task,
> + * and tree_count_tasks).
> + *
> + * The caller of ckpt_write_err() and _ckpt_write_err() must provide
		    ^^^^^^^^^^^^^^^^	 ^^^^^^^^^^^^^^^^^
> + * the additional variabes, in order, to match the @fmt0 (except for
		typo ^^^^^^^^			     ^^^^^^^
> + * the T key), e.g.:
      ^^^^^^^^^^  (obsolete)
> + *
> + *   ckpt_writ_err(ctx, "TEO", "FILE flags %d", err, objref, flags)
			  ^^^^^^^^
> + *
> + * Here, T is simply passed, E expects an integer (err), O expects an
> + * integer (objref), and the last argument matches the format string.

Rewrite example...

> + *
> + * XXX Do we want 'T' and 'C' to simply always be prepended?
> + */
> +char *ckpt_generate_fmt(struct ckpt_ctx *ctx, char *fmt)
> +{
> +	char *format;
> +	int alloclen, len = 0;
> +	int first = 1;
> +
> +	/*
> +	 * 17 for "pid %d" (plus space)
> +	 * 21 for "tsk %s" (tsk->comm)
> +	 * up to 8 per varfmt entry
		      ^^^^^^^^

> +	 */
> +	alloclen = 37 + 8 * strlen(fmt);

This calculation assumed that @fmt had only format string...
At the very minimum you could take strlen(fmt)/3 (+1 to round up)

I thought you were going to use a @ctx->buffer or something ?

> +	format = kzalloc(alloclen, GFP_KERNEL);
> +	if (!format)
> +		return NULL;
> +
> +	for (; *fmt; fmt++) {
> +		BUG_ON(len > alloclen);
> +		if (*fmt != '%' || fmt[1] != '(' || fmt[3] != ')') {

This is still a bit risky .. how about adding
	|| fmt[2] == '\0'
between the 2nd and 3rd test ?

> +			format[len++] = *fmt;
> +			continue;
> +		}
> +		if (!first)
> +			format[len++] = ' ';
> +		else
> +			first = 0;

[...]

Oren.