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

[Scott McKellar]

I don't know if this is the right forum for this post.
 I see a lot
of discussion of configuration and build issues, and
very little
about the actual code.  But I shall forge ahead
anyway.

I just started wading into the C source code (nosing
about in other
people's code is one of my hobbies), starting
arbitrarily with
oils_dataloader.c.  I haven't gotten very far yet, but
I have a
few comments.


1.  Near the end of main() there is a loop reading
characters from
stdin and stuffing them into a buffer.  The return
value from
getchar() is immediately cast from an int to a plain
char.

A plain char behaves like either a signed char or an
unsigned char,
depending on the compiler, and sometimes on compiler
options.  If
it behaves as unsigned, it will be unable to represent
EOF, which
has to be negative.

If this program is compiled in an environment where
plain char is
unsigned, and it reads an input file that contains no
newline, it
will fail to recognize the end of the file.  It will
go into a
loop, appending some character value (probably hex FF)
to the json
buffer over and over until the buffer runs out of
memory, or until
something else bad happens.

I suggest that c be declared an int, and that the cast
associated
with getchar() be eliminated.


2.  There are some minor memory leaks involving
config, context,
and method, each of which points to a dynamically
allocated
buffer.  The method buffer is never freed.  The other
two are
freed in mid-function -- unless there is an early exit
due to an
error condition.

In each case, the leak occurs just before exiting the
program.
Arguably it would be a waste of CPU cycles to free the
buffers
at this point, since the program is about to end
anyway.  However the
program takes pains to free two of them in the middle,
as noted
above.  I can't tell whether this inconsistency is
deliberate or
inadvertent.


3. I'm a bit surprised that the program returns 0 even
if
startTransaction() or endTransaction() fails.  In fact
I sort of
expected that the program would exit early if
startTransaction()
failed.  Since don't know my way around OpenSRF yet, I
can't tell
whether my naive expectations are reasonable.


4.  I've looked around your website, but so far I
haven't found any
systematic documentation of the API for OpenSRF.  I
expect I can
eventually stumble across the OpenSRF source code and
figure it out,
but in the meanwhile it's difficult to guess what much
of the code
is doing.  Is there some kind of shortcut available?


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