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

[Mike Rylander]

On 5/21/07, Dan Scott <denials at gmail.com> wrote:
> On 21/05/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.
> >
> > 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.
> >
> > When I get home I'll try out your fix and subject it to some more
> > torture tests.
> >
> > Scott McKellar
> > http://home.swbell.net/mck9/aargh/
> >
>
> Yeesh. Well, I haven't looked into this part of the code, so take this
> for what it's worth, but it sounds like what you want is either an XML
> schema that can validate the config file to enforce a valid config
> file (with reasonable error messages to boot), or to use precise XPath
> expressions to extract the pieces that you want from the config file
> and just ignore anything else for the tolerant approach. Or a
> combination of the two, if you opt to soften the schema validation to
> just emitting warnings rather than a flat-out error. Either of these
> approaches should be straightforward if you're relying on something
> like libxml2.
>

We're using libxml2 here.  The only place we're poking at xml directly
is in the low-level perl jabber client, as the SAX[2] implementations
for perl have some undesirable behaviors, and DOM parsing on a stream
is .... well, not DOM-ish.  :)

> If you've rolled your own XML parser, though, I probably should just
> stay out of this discussion (and well away from this bit of code).

Jump in, the water's fine!

>
> --
> Dan Scott
> Laurentian University
>


-- 
Mike Rylander