Re: [Libxmlplusplus-general] write() broken, maybe fixed

[Christophe de VIENNE <cdevienne alphacent com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Le Lundi 16 Décembre 2002 12:50, Murray Cumming a écrit :
> On Mon, 2002-12-16 at 11:35, Christophe de VIENNE wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > Le Lundi 16 Décembre 2002 10:27, Murray Cumming a écrit :
> > > The change to Node::write() just before the 0.17 release broke that
> > > method, leading to segfaults in some situations, and at least not
> > > writing out all child nodes in the dom_build example.
> >
> > Well, I've just tried this and had no problems. The output is the same as
> > the source file, except for the namespace which is removed.
>
> That's not what I saw. I guess I could download the release and
> double-check.

Would you mind trying again ? I've just done it (see output at the end of the 
mail), and I really don't see any difference...

>
> > I did the change to Node::write() because the output was different.
>
> Yes, but the change was more than what you described in the ChangeLog.
> That fix is still there.

Agree with you that my log should have been more precise. But I still believe 
it was needed.
The eventual other way to do is to use xmlNewChild only on a non-text node, 
and if it's a text-node, call xmlNodeSetContent on the parent node.
But calling xmlNodeSetContent on a freshly created node (obtained with 
xmlNewChild) will create another node of type XML_TEXT_NODE if we don't set 
the type first (see the implementation in tree.c).

>
> > The segfault happen when the incoming node tree is incorrect (a text node
> > with children).
>
> I don't see how that is possible. It's impossible to write such an XML
> file, even by hand.

Quoting you :
"
	This was leading to segfaults when write() later tried to add a child node
        to a text node.
"

If write() tries to add a node on a text node, it can be only because we have 
a node with _content not empty AND some children nodes.
I don't see any other case were it is possible.

>
> > Running the example with this patch and writing the resulting tree to a
> > new file, I've something like :
> >
> >       1 <?xml version="1.0"?>
> >       2 <Helping>
> >       3   <Jobs>
> >       4     <Job>
> >       5       <Project ID="3"/>
> >       6       <Application>
> >       7         <text>GBackup</text>
> >       8       </Application>
> >       9       <Category>
> >      10         <text>Development</text>
> >      11       </Category>
> >      12       <Update>
> >      13         <Status>
> >      14           <text>Open</text>
> >      15         </Status>
> >      16         <Modified>
> >      17           <text>Mon, 07 Jun 1999 20:27:45 -0400 MET DST</text>
> >      18         </Modified>
> >      19         <Salary>
> > [...]
>
> The problem here seems to be only when writing stuctures that have been
> read in by libxml++ itself. Maybe you should add or modify an example to
> do this, so we will know if it works.

I made a few demonstration program :

#include <libxml++/libxml++.h>

#include <iostream>

int main()
{
        xmlpp::DomParser parser;
        xmlpp::Node * root = parser.set_root_node("myroot", "", "");
        xmlpp::Node * tmp = root->add_child("foo");
        tmp = tmp->add_child("bar");

        tmp->add_content("Some content");

        parser.write_to_file("tmp.xml");

        return 0;
}


The tmp.xml file is the following :

With libxml++-0.17.0 compiled from release package :
<?xml version="1.0"?>
<myroot>
<foo>
<bar>Some content</bar>
</foo>
</myroot>


With the current cvs version :
<?xml version="1.0"?>
<myroot>
<foo>
<bar>
<>Some content</>
</bar>
</foo>
</myroot>



I personnaly don't see anything wrong in the 0.17 version, and the same 
problem as above with the cvs version...


May I ask you how did you add the content to your nodes in your app ? using 
add_content or set_content ? In the later case this accessor should be used 
only on the text node itself, not on it's parent. And this could explain how 
could some nodes have both a non empty _content and some children.


>
> I think the problem here is that we are adding text nodes as regular
> nodes instead of justing using xmlNodeSetContent(). Can't we just check
> for node->type == XML_TEXT_NODE before adding it in write()?
>
> I think we really should use the libxml functions instead of modifying
> the structs directly.

may be, but for me the 0.17 version is perfectly working, unless somebody 
demonstrates me I'm wrong.


I may be wrong or completely blind, and if it the case I apologise for this 
last minute bug and all this noise. But right now I need to be convinced.


Regards,


Christophe


PS: single test of read then write a file :

4 cdevienne asticot:~> cd libxml++-0.17.0/
5 cdevienne asticot:~/libxml++-0.17.0> ./configure && make >/dev/null
[snip]
6 cdevienne asticot:~/libxml++-0.17.0> cd examples/dom_parser/
7 cdevienne asticot:~/libxml++-0.17.0/examples/dom_parser> vim main.cc
[added this line :
     81       parser.write_to_file("tmp.xml");
]
8 cdevienne asticot:~/libxml++-0.17.0/examples/dom_parser> make
[snip]
9 cdevienne asticot:~/libxml++-0.17.0/examples/dom_parser> ./example 
example.xml
[snip]
10 cdevienne asticot:~/libxml++-0.17.0/examples/dom_parser> cat tmp.xml
<?xml version="1.0"?>
<Helping>
  <Jobs>
    <Job>
      <Project ID="3"/>
      <Application>GBackup</Application>
      <Category>Development</Category>
      <Update>
        <Status>Open</Status>
        <Modified>Mon, 07 Jun 1999 20:27:45 -0400 MET DST</Modified>
        <Salary>USD 0.00</Salary>
      </Update>
      <Developers>
        <Developer>
        </Developer>
      </Developers>
      <Contact>
        <Person>Nathan Clemons</Person>
        <Email>nathan windsofstorm net</Email>
        <Company>
        </Company>
        <Organisation>
        </Organisation>
        <Webpage>
        </Webpage>
        <Snailmail>
        </Snailmail>
        <Phone>
        </Phone>
      </Contact>
      <Requirements>
      The program should be released as free software, under the GPL.
      </Requirements>
      <Skills>
      </Skills>
      <Details>
      A GNOME based system that will allow a superuser to configure
      compressed and uncompressed files and/or file systems to be backed
      up with a supported media in the system.  This should be able to
      perform via find commands generating a list of files that are passed
      to tar, dd, cpio, cp, gzip, etc., to be directed to the tape machine
      or via operations performed on the filesystem itself. Email
      notification and GUI status display very important.
      </Details>
    </Job>
  </Jobs>
</Helping>
12 cdevienne asticot:~/libxml++-0.17.0/examples/dom_parser>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iEYEARECAAYFAj394KIACgkQB+sU3TyOQjAUogCgvBWDjxAT+qXQ0nUzwbfa/2VD
deQAn1oLi+tDvWJsrbaBrGWr4nHUnrQF
=vQ3F
-----END PGP SIGNATURE-----