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

[Scott McKellar]

Here are some miscellaneous observations about various functions
in util.c.

Several parameters should carry the const qualifier (I mentioned a
couple of them earlier):

  set_proc_title, format parameter
  va_list_size, format parameter
  va_list_to_string, format parameter
  buffer_add, data parameter
  md5sum, text parameter

va_list_size() adds +2 to the length reported by vsnprintf().  I
don't see why we do that.  I can understand adding +1 to allow
for a terminal nul (which vsnprintf() doesn't count).  The +2
is rather mysterious to me.  A comment would be helpful to the
naive reader.

buffer_data() doesn't protect itself against a null pointer parameter
or a null buffer pointer in the growing_buffer.  Likewise, uescape()
doesn't protect itself against a nul string pointer, and md5sum()
doesn't protect itself against a null text parameter.

Not being familiar with UTF8 strings, I haven't quite figured out
what uescape() does.  However I noticed that at the end it does
an unnecessary malloc(), and an extra layer of copying.  Instead of:

	char* d = buffer_data(buf);  /* malloc() happens here */
	buffer_free(buf);
	return d;

It could do:

	char* d = buf->buf;
	buf->buf = NULL;
	buffer_free(buf);
	return d;

or even:

	char* d = buf->buf;
	free(buf);
	return d;

Finally, I have a number of remarks about file_to_string().

This function has two calls to bzero().  Each call (except for the
last one in the loop) is followed by calls to fgets() and buffer_add(),
of which neither has the slightest interest in whether the buffer
is padded with nul bytes.  These bzeros do nothing useful.

Before calling fclose(), depending on one's level of paranoia, it
might be useful to call ferror() to see if we ran into an IO error 
of some kind.

If we don't successfully open the file, we leak memory by not
freeing buf.

We format the error message into a local buffer of length "l".  A
lower case L is a poor choice for a variable name because it's
difficult to distinguish from the digit "1".  It's as bad as a 
variable named "O" (upper case o).

The calculation of the message length adds a hard-coded 64 to the
length of the file name.  This works, because the length of the
format string is less than 64.

However, maybe someone will change the text some day.  For example
they might translate it into French, and produce a longer message.  
It would be easy to overlook the need to change the hard-coded 64 
to something bigger.  This mistake wouldn't show up in testing unless
they specifically tested for a failure of fopen().

Something like the following would be more robust in the face of
maintenance:

    static const char msg[] = "Unable to open file [%s] in " 
                              "file_to_string()";
    size_t msg_len = strlen( filename ) - 1 + sizeof( msg );
    char b[ msg_len ];

This calculation is exact -- no guesswork.  sizeof( msg ) includes
the terminal nul at the end of msg[], so that adds 1.  But the
formatting replaces the conversion specification "%s", removing
two characters.  The net result is two string lengths minus one.

At the end of this function, as at the end of uescape(),  We can
avoid a malloc() and some copying.by returning a pointer to the 
internal buffer of the growing_buffer gb, instead of making a copy 
of it.

If we're loading a file of any significant length, eliminating this 
layer of copying, along with the needless bzeros, will save a lot of
CPU cycles.

Scott McKellar
27947