Xen devel - Jan 2006 - Re: [Xen-changelog] use format printf style to write to tracefd instead of using write syscall.

On Tue, Jan 24, 2006 at 03:22:08PM +0000, Xen patchbot -unstable
wrote:
> +void trace(const char *fmt, ...)
> +{
> +	va_list arglist;
> +	char *str;
> +	char sbuf[1024];
> +	int ret;
>  
>  	if (tracefd < 0)
>  		return;
>  
> +	/* try to use a static buffer */
sbuf isn''t static here...
> +	va_start(arglist, fmt);
> +	ret = vsnprintf(sbuf, 1024, fmt, arglist);
> +	va_end(arglist);
> +
> +	if (ret <= 1024) {
> +		write(tracefd, sbuf, ret);
>  		return;
vsnprintf can return a negative error value on output failure, in
which case calling write is not a good idea. Also, a return value of
1024 (sizeof(sbuf) instead?) or more means the output was truncated,
not sure if we want to do anything about it.

Also also, do we care if write() failed or did a short write?
> -			write(tracefd, "\n***\n", strlen("\n***\n"));
> +			trace("\n***\n");
trace("%s", "\n***\n")? it''s slightly slower but
more future proof.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Jan 24, 2006 at 08:14:40PM +0200, Muli Ben-Yehuda
wrote:
> > +	/* try to use a static buffer */
> 
> sbuf isn''t static here...
static as contrary to dynamicly allocated, not as a static C variable

do you prefer s/static/stack/ ?
> vsnprintf can return a negative error value on output failure, in
> which case calling write is not a good idea.
Did you really saw a *vSnprintf* fail ever with a negative value ?
I cannot find any reason that it will have an output error, which is the
only reason listed in the manual, since the output is a string in memory.

I think the return value of -1 is only related to:
fprintf, vfprintf, printf (redirection of output), vprintf

If that''s not true, I think that there''s lots of other place
that can
have this problem.
> Also, a return value of
> 1024 (sizeof(sbuf) instead?) or more means the output was truncated,
> not sure if we want to do anything about it.
If it was truncated, that because we force it to write no more than 1024
chars. the stack buffer is just there to avoid dynamic buffer
allocation in the majority of case.

just writing few chars without doing a malloc(N) is better I think,
but speed is not an issue here.
> Also also, do we care if write() failed or did a short write?
I don''t think so, but I don''t see why we shouldn''t
try to write
everything if we can, even at the price of a dynamic allocation .. if
dynamic memory allocation fail, we could just try to write what we can
and don''t really care.
> trace("%s", "\n***\n")? it''s slightly slower
but more future proof.
well it could not be that slower anyway actually:

the algo can just strlen+strcpy the first argument for the output
instead of going through the format string char by char for an escape.
for a short string that doesn''t really matters though.

I''m happy either way, but patch welcome, although that''s just
nitpicking
now ;)

-- 
Vincent Hanquez

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Jan 24, 2006 at 07:57:25PM +0100, Vincent Hanquez
wrote:
> On Tue, Jan 24, 2006 at 08:14:40PM +0200, Muli Ben-Yehuda wrote:
> > > +	/* try to use a static buffer */
> > 
> > sbuf isn''t static here...
> 
> static as contrary to dynamicly allocated, not as a static C variable
> 
> do you prefer s/static/stack/ ?
Doesn''t really matter.
> > vsnprintf can return a negative error value on output failure, in
> > which case calling write is not a good idea.
> 
> Did you really saw a *vsnprintf* fail ever with a negative value ?
Not that I can recall.
> I cannot find any reason that it will have an output error, which is the
> only reason listed in the manual, since the output is a string in memory.
> 
> I think the return value of -1 is only related to:
> fprintf, vfprintf, printf (redirection of output), vprintf
> 
> If that''s not true, I think that there''s lots of other
place that can
> have this problem.
It''s not exactly the bug of the decade, but I don''t see any
downsides
to handling all possible errors here since we''re already checking the
return value...
> > trace("%s", "\n***\n")? it''s slightly
slower but more future proof.
> 
> well it could not be that slower anyway actually:
> 
> the algo can just strlen+strcpy the first argument for the output
> instead of going through the format string char by char for an escape.
> for a short string that doesn''t really matters though.
> 
> I''m happy either way, but patch welcome, although that''s
just nitpicking
> now ;)
Guilty as charged :-)

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel