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

[Dan Scott]

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.

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).

-- 
Dan Scott
Laurentian University