[asterisk-commits] russell: branch 1.6.2 r261498 - in /branches/1.6.2: ./ main/heap.c

[SVN commits to the Asterisk project]

Author: russell
Date: Thu May  6 09:04:36 2010
New Revision: 261498

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=261498
Log:
Merged revisions 261496 via svnmerge from 
https://origsvn.digium.com/svn/asterisk/trunk

........
  r261496 | russell | 2010-05-06 08:58:07 -0500 (Thu, 06 May 2010) | 40 lines
  
  Fix handling of removing nodes from the middle of a heap.
  
  This bug surfaced in 1.6.2 and does not affect code in any other released
  version of Asterisk.  It manifested itself as SIP qualify not happening when
  it should, causing peers to go unreachable.  This was debugged down to scheduler
  entries sometimes not getting executed when they were supposed to, which was in
  turn caused by an error in the heap code.
  
  The problem only sometimes occurs, and it is due to the logic for removing an entry
  in the heap from an arbitrary location (not just popping off the top).  The scheduler
  performs this operation frequently when entries are removed before they run (when
  ast_sched_del() is used).
  
  In a normal pop off of the top of the heap, a node is taken off the bottom,
  placed at the top, and then bubbled down until the max heap property is restored
  (see max_heapify()).  This same logic was used for removing an arbitrary node
  from the middle of the heap.  Unfortunately, that logic is full of fail.  This
  patch fixes that by fully restoring the max heap property when a node is thrown
  into the middle of the heap.  Instead of just pushing it down as appropriate, it
  first pushes it up as high as it will go, and _then_ pushes it down.
  
  Lastly, fix a minor problem in ast_heap_verify(), which is only used for
  debugging.  If a parent and child node have the same value, that is not an
  error.  The only error is if a parent's value is less than its children.
  
  A huge thanks goes out to cappucinoking for debugging this down to the scheduler,
  and then producing an ast_heap test case that demonstrated the breakage.  That
  made it very easy for me to focus on the heap logic and produce a fix.  Open source
  projects are awesome.
  
  (closes issue #16936)
  Reported by: ib2
  Tested by: cappucinoking, crjw
  
  (closes issue #17277)
  Reported by: cappucinoking
  Patches:
        heap-fix.rev2.diff uploaded by russell (license 2)
  Tested by: cappucinoking, russell
........

Modified:
    branches/1.6.2/   (props changed)
    branches/1.6.2/main/heap.c

Propchange: branches/1.6.2/
------------------------------------------------------------------------------
Binary property 'trunk-merged' - no diff available.

Modified: branches/1.6.2/main/heap.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.6.2/main/heap.c?view=diff&rev=261498&r1=261497&r2=261498
==============================================================================
--- branches/1.6.2/main/heap.c (original)
+++ branches/1.6.2/main/heap.c Thu May  6 09:04:36 2010
@@ -92,13 +92,13 @@
 		int r = right_node(i);
 
 		if (l <= h->cur_len) {
-			if (h->cmp_fn(heap_get(h, i), heap_get(h, l)) <= 0) {
+			if (h->cmp_fn(heap_get(h, i), heap_get(h, l)) < 0) {
 				return -1;
 			}
 		}
 
 		if (r <= h->cur_len) {
-			if (h->cmp_fn(heap_get(h, i), heap_get(h, r)) <= 0) {
+			if (h->cmp_fn(heap_get(h, i), heap_get(h, r)) < 0) {
 				return -1;
 			}
 		}
@@ -229,14 +229,22 @@
 	}
 }
 
+static int bubble_up(struct ast_heap *h, int i)
+{
+	while (i > 1 && h->cmp_fn(heap_get(h, parent_node(i)), heap_get(h, i)) < 0) {
+		heap_swap(h, i, parent_node(i));
+		i = parent_node(i);
+	}
+
+	return i;
+}
+
 #ifdef MALLOC_DEBUG
 int _ast_heap_push(struct ast_heap *h, void *elm, const char *file, int lineno, const char *func)
 #else
 int ast_heap_push(struct ast_heap *h, void *elm)
 #endif
 {
-	int i;
-
 	if (h->cur_len == h->avail_len && grow_heap(h
 #ifdef MALLOC_DEBUG
 		, file, lineno, func
@@ -247,11 +255,7 @@
 
 	heap_set(h, ++(h->cur_len), elm);
 
-	for (i = h->cur_len;
-			i > 1 && h->cmp_fn(heap_get(h, parent_node(i)), heap_get(h, i)) < 0;
-			i = parent_node(i)) {
-		heap_swap(h, i, parent_node(i));
-	}
+	bubble_up(h, h->cur_len);
 
 	return 0;
 }
@@ -266,7 +270,7 @@
 
 	ret = heap_get(h, index);
 	heap_set(h, index, heap_get(h, (h->cur_len)--));
-
+	index = bubble_up(h, index);
 	max_heapify(h, index);
 
 	return ret;