[Freeipa-devel] Re: [PATCH] COLLECTION patches resending

Sumit Bose sbose at redhat.com
Fri Jul 10 21:15:51 UTC 2009


On Fri, Jul 10, 2009 at 11:48:26AM -0400, Dmitri Pal wrote:
> 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.

Are you sure that the program will crash or maybe just override some
random memory? If you use a macro you can call abort() if the value is
0 and the program will definitely exit.

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

ah, yeah, sorry, I shoot myself into the foot.

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

Yes, it is completely unrelated and worth a separate patch

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

These patches make sense to me, but I have to admit that I'm not too
familiar with the collection code. I

ACK

these two patches, but it would be nice if Simo or Stephen can have a
closer look on them before pushing them.

bye,
Sumit




More information about the Freeipa-devel mailing list