On Mon, Nov 02, 2009 at 04:23:29PM -0600, serue@xxxxxxxxxx wrote:
> From: Serge E. Hallyn <serue@xxxxxxxxxx>
<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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
