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

[Mike Rylander]

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