Quoting Matt Helsley (matthltc@xxxxxxxxxx):
> > @@ -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
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
