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

[Mike Rylander]

On 5/21/07, Dan Scott <denials at gmail.com> wrote:
> Ahem. With attachment this time...
>
> On 21/05/07, Dan Scott <denials at gmail.com> wrote:
> > 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?
> >

Awesome, and thanks.  I'm thinking this should probably live next to
or near the srfsh example config.  It's not committed, but the xsd is
currently living in ILS/OpenSRF/examples/srfsh_config.xsd in my CVS
working dir.  I'll commit if there's general agreement that that's a
sane location.

The problem I see with embedding the xsd validation in the code is
finding the schema file itself.  The immediate workaround that comes
to mind is to extend the osrfConfigInit() API so that it can accept a
schema file path or URL (or NULL, to ignore it -- thanks, libxml2 ;)
), and gather that from the command line as part of the
troubleshooting process.  I don't have any problem with hosting the
xsd on the open-ils.org site, but I'd hate to require internet access
just to start srfsh, and it could become burdensome to the server if
we enabled validation by default in that case.

Thoughts?

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

Let me know what if and how you need the attribution to look (if
Laurentian requires it, or some such) and I'll make it so.

--miker

> > 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
> >
>
>
> --
> Dan Scott
> Laurentian University
>
>


-- 
Mike Rylander