From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Serge E. Hallyn" Subject: Re: [RFC PATCH 11/17] define function to print error messages to user log Date: Wed, 28 Oct 2009 15:54:24 -0500 Message-ID: <20091028205424.GA27394@us.ibm.com> References: <1256683587-23961-1-git-send-email-serge@us.ibm.com> <1256683587-23961-12-git-send-email-serge@us.ibm.com> <20091028181415.GB14023@count0.beaverton.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20091028181415.GB14023-52DBMbEzqgQ/wnmkkaCWp/UQ3DHhIser@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Matt Helsley Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org List-Id: containers.vger.kernel.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); > ... ... > va_end(aq); > va_copy(aq, ap); > ... ... > 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