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

[Mike Rylander]

On 3/19/07, Scott McKellar <mck9 at swbell.net> wrote:
> 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.
>

Brave man!  As with any project, the developer to user ratio is very
small, and with so few users at this point -- well, you get the idea.

> 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.
>

Heh ... you managed to pick nearly the only utility that hasn't
actually been used.  It was written as a utility for data loading
(hence the name) but given the data we're working with (or, more
precisely, the problems therein) it's easier to deal with in Perl.

>
> 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.
>

Good catch.  Changed.

>
> 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.
>

It's inadvertent.  As you mention, they're not really an issue since
the program is about to exit, but I've added the free for method at
the end.  As far as freeing them when there's an early error, well,
the program ends at that point.  I'm not above letting the system
clean up after me.  ;)

>
> 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.
>

That's not an unreasonable expectation.  If this were built to last
then it would have evolved early exits out of necessity, but as it
wasn't actually used ...

It doesn't hurt anything other than wasting time, of course, but I've
added erroring exit in a couple places, as well as an explicit
rollback on transaction error.  The cstore app handles rolling back
broken or abandoned transactions, but again, better safe than sorry.

>
> 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?
>

The OpenSRF API is ... underdocumented.  For a good example of an
OpenSRF server in C I would suggest looking at the oils_auth.c source
in the same directory as oils_dataloader.c.  For a better example of a
C client I would suggest Open-ILS/src/extras/oils_requestor.c or
OpenSRF/src/srfsh/srfsh.c.  All were written by Bill Erickson, the
author of the C OpenSRF code, and get much more attention than
oils_dataloader.

For Perl examples, there is basically everything that lives
underOpen-ILS/src/perlmods/OpenILS/Application/ and
OpenSRF/src/perlmods/OpenSRF/Application/ for server-side examples.
There are quite a few Perl clients scattered about in CVS,
Open-ILS/src/support-scripts/marc_export being one of the newer
examples.

Thanks for the suggestions on oils_dataloader.  We'd love to clean up
a ton of the code, of course, and every little bit of outside help ...
er ... helps.

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


-- 
Mike Rylander
mrylander at gmail.com
GPLS -- PINES Development
Database Developer
http://open-ils.org