Serge E. Hallyn wrote:
> 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.
I agree. For the cases when there is not task, can put "none"
>
> Should we discuss here what we want an entry to look like? For both
> ckpt_write_err (to the checkpoint image) and ckpt_error()?
>
Yes please !
>>> 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.
How about using a dedicated buffer on @ctx for that ?
>
>>> + 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).
Again, a dedicated buffer on @ctx will be helpful here.
Oren.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
