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

[Dmitri Pal]

Sumit Bose wrote:
> 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 -- ?
>   
These unsigned values are the size of the internal stack or depth level.
They need to  always be positive this is why they are unsigned.
I do not want obfuscate the error by preventing them to go below 0.
If it goes the program will crash and we will see the problem.
If we add the check we most likely hide the problem if there is a coding
error.

> 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.
>
>   
I see the issue, thanks for pointing it to me but I do not like the patch.
Your patch will crash if collection is NULL in the first place since it
will check collection->type before checking the collection itself.
This issue however is not related to the patch I sent. Should I fix it
in this patch or include in the next one?
I would prefer next one since I have other things to do in collection
today.


> 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
>
>   
In future the index might be negative to indicate special value so I
would prefer  leaving it as int.

> and the level of col_iterate_up to unsigned to make it clear the
> unsigned values are expected?
>   
I see the problem. This function needs to be fixed.
Again, should it be done in this patch or a separate one?

> bye,
> Sumit
>   


-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/