Improved DirIter implementation.

libisofs/node.c

@@ -366,8 +366,9 @@ int iso_dir_get_children(const IsoDir *dir, IsoDirIter **iter)
         return ISO_OUT_OF_MEM;
     }
 
-    it->dir = dir;
+    it->dir = (IsoDir*)dir;
-    it->pos = dir->children;
+    it->pos = NULL;
+    it->flag = 0x00;
 
     *iter = it;
     return ISO_SUCCESS;
@@ -375,21 +376,47 @@ int iso_dir_get_children(const IsoDir *dir, IsoDirIter **iter)
 
 int iso_dir_iter_next(IsoDirIter *iter, IsoNode **node)
 {
-    IsoNode *n;
     if (iter == NULL || node == NULL) {
         return ISO_NULL_POINTER;
     }
-    n = iter->pos;
+    
-    if (n == NULL) {
+    /* clear next flag */
+    iter->flag &= ~0x01;
+    
+    if (iter->pos == NULL) {
+        /* we are at the beginning */
+        iter->pos = iter->dir->children;
+        if (iter->pos == NULL) {
+            /* empty dir */
             *node = NULL;
             return 0;
         }
-    if (n->parent != iter->dir) {
+    } else {
+        if (iter->pos->parent != iter->dir) {
             /* this can happen if the node has been moved to another dir */
+            /* TODO specific error */
             return ISO_ERROR;
         }
-    *node = n;
+        if (iter->pos->next == NULL) {
-    iter->pos = n->next;
+            /* no more children */
+            *node = NULL;
+            return 0;
+        } else {
+            /* free reference to current position */
+            iso_node_unref(iter->pos); /* it is never last ref!! */
+
+            /* advance a position */
+            iter->pos = iter->pos->next;
+        }
+    }
+    
+    /* ok, take a ref to the current position, to prevent internal errors
+     * if deleted somewhere */
+    iso_node_ref(iter->pos);
+    iter->flag |= 0x01; /* set next flag */
+    
+    /* return pointed node */
+    *node = iter->pos;
     return ISO_SUCCESS;
 }
 
@@ -406,11 +433,18 @@ int iso_dir_iter_has_next(IsoDirIter *iter)
     if (iter == NULL) {
         return ISO_NULL_POINTER;
     }
-    return iter->pos == NULL ? 0 : 1;
+    if (iter->pos == NULL) {
+        return iter->dir->children == NULL ? 0 : 1;
+    } else {
+        return iter->pos->next == NULL ? 0 : 1;
+    }
 }
 
 void iso_dir_iter_free(IsoDirIter *iter)
 {
+    if (iter->pos != NULL) {
+        iso_node_unref(iter->pos);
+    }
     free(iter);
 }
 
@@ -448,7 +482,7 @@ int iso_node_take(IsoNode *node)
     pos = iso_dir_find_node(dir, node);
     if (pos == NULL) {
         /* should never occur */
-        return ISO_ERROR;
+        return ISO_ASSERT_FAILURE;
     }
     *pos = node->next;
     node->parent = NULL;
@@ -494,41 +528,75 @@ IsoDir *iso_node_get_parent(IsoNode *node)
 /* TODO #00005 optimize iso_dir_iter_take */
 int iso_dir_iter_take(IsoDirIter *iter)
 {
-    IsoNode *pos;
+    IsoNode *pos, *pre;
     if (iter == NULL) {
         return ISO_NULL_POINTER;
     }
     
-    pos = iter->dir->children;
+    if (!(iter->flag & 0x01)) {
-    if (iter->pos == pos) {
+        return ISO_ERROR; /* next not called or end of dir */
-        return ISO_ERROR;
     }
-    while (pos != NULL && pos->next != iter->pos) {
+    
+    if (iter->pos == NULL) {
+        return ISO_ASSERT_FAILURE;
+    }
+    
+    /* clear next flag */
+    iter->flag &= ~0x01;
+    
+    pos = iter->dir->children;
+    pre = NULL;
+    while (pos != NULL && pos != iter->pos) {
+        pre = pos;
         pos = pos->next;
     }
     if (pos == NULL) {
-        return ISO_ERROR;
+        return ISO_ERROR; /* node not in dir */
     }
-    return iso_node_take(pos);
+    
+    if (pos != iter->pos) {
+        return ISO_ASSERT_FAILURE;
+    }
+    
+    /* dispose iterator reference */
+    iso_node_unref(iter->pos);
+    
+    if (pre == NULL) {
+        /* node is a first position */
+        iter->dir->children = pos->next;
+        iter->pos = NULL;
+    } else {
+        pre->next = pos->next;
+        iter->pos = pre;
+        iso_node_ref(pre); /* take iter ref */
+    }
+    
+    /* take pos */
+    pos->parent = NULL;
+    pos->next = NULL;
+    iter->dir->nchildren--;
+    return ISO_SUCCESS;
 }
 
 int iso_dir_iter_remove(IsoDirIter *iter)
 {
+    int ret;
     IsoNode *pos;
+    
     if (iter == NULL) {
         return ISO_NULL_POINTER;
     }
-    pos = iter->dir->children;
+    pos = iter->pos;
+    
+    ret = iso_dir_iter_take(iter);
+    if (ret == ISO_SUCCESS) {
+        /* remove node */
+        iso_node_unref(pos);
+    }
     if (iter->pos == pos) {
         return ISO_ERROR;
     }
-    while (pos != NULL && pos->next != iter->pos) {
+    return ret;
-        pos = pos->next;
-    }
-    if (pos == NULL) {
-        return ISO_ERROR;
-    }
-    return iso_node_remove(pos);
 }
 
 /**

libisofs/node.h

@@ -153,8 +153,15 @@ struct Iso_Special
  */
 struct Iso_Dir_Iter
 {
-    const IsoDir *dir;
+    IsoDir *dir;
+    
+    /* points to the last visited child, to NULL before start */
     IsoNode *pos;
+    
+    /* Some control flags.
+     * bit 0 -> 1 if next called, 0 reseted at start or on deletion 
+     */
+    int flag;
 };
 
 int iso_node_new_root(IsoDir **root);

test/test_node.c

@@ -410,6 +410,7 @@ void test_iso_dir_get_node()
     free(dir);
 }
 
+static
 void test_iso_dir_get_children()
 {
     int result;
@@ -436,6 +437,7 @@ void test_iso_dir_get_children()
     /* 1st node to be added */
     node1 = calloc(1, sizeof(IsoNode));
     node1->name = "Node1";
+    node1->refcount = 1;
     result = iso_dir_add_node(dir, node1, 0);
     CU_ASSERT_EQUAL(dir->nchildren, 1);
     
@@ -461,6 +463,7 @@ void test_iso_dir_get_children()
     /* add another node */
     node2 = calloc(1, sizeof(IsoNode));
     node2->name = "A node to be added first";
+    node2->refcount = 1;
     result = iso_dir_add_node(dir, node2, 0);
     CU_ASSERT_EQUAL(result, 2);
     
@@ -490,6 +493,7 @@ void test_iso_dir_get_children()
     /* addition of a 3rd node, to be inserted last */
     node3 = calloc(1, sizeof(IsoNode));
     node3->name = "This node will be inserted last";
+    node3->refcount = 1;
     result = iso_dir_add_node(dir, node3, 0);
     CU_ASSERT_EQUAL(result, 3);
     
@@ -521,12 +525,17 @@ void test_iso_dir_get_children()
     CU_ASSERT_PTR_NULL(node);
     iso_dir_iter_free(iter);
 
+    /* assert correct refcount */
+    CU_ASSERT_EQUAL(node1->refcount, 1);
     free(node1);
+    CU_ASSERT_EQUAL(node2->refcount, 1);
     free(node2);
+    CU_ASSERT_EQUAL(node3->refcount, 1);
     free(node3);
     free(dir);
 }
 
+static
 void test_iso_dir_iter_take()
 {
     int result;
@@ -551,6 +560,7 @@ void test_iso_dir_iter_take()
     /* 1st node to be added */
     node1 = calloc(1, sizeof(IsoNode));
     node1->name = "Node1";
+    node1->refcount = 1;
     result = iso_dir_add_node(dir, node1, 0);
     CU_ASSERT_EQUAL(dir->nchildren, 1);
     
@@ -581,6 +591,7 @@ void test_iso_dir_iter_take()
     
     node2 = calloc(1, sizeof(IsoNode));
     node2->name = "A node to be added first";
+    node2->refcount = 1;
     result = iso_dir_add_node(dir, node2, 0);
     CU_ASSERT_EQUAL(result, 2);
     
@@ -658,6 +669,7 @@ void test_iso_dir_iter_take()
      * let's insert a node after node2 and before node1 */
     node3 = calloc(1, sizeof(IsoNode));
     node3->name = "A node to be added second";
+    node3->refcount = 1;
     result = iso_dir_add_node(dir, node3, 0);
     CU_ASSERT_EQUAL(dir->nchildren, 3);
 
@@ -670,12 +682,177 @@ void test_iso_dir_iter_take()
 
     iso_dir_iter_free(iter);
 
+    /* assert correct refcount */
+    CU_ASSERT_EQUAL(node1->refcount, 1);
     free(node1);
+    CU_ASSERT_EQUAL(node2->refcount, 1);
     free(node2);
+    CU_ASSERT_EQUAL(node3->refcount, 1);
     free(node3);
     free(dir);
 }
 
+static
+void test_iso_dir_iter_remove()
+{
+    int result;
+    IsoDirIter *iter;
+    IsoDir *dir;
+    IsoNode *node, *node1, *node2, *node3;
+    
+    /* init dir with default values, not all field need to be initialized */
+    dir = malloc(sizeof(IsoDir));
+    dir->children = NULL;
+    dir->nchildren = 0;
+    
+    result = iso_dir_get_children(dir, &iter);
+    CU_ASSERT_EQUAL(result, 1);
+    
+    /* remove on empty dir! */
+    result = iso_dir_iter_remove(iter);
+    CU_ASSERT_TRUE(result < 0); /* should fail */
+    
+    iso_dir_iter_free(iter);
+    
+    /* 1st node to be added */
+    node1 = calloc(1, sizeof(IsoNode));
+    node1->name = "Node1";
+    node1->refcount = 2;
+    result = iso_dir_add_node(dir, node1, 0);
+    CU_ASSERT_EQUAL(dir->nchildren, 1);
+    
+    /* test iteration again */
+    result = iso_dir_get_children(dir, &iter);
+    CU_ASSERT_EQUAL(result, 1);    
+    
+    /* remove before iso_dir_iter_next() */
+    result = iso_dir_iter_remove(iter);
+    CU_ASSERT_TRUE(result < 0); /* should fail */
+    
+    result = iso_dir_iter_next(iter, &node);
+
+    /* this should remove the child */
+    result = iso_dir_iter_remove(iter);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_EQUAL(dir->nchildren, 0);
+    CU_ASSERT_PTR_NULL(dir->children);
+
+    result = iso_dir_iter_has_next(iter);
+    CU_ASSERT_EQUAL(result, 0);
+    
+    iso_dir_iter_free(iter);
+
+    /* add two node */
+    node1->refcount++; /* was removed above */
+    result = iso_dir_add_node(dir, node1, 0);
+    CU_ASSERT_EQUAL(dir->nchildren, 1);
+    
+    node2 = calloc(1, sizeof(IsoNode));
+    node2->name = "A node to be added first";
+    node2->refcount = 2;
+    result = iso_dir_add_node(dir, node2, 0);
+    CU_ASSERT_EQUAL(result, 2);
+    
+    result = iso_dir_get_children(dir, &iter);
+    CU_ASSERT_EQUAL(result, 1);
+
+    /* remove before iso_dir_iter_next() */
+    result = iso_dir_iter_remove(iter);
+    CU_ASSERT_TRUE(result < 0); /* should fail */
+    
+    /* iter should have two items... */
+    result = iso_dir_iter_next(iter, &node);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_PTR_EQUAL(node, node2);
+    
+    /* remove node 2 */
+    result = iso_dir_iter_remove(iter);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_EQUAL(dir->nchildren, 1);
+    CU_ASSERT_PTR_EQUAL(dir->children, node1);
+    
+    /* next should still work */
+    result = iso_dir_iter_next(iter, &node);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_PTR_EQUAL(node, node1);
+    
+    /* ...and no more */
+    result = iso_dir_iter_has_next(iter);
+    CU_ASSERT_EQUAL(result, 0);
+    
+    result = iso_dir_iter_next(iter, &node);
+    CU_ASSERT_EQUAL(result, 0);
+    CU_ASSERT_PTR_NULL(node);
+    iso_dir_iter_free(iter);
+    
+    /* now remove only last child */
+    node2->refcount++; /* was removed above */
+    result = iso_dir_add_node(dir, node2, 0);
+    CU_ASSERT_EQUAL(result, 2);
+
+    result = iso_dir_get_children(dir, &iter);
+    CU_ASSERT_EQUAL(result, 1);
+    result = iso_dir_iter_next(iter, &node);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_PTR_EQUAL(node, node2);
+    
+    result = iso_dir_iter_next(iter, &node);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_PTR_EQUAL(node, node1);
+    
+    /* take last child */
+    result = iso_dir_iter_remove(iter);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_EQUAL(dir->nchildren, 1);
+    CU_ASSERT_PTR_EQUAL(dir->children, node2);
+
+    result = iso_dir_iter_has_next(iter);
+    CU_ASSERT_EQUAL(result, 0);
+    
+    result = iso_dir_iter_next(iter, &node);
+    CU_ASSERT_EQUAL(result, 0);
+    CU_ASSERT_PTR_NULL(node);
+    iso_dir_iter_free(iter);
+    
+    /* Ok, now another situation. Modification of dir during iteration */
+    node1->refcount++;
+    result = iso_dir_add_node(dir, node1, 0);
+    CU_ASSERT_EQUAL(result, 2);
+
+    result = iso_dir_get_children(dir, &iter);
+    CU_ASSERT_EQUAL(result, 1);
+    result = iso_dir_iter_next(iter, &node);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_PTR_EQUAL(node, node2);
+    
+    /* returned dir is node2, it should be the node taken next, but
+     * let's insert a node after node2 and before node1 */
+    node3 = calloc(1, sizeof(IsoNode));
+    node3->name = "A node to be added second";
+    node3->refcount = 2;
+    result = iso_dir_add_node(dir, node3, 0);
+    CU_ASSERT_EQUAL(dir->nchildren, 3);
+
+    /* is the node 2 the removed one? */
+    result = iso_dir_iter_remove(iter);
+    CU_ASSERT_EQUAL(result, 1);
+    CU_ASSERT_EQUAL(dir->nchildren, 2);
+    CU_ASSERT_PTR_EQUAL(dir->children, node3);
+    CU_ASSERT_PTR_EQUAL(node3->next, node1);
+
+    iso_dir_iter_free(iter);
+
+    /* assert correct refcount */
+    CU_ASSERT_EQUAL(node1->refcount, 2); /* node1 is not removed */
+    free(node1);
+    CU_ASSERT_EQUAL(node2->refcount, 1);
+    free(node2);
+    CU_ASSERT_EQUAL(node3->refcount, 2); /* node3 is not removed */
+    free(node3);
+    free(dir);
+}
+
+static
 void test_iso_node_take()
 {
     int result;
@@ -690,6 +867,7 @@ void test_iso_node_take()
     /* 1st node to be added */
     node1 = calloc(1, sizeof(IsoNode));
     node1->name = "Node1";
+    node1->refcount = 1;
     
     /* addition of node to an empty dir */
     result = iso_dir_add_node(dir, node1, 0);
@@ -710,6 +888,7 @@ void test_iso_node_take()
     /* addition of a 2nd node, to be inserted before */
     node2 = calloc(1, sizeof(IsoNode));
     node2->name = "A node to be added first";
+    node2->refcount = 1;
     result = iso_dir_add_node(dir, node2, 0);
     CU_ASSERT_EQUAL(result, 2);
     
@@ -744,6 +923,7 @@ void test_iso_node_take()
     /* ...and a 3rd child, to be inserted last */
     node3 = calloc(1, sizeof(IsoNode));
     node3->name = "This node will be inserted last";
+    node3->refcount = 1;
     result = iso_dir_add_node(dir, node3, 0);
     CU_ASSERT_EQUAL(result, 3);
     
@@ -759,8 +939,12 @@ void test_iso_node_take()
     CU_ASSERT_PTR_NULL(node1->next);
     CU_ASSERT_PTR_NULL(node1->parent);
 
+    /* assert correct refcount */
+    CU_ASSERT_EQUAL(node1->refcount, 1);
     free(node1);
+    CU_ASSERT_EQUAL(node2->refcount, 1);
     free(node2);
+    CU_ASSERT_EQUAL(node3->refcount, 1);
     free(node3);
     free(dir);
 }
@@ -835,6 +1019,7 @@ void add_node_suite()
     CU_add_test(pSuite, "iso_dir_get_node()", test_iso_dir_get_node);
     CU_add_test(pSuite, "iso_dir_get_children()", test_iso_dir_get_children);
     CU_add_test(pSuite, "iso_dir_iter_take()", test_iso_dir_iter_take);
+    CU_add_test(pSuite, "iso_dir_iter_remove()", test_iso_dir_iter_remove);
     CU_add_test(pSuite, "iso_node_take()", test_iso_node_take);
     CU_add_test(pSuite, "iso_node_set_name()", test_iso_node_set_name);
 }