[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