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

[Serge E. Hallyn]

Quoting Matt Helsley (matthltc at us.ibm.com):
> 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>
...
> 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.

Yup, stale comment, thanks.  No fmt_buf_lock this time around.

> > + */
> > +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".

Works for me.

> > +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 */

Well, just return if ctx->msglen < 1, but point taken, will fix.

> 
> > +
> > +	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?

Hmm, no...  in an earlier patchset.  I don't think there will be any
external callers of this fn.  Should be static.

> > +#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.

Sure - and if there are no basic objectiosn then apart from addressing
these I'll also aim to put some meaningful ckpt_errs() on restart failure
paths tomorrow.

thanks,
-serge
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers