[Freeipa-devel] [PATCH] COLLECTION patches resending

[Dmitri Pal]

Dmitri Pal wrote:
> 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?
>>   
>>     
Original patch

    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.

Correcting patch to address issues mentioned above.

    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 missing 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.

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-COLLECTION-Fixed-iterator_up-and-insert_into_curren.patch
Type: text/x-patch
Size: 7626 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090710/6069471d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-COLLECTION-Adding-flat-traversal-copy.patch
Type: text/x-patch
Size: 46962 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090710/6069471d/attachment-0001.bin>