[Devel] Re: [PATCH 01/12] define a new set of functions for error and debug logging

[Matt Helsley]

On Mon, Nov 02, 2009 at 04:23:29PM -0600, serue at us.ibm.com wrote:
> From: Serge E. Hallyn <serue at us.ibm.com>

<snip>

> +/*
> + * _ckpt_generate_fmt - handle the special flags in the enhanced format
> + * strings used by checkpoint/restart error messages.

<snip>

> + *	ckpt_write_err(ctx, "%(T)FILE flags %d %O %E\n", flags, objref, err);
> + *
> + * Must be called with ctx->fmt_buf_lock held.  The expanded format
> + * will be placed in ctx->fmt_buf.

nit:

I haven't gotten through the series but this looks like either a stale
comment (re: "ctx->fmt_buf_lock"), or it's premature -- I don't see you
introduce or use this lock in this patch.

> + */
> +void _ckpt_generate_fmt(struct ckpt_ctx *ctx, char *fmt)
> +{
> +	char *s = ctx->fmt;
> +	int len = 0;
> +	int first = 1;
> +
> +	for (; *fmt && len < CKPT_MSG_LEN; fmt++) {
> +		if (!is_special_flag(fmt)) {
> +			s[len++] = *fmt;
> +			continue;
> +		}
> +		if (!first)
> +			s[len++] = ' ';
> +		else
> +			first = 0;

I'm tempted to say leave the space out. The square brackets delimit
these enough. And if the caller really needs the space then it can be
done: "%(E) %(O)foo".

<snip>

> +void _ckpt_msg_append(struct ckpt_ctx *ctx, char *fmt, ...)
> +{
> +	va_list ap;
> +
> +	va_start(ap, fmt);
> +	_ckpt_msg_appendv(ctx, fmt, ap);
> +	va_end(ap);
> +}
> +
> +void _ckpt_msg_complete(struct ckpt_ctx *ctx)
> +{
> +	int ret;

In the case of do_ckpt_msg() it's clear this won't happen. However
since _do_ckpt_msg() is separate from _ckpt_msg_complete() (looking
at the second patch) it's not clear that this will always be maintained
correctly.

BUG_ON(ctx->msglen < 1); /* detect msg_complete calls without any
				messages */

> +
> +	if (ctx->kflags & CKPT_CTX_CHECKPOINT) {
> +		ret = ckpt_write_obj_type(ctx, NULL, 0, CKPT_HDR_ERROR);
> +		if (!ret)
> +			ret = ckpt_write_string(ctx, ctx->msg, ctx->msglen);
> +		if (ret < 0)
> +			printk(KERN_NOTICE "c/r: error string unsaved (%d): %s\n",
> +			       ret, ctx->msg+1);
> +	}
> +#if 0
> +	if (ctx->logfile) {
> +		mm_segment_t fs = get_fs();
> +		set_fs(KERNEL_DS);
> +		ret = _ckpt_kwrite(ctx->logfile, ctx->msg+1, ctx->msglen-1);
> +		set_fs(fs);
> +	}
> +#endif
> +#ifdef CONFIG_CHECKPOINT_DEBUG
> +	printk(KERN_DEBUG "%s", ctx->msg+1);
> +#endif

Then clear:

	ctx->msglen = 0;

<snip>

> +/*
> + * Expand fmt into ctx->fmt.
> + * This expands enhanced format flags such as %(T), which takes no
> + * arguments, and %(E), which will require a properly positioned
> + * int error argument.  Flags include:
> + *   T: Task
> + *   E: Errno
> + *   O: Objref
> + *   P: Pointer
> + *   S: string
> + *   V: Variable (symbol)
> + * May be called under spinlock.
> + * Must be called under ckpt_msg_lock.
> + */
> +extern void _ckpt_generate_fmt(struct ckpt_ctx *ctx, char *fmt);

I'm not seeing why this is needed outside checkpoint/sys.c. In a future
patch?

<snip>

> +#define _ckpt_err(ctx, fmt, args...) do { \
> +	_do_ckpt_msg(ctx, "[ error %s:%d]" fmt, __func__, __LINE__, ##args);	\
> +} while (0)
> +
> +/* ckpt_logmsg() or ckpt_msg() will do do_ckpt_msg with an added
> + * prefix */

nit: This comment is about future code. It should perhaps be part of
	the patch description rather than in the code as comment.

<snip>
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers