[Libvir] [PATCH] Fix the function that remove the element of the hash table.

[Daniel Veillard]

On Tue, Jan 29, 2008 at 02:45:44PM +0000, Daniel P. Berrange wrote:
> On Tue, Jan 29, 2008 at 11:32:04AM +0900, Hiroyuki Kaguchi wrote:
> > There are two logic error and a unnecessary else-statement
> > in virHashRemoveSet function.
> > 
> > This patch fix the following.
> > (1/3) The logic error that use released memory area.
> > (2/3) The logic error that doesn't remove elements.
> > (3/3) Unnecessary else-statement.
> 

  Okay, I checked that fnction is actually not coming from libxml2
it was derived from the existing call which removes a single element
and doesn't suffer the problem.

> > Index: hash.c
> > ===================================================================
> > RCS file: /data/cvs/libvirt/src/hash.c,v
> > retrieving revision 1.27
> > diff -u -r1.27 hash.c
> > --- hash.c	21 Jan 2008 16:29:10 -0000	1.27
> > +++ hash.c	28 Jan 2008 06:48:09 -0000
> > @@ -543,6 +543,7 @@
> >                  if (prev) {
> >                      prev->next = entry->next;
> >                      free(entry);
> > +                    entry = prev;
> >                  } else {
> >                      if (entry->next == NULL) {
> >                          entry->valid = 0;
> 
> ACK, this is definitely needed.

  Agreed,

> > @@ -553,6 +554,7 @@
> >                                 sizeof(virHashEntry));
> >                          free(entry);
> >                          entry = NULL;
> > +                        i--;
> >                      }
> >                  }
> >                  table->nbElems--;
> 
> I'm still not 100% clear on the logic around here, but I
> think your suggestion is correct.

  Yes, that's right, we are suppressing the first entry in the
clash list, and one way to continue exploring other entries in
the list is to set entry to NULL and decreasing the index. Note that
it could go to -1 until it reaches the outer loop where it will be
increased again to 0, so it's kind of nasty, but this works.
  Instead I changed the patch slightly to avoid this, move
table->nbElems-- before the unlinking, and then setting entry back to
the first element of the list, and issuing a continue to reenter the
  while (entry && entry->valid)
this is cleaner, more local, and avoid the -1 value for i.

> > @@ -560,8 +562,6 @@
> >              prev = entry;
> >              if (entry) {
> >                  entry = entry->next;
> > -            } else {
> > -                entry = NULL;
> >              }
> >          }
> >      }
> 
> ACK, clearly correct.

 yes that's superfluous.
I end up with the following patch, I will commit this shortly,

  thanks !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/
-------------- next part --------------
Index: hash.c
===================================================================
RCS file: /data/cvs/libxen/src/hash.c,v
retrieving revision 1.31
diff -u -p -r1.31 hash.c
--- hash.c	5 Feb 2008 19:27:37 -0000	1.31
+++ hash.c	7 Feb 2008 16:43:24 -0000
@@ -537,9 +537,11 @@ int virHashRemoveSet(virHashTablePtr tab
                 count++;
                 f(entry->payload, entry->name);
                 free(entry->name);
+                table->nbElems--;
                 if (prev) {
                     prev->next = entry->next;
                     free(entry);
+                    entry = prev;
                 } else {
                     if (entry->next == NULL) {
                         entry->valid = 0;
@@ -549,16 +551,14 @@ int virHashRemoveSet(virHashTablePtr tab
                         memcpy(&(table->table[i]), entry,
                                sizeof(virHashEntry));
                         free(entry);
-                        entry = NULL;
+                        entry = &(table->table[i]);
+                        continue;
                     }
                 }
-                table->nbElems--;
             }
             prev = entry;
             if (entry) {
                 entry = entry->next;
-            } else {
-                entry = NULL;
             }
         }
     }