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

[Dan Scott]

On 21/05/07, Mike Rylander <mrylander at gmail.com> wrote:
> On 5/21/07, Scott McKellar <mck9 at swbell.net> wrote:
> >
> > --- Mike Rylander <mrylander at gmail.com> wrote:
> >
> > <snip -- example of mangled configuration file>
> >
> > > Wow ... yeah, we should definitely ignore that -- assuming it parses
> > > properly.  I never would have thought to try that, honestly, and I
> > > can
> > > see no use for such orphaned text nodes.
> > >
> > > I didn't notice such a construct in any of the example config files,
> > > so I suppose this was an experiment on your part?
> >
> > Yes, it was an experiment.  I figured that sooner or later somebody
> > will do this with a config file.  It's valid XML, so it will get past
> > the XML parser, and we'd better be prepared to handle it.  There are
> > probably other perversities that we haven't dreamed up yet.
> >
> > > The key seems to be ILS/OpenSRF/src/utils/xml_utils.c -- specifically
> > > _xmlToJSON().  It should be simple enough to fix this, really.  We
> > > can
> > > just check to see if there is more than one child node for a given
> > > parent, and ignore text nodes in that case.  See
> > >
> > >
> > http://open-ils.org/cgi-bin/viewcvs.cgi/ILS/OpenSRF/src/utils/xml_utils.c?r1=1.2&r2=1.3&diff_format=l
> > >
> > > for my provisional approach to a fix (ignore the spacing :P), and
> > > thanks, as usual...
> >
> > _xmlToJSON() is part of the problem.  I was finding it hard to wrap
> > my head around how it works, so if you can find a fix before I can,
> > that's fine.
> >
> > The other part of the problem is jsonObjectSetKey().  It turns the
> > old node into an array node willy-nilly, without regard for what
> > sort of node it was originally.  In this case it had been a string
> > node.  The code winds up trying to treat a char * as if it were a
> > jsonObjectNode *, and  hilarity ensues.
> >
>
> I would certainly go along with putting some type checking in
> jsonObjectSetKey() and raising an error if the object in question
> already holds a value.
>
> > It may be a mistake to ignore such extraneous text.  The user may
> > have put it there for a reason, trying to specify some option or
> > something.  If we silently ignore the text, the user thinks the
> > config file is okay, and then can't figure out why the option didn't
> > take (or doesn't even notice).  It may be more helpful to respond
> > with a fatal error, or at least a warning.
>
> I think Dan's idea of a validating schema makes the most sense in this
> case, since that would allow us to simply update the schema in new
> versions if we wanted to allow such constructs.  That, or something
> like it (programmatic validation for configs), is something we've
> wanted to put some time into, but time being what it is ...
>
> >
> > When I get home I'll try out your fix and subject it to some more
> > torture tests.
> >
>
> Thanks.
>
> --miker
>
> > Scott McKellar
> > http://home.swbell.net/mck9/aargh/
> >
> >
>
>
> --
> Mike Rylander
>

Attached, please find a patch for an XML schema for the srfsh.xml
config format (srfsh_config.xsd), along with a valid srfsh.xml file
(srfsh.xml) and an invalid srfsh.xml file (srfsh_bad.xml) for testing
purposes. I placed them in the Open-ILS/src/extras/config directory
for now, possibly to be used in a manual check of the .srfsh.xml file
via something like:

xmllint --schema srfsh_config.xsd --noout .srfsh.xml

The right place to incorporate the use of the schema is probably in
orsfConfigInit() in osrfConfig.c, but I won't be doing that tonight :)
Where are those round tuits when you need them?

Hrm, I probably should have added a copyright statement to the schema.
Ah well; I can always catch it in post-production.

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.



-- 
Dan Scott
Laurentian University