[OPEN-ILS-DEV] PATCH: oils_requestor.c memory leaks

[Scott McKellar]

The attached patch plugs some memory leaks and tidies up a few other
things in ILS/Open-ILS/src/extras/oils_requestor.c.

1. I made the functions do_request() and format_response() static 
instead of global, since there appears to be no reference to them
elsewhere in the code base.

2. In main() I changed the type of c from char to int.  That is the
type returned by getopt(), and the type to which c is compared in 
the switch/case structure.  (Contrary to one's intuition, the type 
of a character literal in C is int, not char.) (Yes, I know I'm 
being really picky.)

3. In the main loop toward the end of main(), I contrived to free
the request allocated by readline().  Otherwise we leak a chunk of
memory with every iteration.

4. At the end of main() I freed several buffers that were potentially
allocated as copies of command line parameters.  We were already
freeing authtoken and idl_filename, so we might as well be consistent.
Alternatively we could free none of them, since we're about to exit
anyway.

Note that we still have memory leaks in the case of early exits due
to errors.  Whether these leaks are worth plugging depends on how
compulsive you like to be about such things.

Since I'm too lazy to do a full build and installation, I tested
with a rump version that had most of the functionality commented
out.  Then I copied my changes to an intact version of the code 
and made the patch.  Since I haven't actually compiled the version 
from which I made the patch, it is possible that I introduced some
error along the way.

Here are some further suggestions.  I can create a patch for any or
all of them but first I want to get some feedback.

1. The authtoken variable could almost certainly be declared as local
within main().  I see no reference to it elsewhere in the code base,
nor any reason for it to be global.

2. Likewise for the script variable, but I haven't looked at it
closely.

3. We accept a -s option and load its argument into the script
variable, but then we don't do anything with it.  If we're not going
to do anything with this option, we should get rid of it.  (There may
however be issues of backward compatibility, if this option is merely
obsolete.)

4. I doubt that we need to make copies of the command line arguments.
For example, instead of saying:

    username	= strdup(optarg);

...we could just say:

   username	= optarg;

Then we wouldn't have to worry about freeing those buffers.

Scott McKellar
http://home.swbell.net/mck9/aargh/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oils_requestor_c_1.patch
Type: text/x-patch
Size: 1865 bytes
Desc: 1955188130-oils_requestor_c_1.patch
Url : http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20070503/9804bf79/oils_requestor_c_1.bin