Re: [xml] bug in xmlSetProp?

[Anthony Liguori <ajl13 bellatlantic net>]

>    Yes, and I prefer to avoid adding tons of checks like this. If
> you know what you're doing, then prop->children->content might be useful.
> If unsure, use the API xmlGetProp()

I understand your point fully.  I completely agree.  But to me, the way this
function is behaving is a bit odd.  It takes a string with every intention of
copying it, but then waits a very long time to actually copy it.  Normally
this wouldn't be bad, but in the interim it is deleting memory.  It is
thereby making many, many assumptions about the nature of the buffer that's
being passed in.

It's not really about putting in a check to allow better use of the internal
data structure, but just about a better design of the function.  Here is
another patch that is a total rework of the logic of the function.  There is
actually another problem in this function in how it handles a NULL node
argument.  This patch also addresses that bug.  I understand that this is a
relatively large project so nit-picky things like this aren't of huge concern
but hey, thought I'd try atleast :)

Regards,
Anthony


*** tree.orig.c Sat Nov 24 17:07:30 2001
--- tree.c      Sat Nov 24 17:10:16 2001
***************
*** 4483,4520 ****
   */
  xmlAttrPtr
  xmlSetProp(xmlNodePtr node, const xmlChar *name, const xmlChar *value) {
!     xmlAttrPtr prop = node->properties;
!     xmlDocPtr doc = NULL;
  
      if ((node == NULL) || (name == NULL))
        return(NULL);
      doc = node->doc;
      while (prop != NULL) {
          if ((xmlStrEqual(prop->name, name)) &&
            (prop->ns == NULL)){
!           if (prop->children != NULL) 
!               xmlFreeNodeList(prop->children);
!           prop->children = NULL;
!           prop->last = NULL;
! 
!           if (value != NULL) {
!               xmlChar *buffer;
!               xmlNodePtr tmp;
  
                buffer = xmlEncodeEntitiesReentrant(node->doc, value);
                prop->children = xmlStringGetNodeList(node->doc, buffer);
-               prop->last = NULL;
-               prop->doc = doc;
-               tmp = prop->children;
-               while (tmp != NULL) {
-                   tmp->parent = (xmlNodePtr) prop;
-                   tmp->doc = doc;
-                   if (tmp->next == NULL)
-                       prop->last = tmp;
-                   tmp = tmp->next;
-               }
                xmlFree(buffer);
            }
            return(prop);
        }
        prop = prop->next;
--- 4483,4522 ----
   */
  xmlAttrPtr
  xmlSetProp(xmlNodePtr node, const xmlChar *name, const xmlChar *value) {
!     xmlAttrPtr prop;
!     xmlDocPtr doc;
  
      if ((node == NULL) || (name == NULL))
        return(NULL);
+ 
+     prop = node->properties;
      doc = node->doc;
+ 
      while (prop != NULL) {
          if ((xmlStrEqual(prop->name, name)) &&
            (prop->ns == NULL)){
!           xmlChar *buffer = 0;
  
+           if (value != NULL)
                buffer = xmlEncodeEntitiesReentrant(node->doc, value);
+ 
+           if (prop->children != NULL) {
+               xmlFreeNodeList(prop->children);
+               prop->children = NULL;
+           }
+ 
+           if (buffer) {
                prop->children = xmlStringGetNodeList(node->doc, buffer);
                xmlFree(buffer);
            }
+ 
+           prop->last = prop->children;
+           while (prop->last != NULL && prop->last->next != NULL) {
+                 prop->last->parent = (xmlNodePtr) prop;
+                 prop->last->doc = doc;
+                 prop->last = prop->last->next;
+           }
+ 
            return(prop);
        }
        prop = prop->next;