[Freeipa-devel] [PATCH] COLLECTION Adding flat traversal & copy

[Sumit Bose]

On Thu, Jul 09, 2009 at 06:34:58PM -0400, Dmitri Pal wrote:
>     COLLECTION Adding flat traversal & copy
>    
>     The collection is hierarchical. The flattening
>     of the collection was not implemented before
>     both for traversal and copying. This patch
>     introduces functionality to traverse or
>     iterate through collection as flat set
>     and also copy collection into another flattening
>     it and automatically resolving conflicts.
>     Also improved traceability and fixed memory leak
>     in unbind iterator code.
> 
> Sorry it grew a bit due to me adding extensive unit tests for different
> cases (and fixing things as I uncovered them).
> 1) Checked for tabs
> 2) Ran UT in valgrind
> 3) Checked for warnings in special build using Simo's scripts
>  
> 
Hi Dmitri,

the patch applies and compiles well.

There are a couple of '--' on unsigned values in this patch. I have not
checked if this is always save. Maybe it makes sense to add some
decrement macro which checks for 0 before doing -- ?

While tracking some signed/unsigned comparisons (-Wextra will show them)
I have found an issue in col_insert_item_into_current. If
collection == NULL and collection->type != COL_TYPE_COLLECTION,
collection will be NULL the first time it is accessed. The attached
patch will solve this.

Concerning signed/unsigned comparisons. What do you think about setting
the idx/index arguements of:
- col_insert_item_into_current
- col_extract_item_from_current
- col_extract_item
- col_insert_item

and the level of col_iterate_up to unsigned to make it clear the
unsigned values are expected?

bye,
Sumit
-------------- next part --------------
>From 7d39704baab85118c16ebad22641a38e086440e9 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Fri, 10 Jul 2009 12:14:32 +0200
Subject: [PATCH] fix for a potential segfault

---
 common/collection/collection.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/common/collection/collection.c b/common/collection/collection.c
index b4a2c8d..a2ba328 100644
--- a/common/collection/collection.c
+++ b/common/collection/collection.c
@@ -346,24 +346,21 @@ int col_insert_item_into_current(struct collection_item *collection,
         return EINVAL;
     }
 
+    /* We can add items only to collections */
+    if (collection->type != COL_TYPE_COLLECTION) {
+        TRACE_ERROR_STRING("Attempt to add item to non collection.","");
+        TRACE_ERROR_STRING("Collection name:", collection->property);
+        TRACE_ERROR_NUMBER("Collection type:", collection->type);
+        return EINVAL;
+    }
+
     if (collection == NULL) {
         TRACE_INFO_STRING("col_insert_item_into_current",
                           "Collection accepting is NULL");
-        if (item->type == COL_TYPE_COLLECTION) {
-            /* This is a special case of self creation */
-            TRACE_INFO_STRING("col_insert_item_into_current",
-                              "Adding header item to new collection.");
-            collection = item;
-        }
-    }
-    else {
-        /* We can add items only to collections */
-        if (collection->type != COL_TYPE_COLLECTION) {
-            TRACE_ERROR_STRING("Attempt to add item to non collection.","");
-            TRACE_ERROR_STRING("Collection name:", collection->property);
-            TRACE_ERROR_NUMBER("Collection type:", collection->type);
-            return EINVAL;
-        }
+        /* This is a special case of self creation */
+        TRACE_INFO_STRING("col_insert_item_into_current",
+                          "Adding header item to new collection.");
+        collection = item;
     }
 
     /* After processing flags we can process disposition */
-- 
1.6.2.5