[OPEN-ILS-DEV] PATCH: osrf_stack.c (miscellaneous)

[Scott McKellar]

This patch started out mostly as a refactoring, but it turned into a
bug fix.

1. In osrf_stack_transport_handler(): removed the memset() as 
   pointless.

2. Also in osrf_stack_transport_handler(), in the loop traversing
   arr[]: changed the loop condition from "i != num_msgs" to
   "i < num_msgs", for hygienic reasons.

3. Eliminated osrf_stack_message_handler().  The first half of it moved
   into its caller.  The second half moved into the two callees
   _do_client() and _do_server().  This refactoring made it possible, or
   at least easier, to eliminate a memory leak where _do_server() was
   failing to free the input osrfMessage.  I also eliminated a bug
   whereby we potentially tried to access a member of a freed
   osrfMessage.

4. osrf_stack_application_handler() now returns void instead of int,
   since we were ignoring the return value anyway.

----------------

Commentary, following the same numbering as above:

1. The memset() accomplished nothing, since nothing in the ensuing
code ever looks at the prior contents of the array.  The memset()
was inappropriate anyway, because we were using it to clear an 
array not of characters but of pointers.  A NULL pointer is not
necessarily represented by all-bits-zero.

2. A loop condition like "i != num_msgs" always makes me nervous,
because it invites serious misbehavior where i is initially
greater than num_msgs.  In this case that (currently) can't happen,
but there's no reason to tempt fate.

3. I found the old osrf_stack_message_handler function terribly 
confusing.  It received a pointer to osrfMessage, which it sometimes
freed and sometimes didn't.  It called _do_client() or _do_server() to 
get another pointer to osrfMessage, which was sometimes NULL and 
sometimes wasn't, and was sometimes the same as the original pointer.

As I continued to stare at the code, I realized that we had a memory
leak.  _do_server() never freed the osrfMessage that it received,
but osrf_stack_message_handler() seemed to assume that it had, at
least in some cases.

Toward the end, osrf_stack_message_handler() would sometimes issue a
debug message reporting the value of one of the members of the
original osrfMessage.  However in some cases, by the time we tried to
issue the message, we would have already destroyed that osrfMessage.
Oops.

In the course of unraveling this tangled skein, I rearranged
_do_client() and _do_server() so that they would each have a single
exit point.

Next I replicated the latter half of osrf_stack_message_handler() at
the end of _do_client() and _do_server().  Then with a little further
rearrangement I could eliminate the two bugs described above.  Also
I turned _do_client() and _do_server() into void functions, since
there was no longer anything to do with the pointers they were
returning.

By this point there wasn't much left of osrf_stack_message_handler() --
just a small if/else.  So I moved that into the calling code and
eliminated the function entirely.

4. Since osrf_stack_application_handler() no longer returns a value,
I could simplify it a bit.  It now selects between two branches of
code that don't really have anything to do with each other.  I *think*
that one branch is used only by _do_client() and the other only by
_do_server().  If so, there's no reason to keep them in the same
function.  We can either break them out into separate functions or
move them up into the respective callers.  However I didn't take that
step, because I wasn't sufficiently certain of my conclusions.

-----------------------

Please consider changing the comments associated with _do_client() and
_do_server(), because they may not make sense any more.

In particular I'm not sure what the "stack" is, or what "up" and "doen"
mean.  Function calls in C use a stack.  When I call a function, I think
of it as going *down* in the function hierarchy.  But when I'm stacking
some coins -- or books, since we're all librarians here -- the stack 
grows *up*, not down.  The main exception is if you're stacking helium 
balloons against the ceiling.

The new structure is hierarchical.  Each level of function gets a 
message, and either handles it or delegates the handling to somebody
else.  A "dispatcher" may be a better metaphor than a "stack."

----------------

This patch is a bit more intricate than usual, and I am not equipped
to test it properly myself.  I trust you will give it a good workout.

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

Developer's Certificate of Origin 1.1 By making a contribution to
this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license indicated
in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source license
and I have the right under that license to submit that work with
modifications, whether created in whole or in part by me, under the
same open source license (unless I am permitted to submit under a
different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person
who certified (a), (b) or (c) and I have not modified it; and

(d) In the case of each of (a), (b), or (c), I understand and agree
that this project and the contribution are public and that a record
of the contribution (including all personal information I submit
with it, including my sign-off) is maintained indefinitely and may
be redistributed consistent with this project or the open source
license indicated in the file.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: osrf_stack_c_3.patch
Type: text/x-patch
Size: 12278 bytes
Desc: not available
Url : http://libmail.georgialibraries.org/pipermail/open-ils-dev/attachments/20081205/47399712/attachment.bin