Re: [RFC PATCH 11/17] define function to print error messages to user log

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> > @@ -401,6 +409,9 @@ char *ckpt_generate_fmt(struct ckpt_ctx *ctx, char *fmt)
> >  		case 'E':
> >  			len += sprintf(format+len, "[%s]", "err %d");
> >  			break;
> > +		case 'C': /* count of bytes read/written to checkpoint image */
> > +			len += sprintf(format+len, "[%s]", "pos %d");
> > +			break;
> 
> Instead we could always output ckpt->total and then we wouldn't need %(C). I
> suspect it's such a useful piece of information that it'll be repeated
> in many/all format strings eventually.

Yes, likewise %(T).  If that's what we want to do.

Should we discuss here what we want an entry to look like?  For both
ckpt_write_err (to the checkpoint image) and ckpt_error()?

> >  		case 'O':
> >  			len += sprintf(format+len, "[%s]", "obj %d");
> >  			break;
> > @@ -435,6 +446,51 @@ char *ckpt_generate_fmt(struct ckpt_ctx *ctx, char *fmt)
> >  	return format;
> >  }
> > 
> > +void ckpt_log_error(struct ckpt_ctx *ctx, char *fmt, ...)
> > +{
> > +	mm_segment_t fs;
> > +	struct file *file;
> > +	int count;
> > +	va_list ap, aq, az;
> > +	char *format;
> > +	char buf[200], *bufp = buf;
> 
> I believe this buffer is too big for a kernel stack -- especially
> for ckpt_log_error() which might be invoked "deep" in
> the kernel stack.

200 bytes?  Well, I guess I can try with 50 which still may often be
enough.

> > +	if (!ctx || !ctx->logfile)
> > +		return;
> > +	file = ctx->logfile;
> > +
> > +	va_start(ap, fmt);
> > +	format = ckpt_generate_fmt(ctx, fmt);
> > +	va_copy(aq, ap);
> > +	va_copy(az, ap);
> > +	/* I'm not clear here - can I re-use aq, or do i need
> > +	 * a third copy? */
> 
> I'm no varargs expert but I have re-read the man page and
> seen a purported snippet of the standard. :)
> 
> I think you need a third copy operation but you may only need
> two va_lists so long as you do a va_end before the next va_copy:
> 
> 	va_copy(aq, ap);
> 	... <use aq> ...
> 	va_end(aq);
> 	va_copy(aq, ap);
> 	... <use aq> ...
> 	va_end(aq);
> 	...
> 	va_end(ap); 
> 
> Based on my reading it sounded like some arch/ABIs require space
> proportional to the number of arguments for each un-va_end-ed copy.

Ok, I'll do that, thanks.

> > +	count = vsnprintf(bufp, 200, format ? : fmt, aq);
> 
> BTW -- I think you can use snprintf() without the buffer and length
> arguments if you just need the length calculated. Perhaps the same
> is possible with vsnprintf():
> 
> 	count = vsnprintf(NULL, 0, format ? : fmt, aq);
> 
> If that works with vsnprintf() too then you could get rid of the
> stack buf and always kmalloc the space..

Hmm, yeah...  though i don't know that I *want* to always kmalloc
the space :)  It does look like it should work (though no comment
to that effect), but there is no speed advantage, save a bit of
memcpy (vs. always having a kmalloc).

-serge