COLLECTION Fixed: iterator_up and insert_into_current

During a review of the previous patch the two issues
were found:
a) The col_iterator_up function was not implemented properly
so it got reworked. New implementation changes
the way error condition is handled. Comments were updated accordingly.
b) There was a mising check for validity of the argument in
the col_insert_into_current function. Check was added.
c) Unit test modified to reflect the change in functionality.

3 files changed, 83 insertions, 17 deletions

diff --git a/common/collection/collection.c b/common/collection/collection.c
index b4a2c8d5..1e3fe5c3 100644
--- a/common/collection/collection.c
+++ b/common/collection/collection.c
@@ -355,6 +355,10 @@ int col_insert_item_into_current(struct collection_item *collection,
                               "Adding header item to new collection.");
             collection = item;
         }
+        else {
+            TRACE_ERROR_STRING("Passed in item is invalid", "");
+            return EINVAL;
+        }
     }
     else {
         /* We can add items only to collections */
@@ -1243,6 +1247,9 @@ static int col_walk_items(struct collection_item *ci,
     TRACE_INFO_NUMBER("Mode flags:", mode_flags);
 
     /* Increase depth */
+    /* NOTE: The depth is increased at the entry to the function.
+     * and decreased right before the exit so it is safe to decrease it.
+     */
     (*depth)++;
 
     current = ci;
@@ -2605,18 +2612,23 @@ int col_bind_iterator(struct collection_iterator **iterator,
 
 /* Stop processing this subcollection and move to the next item in the
  * collection 'level' levels up.*/
-int col_iterate_up(struct collection_iterator *iterator, int level)
+int col_iterate_up(struct collection_iterator *iterator, unsigned level)
 {
     TRACE_FLOW_STRING("iterate_up", "Entry");
 
-    if ((iterator == NULL) || (level >= iterator->stack_depth)) {
+    if (iterator == NULL) {
         TRACE_ERROR_NUMBER("Invalid parameter.", EINVAL);
         return EINVAL;
     }
 
     TRACE_INFO_NUMBER("Going up:", level);
+    TRACE_INFO_NUMBER("Current stack depth:", iterator->stack_depth);
 
-    iterator->stack_depth--;
+    /* If level is big just move to the top,
+     * that will end the iteration process.
+     */
+    if (level >= iterator->stack_depth) iterator->stack_depth = 0;
+    else iterator->stack_depth -= level;
 
     TRACE_INFO_NUMBER("Stack depth at the end:", iterator->stack_depth);
     TRACE_FLOW_STRING("col_iterate_up", "Exit");
diff --git a/common/collection/collection.h b/common/collection/collection.h
index 7de17d2e..5483c138 100644
--- a/common/collection/collection.h
+++ b/common/collection/collection.h
@@ -544,9 +544,11 @@ int col_iterate_collection(struct collection_iterator *iterator,
  * collection 'level' levels up.
  * The 'Level' parameter indicates how many levels up you want to jump.
  * If 0 - call is a no op.
- * If the depth is less than requested level function will return error EINVAL.
+ * If the depth is less then requested level the iterator will
+ * get to the 0 level and next call to col_iterate_collection
+ * will return NULL item.
  */
-int col_iterate_up(struct collection_iterator *iterator, int level);
+int col_iterate_up(struct collection_iterator *iterator, unsigned level);
 
 /* How deep are we relative to the top level.
  * This function might report depth that might look misleading.
diff --git a/common/collection/collection_ut.c b/common/collection/collection_ut.c
index c0adf348..c50a8384 100644
--- a/common/collection/collection_ut.c
+++ b/common/collection/collection_ut.c
@@ -676,13 +676,14 @@ int iterator_test(void)
             return error;
         }
 
+        /* Are we done ? */
+        if (item == (struct collection_item *)(NULL)) break;
+
         depth = 0;
         col_get_item_depth(iterator, &depth);
         idepth = 0;
         col_get_iterator_depth(iterator, &idepth);
 
-        /* Are we done ? */
-        if (item == (struct collection_item *)(NULL)) break;
 
         printf("%*sProperty (%s), type = %d, data size = %d depth = %d idepth = %d\n",
                 depth * 4,  "",
@@ -695,13 +696,7 @@ int iterator_test(void)
         if ((strcmp(col_get_item_property(item, NULL), "id")==0) &&
            (*((int *)(col_get_item_data(item))) == 1)) {
             printf("\n\nFound property we need - go up!!!\n\n\n");
-            error = col_iterate_up(iterator, 5);
-            if (!error) {
-                printf("We expected error but got seucces - bad.\n");
-                col_unbind_iterator(iterator);
-                col_destroy_collection(peer);
-                return -1;
-            }
+
             /* This should work! */
             error = col_iterate_up(iterator, 1);
             if (error) {
@@ -925,9 +920,6 @@ int iterator_test(void)
         return error;
     }
 
-    /* This should also work becuase iterator holds to collection */
-    col_destroy_collection(peer);
-
     printf("\n\nIteration (7 show headers only no END):\n\n");
 
     do {
@@ -936,6 +928,7 @@ int iterator_test(void)
         error = col_iterate_collection(iterator, &item);
         if (error) {
             printf("Error (iterate): %d\n", error);
+            col_destroy_collection(peer);
             col_unbind_iterator(iterator);
             return error;
         }
@@ -954,6 +947,65 @@ int iterator_test(void)
     /* Do not forget to unbind iterator - otherwise there will be a leak */
     col_unbind_iterator(iterator);
 
+
+    /* Bind iterator */
+    error =  col_bind_iterator(&iterator, peer, COL_TRAVERSE_DEFAULT);
+    if (error) {
+        printf("Error (bind): %d\n", error);
+        col_destroy_collection(peer);
+        return error;
+    }
+
+    col_destroy_collection(peer);
+
+    printf("\n\nIterate up test:\n\n");
+
+    do {
+
+        /* Loop through a collection */
+        error = col_iterate_collection(iterator, &item);
+        if (error) {
+            printf("Error (iterate): %d\n", error);
+            col_unbind_iterator(iterator);
+            return error;
+        }
+
+        /* Are we done ? */
+        if (item == (struct collection_item *)(NULL)) break;
+
+        depth = 0;
+        col_get_item_depth(iterator, &depth);
+        idepth = 0;
+        col_get_iterator_depth(iterator, &idepth);
+
+
+        printf("%*sProperty (%s), type = %d, data size = %d depth = %d idepth = %d\n",
+                depth * 4,  "",
+                col_get_item_property(item, NULL),
+                col_get_item_type(item),
+                col_get_item_length(item),
+                depth,
+                idepth);
+
+        if (strcmp(col_get_item_property(item, NULL), "queue") == 0)  {
+
+            printf("\n\nFound property we need - go up!!!\n");
+            printf("Expect bail out of collection processing.\n\n");
+
+            /* This should work! */
+            error = col_iterate_up(iterator, 10);
+            if (error) {
+                printf("We expected success but got error %d\n", error);
+                col_unbind_iterator(iterator);
+                col_destroy_collection(peer);
+                return error;
+            }
+
+        }
+    }
+    while(1);
+
+    col_unbind_iterator(iterator);
     return EOK;
 }