[OPEN-ILS-DEV] C nits: utils.c (Part 2)

[Scott McKellar]

--- Bill Erickson <billserickson at gmail.com> wrote:

> Below is some code I'm currently testing.  I had to make one
> concession with
> an addition call to memset (still not sure why), but, other than
> that, it's
> been revamped more or less according to Scott's suggestions.
> 
> Unfortunately, I probaly won't have time to update/test all of the C
> code
> with such detail, and, as you guys pointed out, fussiness, but this
> chunk of
> code is used /a lot/, so it's worth it.
> 
> Thanks, Scott and David for the comments.

<snip - revised code>

If you need to use memset(), then you might as well go back to 
using malloc().  realloc() avoids some overhead only some of the 
time, quite possibly a small minority of the time.  But the memset()
adds overhead every time.

It is unfortunate that some bit of code demands to see a long 
series of null bytes in its buffers, because that code imposes
overhead indirectly on code that doesn't otherwise share the same
requirement.  If you found the culprit code you could probably 
find a way to eliminate that requirement -- but there might be a
similar bit of code somewhere else.  There's no good way to find
them all.

I looked over the growing_buffer code again and reassured myself 
that n_used does not include the terminal nul.  So appending
the new string fragment to buf + n_used is appropriate, whether
you use strcpy() or, in this case, memcpy().  On the other hand 
the size member reflects the total buffer size, including the 
terminal nul.

Next topic: buffer_fadd().

This function appends a printf-style formatted string to an existing
growing_buffer, using data values in a variable length argument list.

Internally, it uses vsnprintf() to do the printf-style formatting.
vsnprintf() writes to a supplied buffer, taking pains to avoid
overflowing it.

As it turns out, we call vsnprintf() twice.  The first call is hidden
in a call to va_list_size(), which we use to find out how big a
buffer we need.  

(Note that we call memset() to fill the buffer with binary zeros -- 
a pointless waste of CPU cycles, since vsnprintf() will ignore the
previous contents of the buffer.)

Having obtained a buffer, we then call vsnprintf() again to populate
it.

What bothers me is that vsnprintf() has to do most of the same work
twice, parsing the format string and figuring out how to represent
the values in the argument list.

I suggest that you not use va_list_size() here.  Call vsnprintf()
directly to format the string into a fixed size buffer -- say, 128
bytes.  If you pick an appropriate size, this fixed size buffer 
will be big enough most of the time, and you can pass it to
buffer_add() without further ado.  

If the buffer isn't big enough, vsnprintf() will say so.  Only then 
do you need to allocate a variable-sized buffer and call vsnprintf()
a second time.  In that case you're duplicating some effort after
all, but you're no worse off than you are now.

Actually you could call vsprintf() the second time instead of 
vsnprintf(), since you would have already ensured that overflow 
won't occur.

Scott McKellar
http://home.swbell.net/mck9/aargh/