[OPEN-ILS-DEV] C nits: utils.c

[Scott McKellar]

The other day I posted some remarks about the oils_dataloader.c
program,
only to find that this program isn't actually used.  So I decided to 
look at ILS/OpenSRF/src/utils/utils.c, on the theory that these 
functions have more of an impact.  Besides, being familiar with some 
low-level functions should make it easier to understand the higher 
level ones.

I intend to be thorough amd pedantic, if not in fact obnoxious.  That
doesn't mean that I expect anyone to jump up and rewrite everything to 
my liking.  It just means that I would rather be annoying than shrug
off
something that might have been helpful if I had pointed it out.  The
people who actually do the work are of course free to ignore anything I
say, and no doubt will in some cases, and maybe most cases.

---------

There are a number of calls to bzero().  It's not obvious to me that
there is any good reason to use this non-standard function, when it
can be trivially replaced by the standard function memset().

---------

safe_malloc() probably should have been named safe_calloc(), because
like calloc() it fills the newly allocated block of memory with binary
zeros.  I don't know why it does that.  In my experience it is rarely
useful to initialize fresh memory this way, because the code that needs
the memory will generally fill it with something else anyway.  However
I assume there is some reason.  In any case there is probably code
somewhere by now that relies upon this behavior.

Instead of having separate calls to malloc() and memset(), there may be
some virtue in making a single call to calloc().

The size parameter probably should have been a size_t instead of an
int.

Since malloc() returns a pointer to void, there is no point in casting
its return value to a pointer to void.  In fact there's never any good
reason to explicitly cast the return value of malloc() to any pointer
type, but that's a rant for another day.

---------

Following the definition of safe_malloc, there are declarations of two
variables: __global_argv and __global_argv_size.  These variables
should be renamed to g_argv,and g_argv_size, or something like that.
In C89, and I presume in C99 as well, identifiers beginning with two
underscores are reserved for use by the implementation.

(Likewise in utils.h some of the macros declare local variables named
__len.  This name too is reserved for use by the implementation, for
the same reason.)

In addition, I suspect that these variables should have been declared
static, because I suspect that they are nowhere referenced by name
outside of this source file.  I can't be sure because I haven't grepped
the whole code base for them.  However I don't see them declared in
utils.h, which is where I would expect them to be declared if they are
truly intended for external linkage.

----------

The two variables mentioned above are used by the next two functions,
init_proc_title() and set_proc_title().  I'm still scratching my head
over these functions, trying to figure out what they do for a living.
However I do have a few comments about init_proc_title().

This function receives argc and argv as if from a command line, and 
overwrites each of the strings in the argv array with binary zeros.
I don't know why it overwrites all of each string, instead of just the
first character in each string.  Maybe it merely applies the same
philosophy as safe_malloc().  Or maybe the reason will become clear to 
me once I see the code that calls this function.

If any of the pointers in argv[] is NULL, bad things will happen.

len should be declared not as an int but as a size_t.  That is the type
returned by strlen() and accepted by bzero().

__global_argv_size should possibly be declared as a size_t as well.
However size_t is an unsigned type.  If argc is ever zero, or if
argc == 1 and strlen( argv[ 0 ] ) < 2, __global_argv_size becomes a
negative number at the end.  I don't know what happens then.

---------

By no means am I finished with utils.c, but I'm going to skip to the
stringisnum function, which I noticed because it needs a new name.
Identifiers beginning with "str" followed by a lower case letter are
reserved for future library directions (at least in C89; I don't know
about C99).  And don't rename it "isstringnum", either.  That's
reserved too, because it starts with "is" followed by a lower case
letter.  I suggest "str_isnum".

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