[OPEN-ILS-DEV] PATCH: osrf_system.c

[Scott McKellar]

Your revision looks pretty good to me, except that you forgot to 
change the logged messages for missing usernames and passwords.  
They're still complaining about domains -- an obvious copy-and-modify 
without the modify.

I like the way your rearrangement lets you consolidate the frees.
I might have suggested something similar but I didn't know which 
fields should be treated as required.  

If more than one required field is missing, you report all of them,
instead of bailing out on the first one.  That's the right thing
to do.

I also note that you eliminated a strdup() of the domain.

If the config file doesn't specify a log file, you're still
returning a -1 flavor of "success" without actually connecting.  I
don't know if that's because that's really how it should work or if
you don't want to address that issue in the same patch.

I intend to keep working on the configuration/logon stuff.  It isn't
very sexy but it's important that it be robust.  Most users don't
configure things often enough to really know what they're doing.
When something goes wrong -- as it inevitably will -- the system 
should say exactly what the problem is, so far as possible.  Coredumps
and segfaults leave the victims feeling helpless as well as 
frustrated. 

Here's a teaser for you.  If the configuration file contains extra
text between elements -- i.e. not within tag delimiters -- we get a
segfault when we try to translate the XML into JSON objects.  I
pretty much know why it's happening but I haven't figured out how
to fix it yet.

One thing I don't know is: should the extra text be reported as a
fatal error, logged in a warning message, or silently ignored?  Are
there cases where such extra text is legitimate?

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

--- Mike Rylander <mrylander at gmail.com> wrote:

> Scott,
> 
> I've gathered up some of your surrounding commentary on osrf_system.c
> (username potentially causing segfault, etc) and extended this patch
> to include checks for other required fields.  I applied some
> reordering, so that the log file is checked first (so we can do some
> logging) but generally followed your convention.  If you have a
> minute, please take a look at
> 
>
http://open-ils.org/cgi-bin/viewcvs.cgi/ILS/OpenSRF/src/libstack/osrf_system.c?r1=1.33&r2=1.31&diff_format=l
>   or
> http://ln-s.net/XWF
> 
> and eyeball the diff for anything obvious.
> 
> Thanks a ton for the patch ... keep 'em coming!
> 
> --miker