On 07.02.25 20:56, ichthyo wrote:
Basically, as it stands now, XMLWrapper addresses two concerns:
- providing an API to the outside to navigate and add / retrieve values
- maintaining a context stack and handling nodes to add / convert values
So the second aspect is quite obviously something that could be extracted
into an inner component, which would then encapsulate the library access.
Hi Yoshimi-devs,
just a short notice: during the last weeks, I was quite occupied with other
stuff, but now I have picked up work on this topic. I made a draft / plan
for the refactoring and setup a simple test function to validate the
changes I make to the code.
As usual, I'll publish my work-in-progress code to my Github "yoshimi"
repository, this time on a feature branch "mxml", which I'll re-base
regularly on top of current master.
My plan looks as follows:
- create a copy of the existing XMLwrapper
- re-order existing functionality into three layers
+ the front-end used by Yoshimi core logic (I'll call this "XMLStore")
+ a new internal abstraction of an XML subtree (I'll call this "XMLtree")
+ a back-end or policy class which exposes some relevant base operations
of lib MXML, to the degree necessary to implement "XMLtree"
- next step is to simplify the XMLStore (front-End) and remove the
unnecessary statefulness (esp. the context stack adds a lot of complexity
and it seems it can be replaced by direct navigation functions of MXML)
- switch existing usages of XML access to the reworked code
- the direct dependencies to MXML can then be removed from Yoshimi core code
At this point, we should test and integrate; if all was done correct,
no externally visible behaviour of yoshimi is changed.
Then, in a second round, I'll install a local version of the new MXML v4
to my PC and add the necessary branching / adapters to the backend-code
plus some detection-code to pick the proper backend, based on the actually
visible definition (API).
----------------------------------------------------------------------
To support my work on this refactoring, I "sampled" / copied some
typical invocations of XML handling into a test function, which
can be triggered from the CLI.
While doing so, I discovered a problem in the existing code, which
just luckily gets never triggered by the code in current shape:
If you invoke `XMLwrapper::loadXMLfile()` followed immediately by
`XMLwrapper::getXMLdata()`, a memory corruption and possibly a SEGFAULT
happens. The reason is as follows
- the constructor of XMLwrapper not only creates the object,
but immediately populates it with data from a given Config instance
- `loadXMLfile()` throws away this baseline-tree, but fails to
re-initialise the XMLwrapper::info pointer.
- the `getXMLdata()` code uses a hidden additional control parameter,
which for some reason is passed through a public data field of a
Config instance (instead of just directly passing this control
value as additional parameter "xmltype" in plain sight)
- also for some reason I don't understand, the XMLtype field, which
is embedded into an <INFORMATION> node, is not added to the XML
tree proper. Rather, it is injected by side-effect prior to
rendering the tree into an string in XML format. It does so using
the XMLwrapper::info pointer. Which however is only properly
configured on a newly constructed XMLwrapper object, but not
after loadXMLfile() has thrown away the initial object contents.
Thus we're adding XML content through a dangling pointer and an already
de-allocated MXML data structure.
As said: in its current shape, the existing code never happens
to trigger this dormant problem. Thus IMHO it is sufficient when
I fix it as part of the refactoring, presumably by just not relying
on additional state but rather passing parameters directly.
-- Hermann
_______________________________________________
Yoshimi-devel mailing list
Yoshimi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
