Re: [xml] bug in xmlSetProp?

[Daniel Veillard <veillard redhat com>]

On Sat, Nov 24, 2001 at 07:04:01PM -0500, Anthony Liguori wrote:
> These are the things addressed in my patch.
>
> 1) Passing node == NULL would cause a segmentation fault, the logic that seemed to
> be trying to achieved was to return NULL in this event.

  Agreed,

> 2) The logic of the function was somewhat redundant.  There were many instances of
> values being set to what their value was already set to.  This includes L4506
> where prop->last was being set to NULL when it's value was already set to NULL in
> L4498.  There are some other instances but they are a little more involved.

  And those were not functional flaws at worse lack of optimizations.
  You're asserting that you minimized the number of memory allocation done,
which is wrong, I may set prop->last twise, but you're setting it for each
iteration in the loop.

> 3) Most importantly, the function was not nearly as efficent as it could of been.
> This is evident in comparing the two sources.  The use of local variables
> (specifically tmp) is kind of nasty and makes the function a little confusing.

  I don't see why it's nasty, I used it as the variable used to walk the
list of children. Instead you overload the prop->last values to get this
function, this is IMHO poor coding style, and unlikely to get optimized
properly by the compiler.

> This is a fix for a simple bug - number 1 - that also involved what at my work we
> call an AEM.  Basically, a rewrite to improve performance and mantainability.
> There are still some more things I would of done, but it's not really worth
> over-analyzing the function :)  If it didn't fix the aforementioned bug, if I were
> you I probably wouldn't except it, but it does fix a bug after all (and not even
> the one I was trying to fix) ;-)

  But your new code introduced a serious bug. The last node in the prop
children list has prop->last->next == NULL, so you never set its parent
nor doc which actually mean that your patch destroys those properties
for nearly all the attributes in a document. This also seems to indicate
that you did not test your patch under gdb before sending it, this really
doesn't hint me to trust your patches in the future.
  
  "xmlChar *buffer = 0;" is IMHO poor coding style, NULL is the value to
use for end pointers.

  I ran "make tests" with your supplied patches and it broke in the
catalog test. Trying to run the tests for the XSLT library
it broke *everywhere*. At that point I reverted it and made the couple
of fixes needed starting from my existing code.

  In general before complaining about my code, make sure that your suggested
patch is not broken and has been seriously tested, at the very least that
it passes existing regression tests :-\

> encountered.  This patch isn't fixing a show stopping problem but I believe it
> does improve the overall code.  It's also my first hack of which will probably be
> many hacks to this library ;-)

  You may not fully understand all the requirements of my code, or the way
I design it, but before claiming you can do better, have the minimal courtesy
to check yours and verify your claims.

  My code had one error: the prop initialization.
  My code didn't allow you to call xmlSetProp with one of the buffer already
used for that property. 
  The first one is a bug, I fixed it thanks, the second was IMHO a request
for enhancement I did it too by moving the deallocation later. The patch
is in CVS:

 
http://cvs.gnome.org/bonsai/cvsquery.cgi?module=gnome-xml&branch=HEAD&branchtype=match&dir=gnome-xml&file=&filetype=match&who=veillard&whotype=match&sortby=Date&hours=&date=explicit&mindate=11%2F25%2F01+05%3A34&maxdate=11%2F25%2F01+05%3A36&cvsroot=%2Fcvs%2Fgnome

  You can send fixes, but please check them before. I will have to do 
that too, but spending an hours arguing with you about a broken patch is not
a really good use of both of us time, isn't it ?

Daniel

-- 
Daniel Veillard      | Red Hat Network https://rhn.redhat.com/
veillard redhat com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/